Add attribute changed steps to dialog element

[lukewarlow]

Add attribute changed steps to dialog element

When the dialogs open attribute is removed:

1. Remove dialog from the document's open dialogs list.
2. Destroy and nullify dialog's close watcher

When the dialogs open attribute is added:

1. Add dialog to the document's open dialogs list.
2. Create the close watchers dialog.

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 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.