MBS-13320: Prompt before unloading modified forms

[derat]

MBS-13320

Install a beforeunload event listener on the artist, event, label, place, recording, release group, series, and work edit pages that prompts the user if any of the form's inputs have been changed.

One limitation of the approach used here is that it doesn't detect relationship changes.

Note that the release editor already displays a prompt if the page is unloaded with unsubmitted changes.

Problem

If an editing page is accidentally closed or navigated away from by a user before it's been submitted, data entered in JavaScript-dependent fields like Area, Relationships, and External Links are lost.

Solution

Listen for change events within form elements and install a beforeunload handler that prompts the user if the page is being unloaded after changes have been made. I added a small MB.installFormUnloadWarning function that seems to be sufficient for all of the forms that I'm updating here.

Note that since I'm watching for change events (rather than input), the prompt won't be primed until the user actually commits a change by e.g. de-focusing an input after typing into it. The approach used here doesn't seem to detect relationship changes; I think that the separate implementation of the relationship editor may make this challenging.

I think that the release editor is able to use a different approach where it maintains a list of unsubmitted changes, but I believe that that approach is unavailable in (all?) other pages.

Testing

I verified that all of the pages that I've updated don't prompt if I close them without making any changes, but do prompt if I've made name, area, URL, etc. changes. They also don't prompt when the submit button is clicked.

As mentioned above, the prompt isn't shown if only relationships are changed.

[derat]

Does the relationship editor e.g. emit custom events for changes that I could also listen for here?

[mwiencek]

No custom events, but...perhaps if you check #relationship-editor for the presence of hidden inputs, that would indicate whether changes were made.

[derat]

Hmm. That's a clever idea, but I'm not seeing any hidden relationship inputs at the time of the beforeunload event. This is my first time glancing at the relationship editor code, but is it possible that root/static/scripts/relationship-editor/utility/prepareHtmlFormSubmission.js only adds the hidden inputs immediately before the form is posted? If so, I don't think I'll be able to use that approach here.

[mwiencek]

Ah, that's right, it only adds them once the submit event is triggered. Sorry.

[derat]

@reosarevok Okay, I think I finally got all the Selenium tests passing. Mind taking a quick look and letting me know if this seems like a reasonable approach?

[derat]

@mwiencek and/or @yvanzo, mind taking a look? Sorry, not sure who I should be asking for reviews. :-)

[mwiencek]

Okay, I think I finally got all the Selenium tests passing. Mind taking a quick look and letting me know if this seems like a reasonable approach?

Seems like a reasonable approach for now! When these forms are converted to React later, we'll probably have a more direct way of calculating changes from the internal state, but things can always be improved later, and this is a good improvement on its own.

[derat]

Thanks for the quick review! I think that this is ready to go in on my side (assuming there isn't some other easy way to check for pending relationship edits).

[reosarevok]

Quickly tested locally and it seems to work fine - it does not trigger if I close the page with ctrl+w while still inside the first field I have changed, but that's not a big issue since there's very little to lose in that case. As soon as I tab out of the first field the alert activates when trying to close the tab.

I guess we should squash all the test fixes for merging? :)

[derat]

Quickly tested locally and it seems to work fine - it does not trigger if I close the page with ctrl+w while still inside the first field I have changed, but that's not a big issue since there's very little to lose in that case. As soon as I tab out of the first field the alert activates when trying to close the tab.

Would it be better to additionally watch for input events? I tend to prefer change since it's less spammy when the user is typing, but watching for input would catch modifications before they've been committed by a focus change.

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/input_event warns that checkboxes and radio buttons haven't always triggered input events (presumably on old versions of IE), so maybe it's safest to continue listening for change in either case.

And just to mention it, I noticed that neither change nor input appears to fire if the Area field on the artist form is empty and I select a new value from the suggestions using only the mouse. Lots of weird edge cases that'll be easier to handle once this is using React. :-)

I guess we should squash all the test fixes for merging? :)

Yeah, that was my hope! Is there a preferred approach to use here for MBS, by the way? I'd usually amend my change locally and force-push it to the branch, but I think that confuses GitHub when there are ongoing conversations on the older version of the change, so I sometimes avoid doing it.

[derat]

@mwiencek or @reosarevok, mind merging this? Or please let me know if you'd prefer that I also add a listener for input events.

[mwiencek]

Fine with just handling change events for now!

[jesus2099]

I'm only on phone for a few days but I think unsubmitted relationships (add, edit, remove) might have some specific CSS class, that we could test.

If we see such classes with document.querySelector(".pending-rel-add, .pending-rel-edit, .pending-rel-remove") or something like document.querySelector("[class^='pending-rel']") then we would trigger the stop code.

Just an idea.

[jesus2099]

I checked with Kiwi Browser devtools, the pending relationship classes are:

• table.rel-editor-table span.rel-add
• table.rel-editor-table span.rel-edit
• table.rel-editor-table span.rel-remove

[jesus2099]

@derat

• input change events: handled
• relationship changes: see above
• URL relationship changes: is it handled?

[derat]

@jesus2099 Thanks for the suggestion! I've created #3064 to detect relationship and URL changes.