MBS-13320 (II): Warn on form unload with rel changes

[derat]

MBS-13320 (II)

Make the beforeunload listener installed by MB.installFormUnloadWarning additionally use CSS selectors to check for pending relationship or URL changes.

Previously, only changes to input elements were detected.

Problem

#3054 only warns when navigating away from edit forms if a change event has been observed. This detects changes to text fields or checkboxes, but it doesn't catch relationship or URL changes. As a result, editors could still lose work if they accidentally closed or navigated away from forms when making relationship or URL edits.

Solution

@jesus2099 pointed out that the relationship editor uses rel-add, rel-edit, and rel-remove CSS classes when relationships are being changed. This change queries for those classes in the beforeunload handler to additionally detect relationship and URL changes.

Just to mention it, I think that changes to the artist form's "area" input still aren't detected, but I don't see an obvious way to detect those without adding special code for that one specific case.

Testing

I verified that I now get prompted when navigating away from the artist and release group forms after adding/editing/removing relationships and adding/editing/removing URLs.

I suspect that I'll need to update this PR to update fix Selenium tests that end with pending relationship/URL changes, but I don't think I have any easy way of figuring out which ones need to be updated until the checks run (since I'm using musicbrainz-docker, I can't run Selenium tests locally).

Action

Nothing special needed here, I don't think.

[derat]

(I've started the process of updating Selenium tests that leave pending changes lying around. Hopefully there'll be fewer of them this time.)

[derat]

@mwiencek and @reosarevok, this is ready for review now.

[jesus2099]

About this loop:

// Check if there are pending relationship or URL changes.
    if (!inputsChanged) {
      let relsChanged = false;
      for (const sel of [
        '#relationship-editor .rel-add',
        '#relationship-editor .rel-edit',
        '#relationship-editor .rel-remove',
        '#external-links-editor .rel-add',
        '#external-links-editor .rel-edit',
        '#external-links-editor .rel-remove',
      ]) {
        if (!!document.querySelector(sel)) {
          relsChanged = true;
          break;
        }
      }
      if (!relsChanged) {
        return false;
      }
    }

I think I would have simply used a single querySelector but maybe it's for layout purpose? Eslint or something?

Something like:

// Check if there are pending relationship or URL changes.
    if (!inputsChanged) {
      if (!document.querySelector("form[class^='edit'] .rel-add, form[class^='edit'] .rel-edit, form[class^='edit'] .rel-remove")) {
        return false;
      }
    }

Or, if the form variable is available there:

// Check if there are pending relationship or URL changes.
    if (!inputsChanged) {
      if (!form.querySelector(".rel-add, .rel-edit, .rel-remove")) {
        return false;
      }
    }

I had difficulty understanding the code, otherwise... But I'm not a reference. 🤭

[derat]

I think I would have simply used a single querySelector but maybe it's for layout purpose? Eslint or something?

I was concerned that more-generic selectors could unintentionally match other elements (especially if someone makes CSS changes in the future). Happy to switch it if the maintainers think it's safe to do so, though. :-)

[jesus2099]

Yes or instead of a loop, we could join them:

form.querySelector([
  '#relationship-editor .rel-add',
  '#relationship-editor .rel-edit',
  '#relationship-editor .rel-remove',
  '#external-links-editor .rel-add',
  '#external-links-editor .rel-edit',
  '#external-links-editor .rel-remove',
].join(", "))

It's just me maybe, I found a loop was kind of complex.

[derat]

Thanks, that feels cleaner to me too! I've pushed an updated version.

[reosarevok]

Tested locally, seems to work nicely :)

[mwiencek]

LGTMBDNT