Skip to content

Add unsaved changes warning to message and diary forms#7076

Open
phanq003 wants to merge 4 commits into
openstreetmap:masterfrom
phanq003:feature/text-editor-prompt-before-unload-
Open

Add unsaved changes warning to message and diary forms#7076
phanq003 wants to merge 4 commits into
openstreetmap:masterfrom
phanq003:feature/text-editor-prompt-before-unload-

Conversation

@phanq003

@phanq003 phanq003 commented May 9, 2026

Copy link
Copy Markdown

Description

Addresses #4898, addresses data loss from accidental navigation aspect of #7044

Users writing diary entries or messages can accidentally navigate away from the page and lose their work. This adds the built-in browser warning (using the beforeunload event) when a user tries to leave a page with edits in progress.

The JavaScript should be able to be used on other forms across the site by adding the data-unsaved-changes-warning attribute to relevant form elements.

Reference: developer.chrome.com/docs/web-platform/page-lifecycle-api#the_beforeunload_event

How has this been tested?

Manually verified that warning appears when trying to navigate away from page with content in diary entry and messages creation, and editing for the former.

Four test cases were added to a new test file for unsaved changes:

  • Warn before leaving unsaved diary entry form
  • Does not warn when publishing diary entry
  • Warns before leaving unsaved message form
  • Message form does not warn before any changes are made

@phanq003 phanq003 force-pushed the feature/text-editor-prompt-before-unload- branch 2 times, most recently from e2ad6d4 to 29b45e1 Compare May 9, 2026 12:47
Comment thread app/assets/javascripts/unsaved_changes.js Outdated
@phanq003 phanq003 force-pushed the feature/text-editor-prompt-before-unload- branch from 1797859 to 248b8e9 Compare May 10, 2026 08:38
@phanq003

phanq003 commented May 11, 2026

Copy link
Copy Markdown
Author

With tests, originally tried to test the beforeunload popup directly using dismiss_confirm, but according to https://makandracards.com/makandra/622849-allow-testing-beforeunload-confirmation-dialogs-modern ChromeDriver 127+ automatically closes the dialogs. This is causing Capybara::ModalNotFound: Unable to find modal dialog.

Attempted to work around this adapting the solution from the mentioned reference, but then would get Selenium::WebDriver::Error::SessionNotCreatedError: session not created: Chrome instance exited.

As a workaround I've used a DOM attribute to keep track of when the listener is attached to be able to test that its there.

@phanq003 phanq003 force-pushed the feature/text-editor-prompt-before-unload- branch from 55c8bb2 to 6365dcf Compare May 11, 2026 14:00
phanq003 added 3 commits May 11, 2026 23:34
Added JavaScript logic that watches any form with data-unsaved-changes-warning attribute. If the user edits the form and tries to close/navigate away, displays browser's default popup. Also added file to application.js.

To get around issue with modalnotfound in tests when trying to interact with `beforeunload` with `dismiss_confirm`, using a attribute to keep track of when the event listener is added/removed instead.
Added attribute to forms for creating new messages and diary entries, as well as editing the latter.
Tests use unsaved-changes attribute to track if warning will appear.
@phanq003 phanq003 force-pushed the feature/text-editor-prompt-before-unload- branch from 6365dcf to 1d6de08 Compare May 11, 2026 14:04
@phanq003 phanq003 marked this pull request as ready for review May 11, 2026 14:09
Comment thread app/assets/javascripts/unsaved_changes.js Outdated
removed code for legacy support for beforeunloadlistener, and removed comments describing functions as per PR review
@phanq003 phanq003 force-pushed the feature/text-editor-prompt-before-unload- branch from 1244bf1 to 08069e9 Compare May 12, 2026 14:44

@gravitystorm gravitystorm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a clear discussion about alternative approaches. I'm concerned that this is a lot of hard-to-test code and hard to maintain code.

  • Does rails (or hotwire, stimulus etc) offer anything similar?
  • Is there a ruby gem or standard javascript library that offers anything similar?
  • If this is available in other text editors (e.g. Trix) or similar situations (e.g. iD) can we re-use code from there?
  • Is there something

It might be that what is presented here is the best approach, but I would like to see it reasoned out.


Also, I'd like to see some reasoning as to which forms should get this warning, and which ones should not. That helps decide whether this PR covers enough of them, or if we can make it enabled by default (since someone adding a new form on the website might not be aware this exists).

@phanq003

Copy link
Copy Markdown
Author

I'd like to see a clear discussion about alternative approaches. I'm concerned that this is a lot of hard-to-test code and hard to maintain code.

* Does rails (or hotwire, stimulus etc) offer anything similar?

* Is there a ruby gem or standard javascript library that offers anything similar?

* If this is available in other text editors (e.g. Trix) or similar situations (e.g. iD) can we re-use code from there?

* Is there something

It might be that what is presented here is the best approach, but I would like to see it reasoned out.

Also, I'd like to see some reasoning as to which forms should get this warning, and which ones should not. That helps decide whether this PR covers enough of them, or if we can make it enabled by default (since someone adding a new form on the website might not be aware this exists).

I haven't had time to research alternative libraries or existing implementations for this, so I can't really say whether there's a library in Rails/hotwire available.

My thinking behind this was that because the underlying logic (beforeunload and event.preventDefault()) is the browser-supported way for implementing warnings like this. At least from a user perspective I've seen similar popups on other websites so it would align with expected browser behaviour.

For the scope of forms, I would think it would mainly be attached to forms where users are creating or editing content over multiple fields, where navigating away would mean meaningful loss of work (e.g. diary entries, profile edits). This would then exclude more temporary forms like searches or login flows. If this was enabled by default, it could be a bit annoying to users when navigating away from the latter, and they'll could end up desensitised to it, which would probably reduce its effectiveness. Of course then there's still the problem of developers being aware it exists though.

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

Successfully merging this pull request may close these issues.

4 participants