Unsaved changes handler is excessive

[oliversalzburg]

Nature of issue?

• Found a bug

Details about the bug:

• Web browser and version: Google Chrome 86.0.4240.75
• Operating System: Windows
• Steps to reproduce this bug:

Open a sketch, press Play, then close the tab.

Even though I made no changes, the annoying beforeunload handler prevents me from closing the tab.

Feature enhancement details:

Some ideas:

a) Remove the handler entirely, it's more frustrating than it helps.
b) Automatically save drafts to prevent data loss. LocalStorage is a great place for that.
c) Make installation of beforeunload handler configurable.
d) Properly detect actual changes and don't prevent user's from closing a tab just because they played the sketch.

[welcome]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

[catarak]

Thanks for reporting! I have noticed that this alert has been coming up when it shouldn't, and I'm not sure why. It's only supposed to appear if you have unsaved changes in the sketch you're working on. Thanks for the suggestions on how to fix this!

[oliversalzburg]

I also briefly looked into it so I could submit a PR. Sadly, I don't understand enough about React :( Of course, removing the feature entirely would be easy, but I understand that's not a desirable solution.

I feel like draft saving locally would be nice, but I'm sure it introduces new challenges and is easier said than done.

Option d) is probably the preferable solution, but it's not clear to me how playing the sketch effects the properties of the component.

However, here's my take on the current implementation:
It seems like https://github.com/processing/p5.js-web-editor/blob/develop/client/modules/IDE/pages/IDEView.jsx#L69 results in a return value of true regardless of there being no changes. Returning anything other than null or undefined from an onbeforeunload handler will cause the dialog to be shown.

From what I can tell, the handler correctly determines that there are no unsaved changes, but the return value still triggers the browser's default behavior.

When I make changes to the sketch, the path in warnIfUnsavedChanges is correctly taken and the window.confirm is triggered. When I make no changes, the other path is taken correctly.

[catarak]

Thanks for digging into the code and figuring out why this is is happening! onbeforeunload is definitely not always supposed to return true, so it seems like that logic somehow got messed up.

[oliversalzburg]

I'll submit a PR in a moment. I'm not sure how to properly test locally, but maybe we can figure out any potential issues in the PR.