Fix dialog closedby and requestClose()

[lukewarlow]

Update dialog to minimally handle open attribute mutations

We define two new algorithms for dialog:

1. Dialog cleanup steps which:

   1. Removes dialog from the document's open dialogs list
   2. Destroy close watcher (if the dialog has no open attribute)

2. Dialog setup steps which:

   1. Creates the dialog's close watcher.
   2. Adds dialog to the document's open dialogs list.

We also define attribute change steps and HTML element insertion steps for the dialog element.

We call dialog cleanup steps when:

a) An open dialog is removed from the document
b) A dialog has its open attribute removed

We call dialog setup steps when:

a) An open dialog is inserted into the document
b) A dialog has an open attribute added to it

This fixes spec issues that can lead to assertions being hit but also incorrect behaviour. This also fixes closedby not working as expected for initially open dialogs.

Fixes #10953 and #10982

• At least two implementers are interested (and none opposed):
  • Chromium - Already implements a version of this.
  • …
• Tests are written and can be reviewed and commented upon at:

  • This behaviour is already enforced by multiple Web platform tests. e.g. https://wpt.live/html/semantics/interactive-elements/the-dialog-element/dialog-remove-open-attr-crash.html

  • Added better test coverage for requestClose and initially open dialogs: Add requestClose() subtests for initially open dialog web-platform-tests/wpt#50432

  • https://wpt.live/html/semantics/interactive-elements/the-dialog-element/dialog-closedby-start-open.html - Added by Chromium change

  • Added a new test that doesn't rely on Assertions: Add test for dialog attribute changed steps web-platform-tests/wpt#50393

• Implementation bugs are filed:
  • Chromium: N/A
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1936940 (meta bug)
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=284592 (meta bug)
• Corresponding HTML AAM & ARIA in HTML issues & PRs: n/A
• [ x] MDN issue is filed: N/A
• The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

---

/interactive-elements.html ( diff )

[domenic]

• This behaviour is already enforced by multiple Web platform tests. e.g. https://wpt.live/html/semantics/interactive-elements/the-dialog-element/dialog-remove-open-attr-crash.html

This test would also pass if we removed the assert, i.e. fixed the regression introduced recently.

Marking "do not merge yet" until we get more clarity in #10953 as to what the right path forward is.

[domenic]

Removing "do not merge yet" as I'm convinced by the arguments in #10953, but of course we still need to check all the boxes to merge.

For the tests box, could you point to more tests that enforce this behavior that aren't crash tests? Since assertions are actually comments; they don't cause crashes.

[lukewarlow]

I've opened web-platform-tests/wpt#50393 which fails in a browser which follows the current spec:

• I tested by locally modifiying chrome to use an ordered list for the open dialogs list, and removed the attribute changed set. In that build both the escape case and the click outside case result in the dialog being closed when it shouldn't be.

[lukewarlow]

Have rebased with Keith's changes hopefully it's where its needed as of the new model.

I think this just needs Implementer interest. I believe whatwg needs someone appointed by the projects to provide a position, so me for WebKit and Keith for Firefox wouldn't be enough. (Cc @smaug---- and @annevk )

[lukewarlow]

Per https://issues.chromium.org/issues/384549097 I've changed the new assertion within set the dialog close watcher to be an early return as it's possible that this scenario happens in legitimate cases (see the issue for more detail).

[mfreed7]

FYI, see comments at #10953 (comment) and after.

[lukewarlow]

Need to work out if we actually still need this. It's in Chromium atm but based on other changes may no longer be needed (instead can be replaced with an assert)

[lukewarlow]

This has been updated to account for changes per discussions at whatnot, above and in the Chromium CL.

TLDR we now handle adding the open attribute and removing it slightly more gracefully. This should fix various assertions being hit aswell as requestClose being broken.

I've left open dialogs list as a list in spec despite it being a set in Chromium, the various assertions should ensure this isn't an observable difference (happy to make that change if it's desired?).

[lukewarlow]

Pending approval from a second implementor this should be ready to go (aside from editorial remarks).

Though it would be good for @mfreed7 to check my homework.

[mfreed7]

Pending approval from a second implementor this should be ready to go (aside from editorial remarks).

Though it would be good for @mfreed7 to check my homework.

Thanks for updating this!

So as I'm working on the Chromium changes, I've got another CL going that (I think) better handles the additional case that a <dialog open> is removed and inserted into the document. The general idea would be to remove the dialog from the open dialogs list and kill its closewatcher when the dialog is removed from the doc, and vice versa. I'm still working on it, but I do think there might still be some cases that this PR doesn't cover. Perhaps let me get that CL into working shape and then I can come back here and reconcile it with this PR?

[lukewarlow]

So as I'm working on the Chromium changes, I've got another CL going that (I think) better handles the additional case that a is removed and inserted into the document. The general idea would be to remove the dialog from the open dialogs list and kill its closewatcher when the dialog is removed from the doc, and vice versa. I'm still working on it, but I do think there might still be some cases that this PR doesn't cover. Perhaps let me get that CL into working shape and then I can come back here and reconcile it with this PR?

Feel free to give me a shout if you want to explain what you think I'm missing and I can probably work out the missing piece myself.

[annevk]

@mfreed7 said that if this PR landed first #10657 would need changes, but that PR landed first. So this PR will have to account for that.

[lukewarlow]

I just looked into this and I think we don't actually need any changes here. This is because of a difference in spec layout vs chromium impl. In chrome the RemovedFrom and InsertedInto methods are invoked and the then there's early returns for state preserving atomic move operations. However, the spec went a different approach and defined separate moving steps.

Because we don't have moving steps defined for dialog, nothing happens to it which is expected.

@mfreed7 @domfarolino does this make sense to you or am I missing something?

[mfreed7]

I just looked into this and I think we don't actually need any changes here. This is because of a difference in spec layout vs chromium impl. In chrome the RemovedFrom and InsertedInto methods are invoked and the then there's early returns for state preserving atomic move operations. However, the spec went a different approach and defined separate moving steps.

Because we don't have moving steps defined for dialog, nothing happens to it which is expected.

That sounds right to me. I just wanted to highlight it since I did have to handle this case in code, and assumed it would therefore have spec impact. I'm not the moveBefore expert, so I'll defer to @domfarolino. But fingers crossed that nothing needs to change here.