Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancelling Reload site prompt still refreshes the page #15198

Open
2 of 11 tasks
bilogic opened this issue Oct 15, 2024 · 33 comments
Open
2 of 11 tasks

Cancelling Reload site prompt still refreshes the page #15198

bilogic opened this issue Oct 15, 2024 · 33 comments

Comments

@bilogic
Copy link

bilogic commented Oct 15, 2024

What happened?

vid.mp4
  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
24ae693

Platform

  • Chrome (or Chromium based)
  • Firefox
  • Safari
  • Other desktop browser
  • Android browser
  • iOS browser
  • Electron app
  • Android mobile app
  • iOS mobile app
  • Custom app using a mobile SDK

Browser / app / sdk version

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

Relevant log output

No response

Reproducibility

  • The problem is reproducible on meet.jit.si

More details?

No response

@damencho
Copy link
Member

  • 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
Copy link
Author

bilogic commented Oct 15, 2024

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
Copy link
Member

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
Copy link
Author

bilogic commented Oct 15, 2024

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
Copy link
Author

bilogic commented Oct 15, 2024

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
Copy link
Member

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
Copy link
Author

bilogic commented Oct 15, 2024

Did you start recording in step 1?

@bilogic
Copy link
Author

bilogic commented Oct 15, 2024

  • repro of the issue
vid.mp4

@damencho
Copy link
Member

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.

beforeUnloadHandler = (e?: any) => {

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
Copy link
Author

bilogic commented Oct 15, 2024

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
Copy link
Author

bilogic commented Oct 15, 2024

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 jitsi/jitsi-meet-electron#986

@aaronkvanmeerten
Copy link
Member

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
Copy link
Member

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
Copy link
Author

bilogic commented Oct 15, 2024

ok will do, give me a while

@bilogic
Copy link
Author

bilogic commented Oct 16, 2024

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
Copy link
Member

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().

@bilogic
Copy link
Author

bilogic commented Oct 17, 2024

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
Copy link
Member

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

@bilogic
Copy link
Author

bilogic commented Oct 17, 2024

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
Copy link

SakkyOP commented Oct 18, 2024

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
Copy link
Author

bilogic commented Oct 18, 2024

@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
Copy link

SakkyOP commented Oct 18, 2024

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:

  • avoid removing functionality of reload shortcuts such as F5 and ctrl + R
  • only stop the recording when the page is actually reloaded by the user ( hence minimal consequence )
  • make the reload prompt more reliable and actually work in various legacy versions as well

My references:

  • page life cycle This was to get a better understanding where and when to use visibilitychange and pagehide
  • unload event I was going to suggest the unload event but it is deprecated alternatives are mention in the Usage Notes section.
  • beforeunload event This mentions when the dialog box appears.

@bilogic
Copy link
Author

bilogic commented Oct 18, 2024

@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
Copy link

SakkyOP commented Oct 18, 2024

@bilogic

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

@damencho
Copy link
Member

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

@bilogic
Copy link
Author

bilogic commented Oct 18, 2024

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

@damencho
Copy link
Member

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

@damencho
Copy link
Member

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

@bilogic
Copy link
Author

bilogic commented Oct 24, 2024

@damencho any examples of file hooks in index.html?

@damencho
Copy link
Member

https://github.com/jitsi/jitsi-meet/blob/master/head.html
https://github.com/jitsi/jitsi-meet/blob/master/plugin.head.html

The content of these files are included in index.html

<!--#include virtual="head.html" -->

You can use an nginx rule to override the file location, so you can put the files in /etc/ folder or somewhere else which is different than /use/share/jitsi-meet so upgrades will not overwrite your changes.
Similar to this

@bilogic
Copy link
Author

bilogic commented Oct 24, 2024

Thanks, I'm using https://github.com/jitsi/docker-jitsi-meet, trying to map the files you mentioned, but the default does not expose any filesystem to the host

@damencho
Copy link
Member

Yeah we need to add custom meet.conf where you can add the aliases to the files in /config and put the files in config. Any PRs are welcome.

@bilogic
Copy link
Author

bilogic commented Oct 24, 2024

I will probably modify docker-jitsi-meet, I found and added a minimal script to head.html that was able to block F5 and ctrl+r

<script>
document.addEventListener('keydown', (e) => {
    e = e || window.event;
    if(e.keyCode == 116  || (e.ctrlKey && e.keyCode == 82) ){
        e.preventDefault();
    }
});
</script>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants