search

jitsi / jitsi-meet

Jitsi Meet - Secure, Simple and Scalable Video Conferences that you use as a standalone app or embed in your web application.
https://jitsi.org/meet
Apache License 2.0
23.1k stars 6.71k forks source link

Cancelling `Reload site` prompt still refreshes the page #15198

Open bilogic opened 1 week ago

bilogic commented 1 week ago

What happened?

https://github.com/user-attachments/assets/85f739eb-bb15-4d28-9fb4-ce40eb12bc97

  1. Start a recording
  2. Press F5, Reload site pops up
  3. Click Cancel
  4. Page refreshes, and recording is downloaded
  5. I expected the page would not refresh, and recording continues

My jitsi runs in docker using this commit https://github.com/jitsi/jitsi-meet/commit/24ae69348bdafe7dba229dcc8fa8d943c16e9469

Platform

Browser / app / sdk version

Chrome 129.0.6668.100 (Official Build) (64-bit) /

Relevant log output

No response

Reproducibility

More details?

No response

damencho commented 1 week ago
  • Press F5, Reload site pops up

We do not have any such reload, this is handled by the browser not jitsi-meet, isn't it?

bilogic commented 1 week ago

Yes, the prompt showed probably because jitsi-meet told Chrome that there is some dirty info on the page.

So the issue here is probably because jitsi-meet did not handle the return.

damencho commented 1 week ago

Can you show a screenshot of that dialog? There is no such thing handled in jitsi-meet. So I don't see what we can do about it.

bilogic commented 1 week ago

image

  1. Are you able to repro 1-4 of my OP?
  2. If not, which step were you stuck at on meet.jit.si?
  3. The issue is not the prompt, the issue is how jitsi incorrectly handled the prompt
  4. When Cancel is clicked, jitsi has to prevent the page from refreshing + not interrupt the recording

https://dev.to/chromiumdev/sure-you-want-to-leavebrowser-beforeunload-event-4eg5 For the record, JS is not my regular language, but I have sufficient understanding that this prompt can be handled correctly by any JS

bilogic commented 1 week ago

There is no such thing handled in jitsi-meet. So I don't see what we can do about it.

Then it might be a case of adding a handler to deal with it

damencho commented 1 week ago

When I do the shortcut for reload the page reloads. There is no dialog. This is what I experience. That's why I ask for screenshot cause there is no such dialog implemented in jitsi-meet and I think this is something external that has nothing ti do with jitsi-meet.

bilogic commented 1 week ago

Did you start recording in step 1?

bilogic commented 1 week ago

https://github.com/user-attachments/assets/85f739eb-bb15-4d28-9fb4-ce40eb12bc97

damencho commented 1 week ago

Thanks for the video. You are right, I was skipping that part, sorry and I was not aware of the dialog. We do have it, but still it is the browser one and so the strings in it are not available in jitsi-meet. https://github.com/jitsi/jitsi-meet/blob/639114f2e19ad2e619ba81d8c6552b2385169c81/react/features/base/conference/middleware.any.ts#L302 I'm not sure you can detect whether the user has clicked cancel or not https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

bilogic commented 1 week ago

I'm not sure you can detect whether the user has clicked cancel or not https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

Yes you can, that is the sole purpose of that dialog, to prevent users from closing their window resulting in a catastrophic disaster, i.e. loss of data

bilogic commented 1 week ago

In fact, it would be great if jitsi always pops this dialog for any refresh as I see no use case for a refresh. I reported an issue here https://github.com/jitsi/jitsi-meet-electron/issues/986

aaronkvanmeerten commented 1 week ago

Refresh is actually an important part of the meeting experience. Anytime a client gets into an inconsistent state (connected to new backends, failure in existing connections) that cannot be resolved internally, a reload is triggered to rejoin the meeting.

damencho commented 1 week ago

Yes you can, that is the sole purpose of that dialog, to prevent users from closing their window resulting in a catastrophic disaster, i.e. loss of data

If you can detect when the user clicks Cancel, please share it with us or create a PR. I'm not familiar with that and a simple search shows that this is not possible with current browser API.

bilogic commented 1 week ago

ok will do, give me a while

bilogic commented 1 week ago

https://jsfiddle.net/2tc3krz4/4/ has different behaviors when clicking Reload vs Cancel

I think jitsi is lacking the preventDefault(), as a result, the refresh bubbles upwards causing the jitsi to refresh

damencho commented 1 week ago

The problem is that when the reload confirmation dialog appears if you click cancel the local recording is stopped and saved. How in the code in that jsfiddle do you determine whether the user has clicked cancel?

Have you been able to check the code I posted above? It is already calling preventDefault().

https://github.com/jitsi/jitsi-meet/blob/639114f2e19ad2e619ba81d8c6552b2385169c81/react/features/base/conference/middleware.any.ts#L306

bilogic commented 1 week ago

How in the code in that jsfiddle do you determine whether the user has clicked cancel?

https://stackoverflow.com/questions/63405982/how-to-know-when-a-page-redirect-or-refresh-action-is-cancelled-javascript

damencho commented 1 week ago

Seems there is no reliable way to do that for now.

bilogic commented 1 week ago

Can I suggest blocking F5 and ctrl+R for refreshing and allow refreshes only when a user clicks on the browser's refresh button or click on URL and press enter

In other words:

  1. Pressing F5/ctrl+R does not trigger the prompt (see https://vscode.dev/, both keys do not refresh the page)
  2. Clicking on refresh button or updating the URL will trigger the prompt
SakkyOP commented 5 days ago

After looking at the code snippet mentioned by @damencho I understand that regardless the user clicks reload / cancel the browser will stop the recording first as it is notified that a possible unload is about to occur thus triggering the before unload event which is a justified approach.

To my knowledge there was only one person in that meeting and reloading the page should technically close that meeting as the only recipient left the meeting thus saving the recording, have you tried this with multiple users in the meeting? We could probably add a condition where if the meeting isn't empty the recording shouldn't stop.

bilogic commented 5 days ago

@SakkyOP

  1. Recording is local, even if there are 2 users in the meeting, if I'm recording and I hit F5, the recording is disrupted
  2. The recording is stopped today because that is last possible opportunity to save, UI are for humans and ought to be designed as such: mistakes are reversible with minimal/0 consequences
  3. Instead of dealing with beforeunload limitations head on, a simple straight forward way can be to prevent the prompt from showing, e.g. disable F5/ctrl+R to deal with the OP while still leaving some options to refresh
SakkyOP commented 5 days ago

I think a better approach would be to shift the stop recording functionality to take place when the pagehide / visibilitychange event occurs

Also I noticed that the use of returnValue is wrong in beforeunload event handler, the prompt is shown when preventDefault() is called which is correct but in legacy versions the prompt appears when the returnValue property is set to a truthy value ( it is set to null in the current version which is falsy )

This will help:

My references:

bilogic commented 5 days ago

@SakkyOP

From what I understand of your approach:

  1. F5 will cause the Reload prompt to show, but recording continues
  2. If Reload, pagehide will run and stops the recording
  3. If Cancel, prompt closes and no refreshing takes place

Correct? If yes, then I agree yours is the better approach

SakkyOP commented 5 days ago

@bilogic

Yes that's exactly what I'm trying to say 👍

damencho commented 5 days ago

You cannot execute async code in those events, it is unreliable.

bilogic commented 5 days ago

@damencho then are we better off with blocking F5 and ctrl+R?

damencho commented 5 days ago

I don't think we will want to block that.

damencho commented 5 days ago

You can configure it on your deployment via some of the empty file hooks in index.html.