jmoenig / Snap

a visual programming language inspired by Scratch
http://snap.berkeley.edu
GNU Affero General Public License v3.0
1.5k stars 744 forks source link

dont show warning onbeforeunload if no unsaved changes #3361

Closed 2-www closed 3 months ago

jmoenig commented 4 months ago

thanks for the PR. Could you perhaps issue it against the dev branch, that would make it easier for me to review it.

So, it seems that currently it is again possible to do this. In the past this has been notoriously unreliable and not consistent across browsers (https://stackoverflow.com/questions/38879742/is-it-possible-to-display-a-custom-message-in-the-beforeunload-popup) even as recent as a few months ago (https://chriscoyier.net/2023/11/15/careful-about-chrome-119-and-beforeunload-event-listeners/), that's why I'm rather reluctant to add some complicated mechanism to widely used production code, since i'll have to support and maintain it indefinitely. But I'll happily look into it. Again, thanks!

2-www commented 4 months ago

thanks for the PR.

youre welcome!

In the past this has been notoriously unreliable and not consistent across browsers (https://stackoverflow.com/questions/38879742/is-it-possible-to-display-a-custom-message-in-the-beforeunload-popup)

yes, but this this about changing the browser message to something else, it seems now it just checks if the return value is not false/null/undefined and displays the standard message. here if there are changes, we return a non-empty string (whose contents are ignored by the browser nowadays, but the fact that we give it a string makes it do the popup), and if everything is saved, we do return; (= return undefined)

even as recent as a few months ago (https://chriscoyier.net/2023/11/15/careful-about-chrome-119-and-beforeunload-event-listeners/)

i don't know, i uploaded it at https://two.envs.net/snap/two-onbeforeunload/, and tried it in chrome 125, and it works there. perhaps that was a regression bug in 119 that is fixed by now? but anyway if this event (which Snap! already uses) is broken in some browsers, this patch will not change anything there

that's why I'm rather reluctant to add some complicated mechanism

it reuses the same check that Snap! already uses to decide whether to show its own "Unsaved Changes!" dialog when you try to open another project in a tab that already has a project open, so i think it won't complicate too much

2-www commented 4 months ago

did i change the branch correctly?

jmoenig commented 4 months ago

looks perfect now, thank you!