Fix dialog closedby and requestClose() (take 2) by keithamus · Pull Request #11326 · whatwg/html

keithamus · 2025-05-21T10:03:30Z

This is a continuation of #10954 to update the dialog element to handle changes to the open attribute. It includes:

New HTML Element insertion steps which:
1. Run dialog setup if connected & open attribute is present
new HTML element removal steps which:
1. Run dialog cleanup if open attribute is present
New dialog setup steps which:
1. Creates the dialog's close watcher.
2. Adds dialog to the document's open dialogs list.
New 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)
Changes to requestClose which:
1. Returns early if the document is not fully active.

At least two implementers are interested (and none opposed):
- Chromium: Already implements this.
- Mozilla: Interested.
Tests are written and can be reviewed and commented upon at:
- https://wpt.fyi/results/html/semantics/interactive-elements/the-dialog-element
Implementation bugs are filed:
- Chromium: N/A
- Gecko: N/A
- WebKit: N/A
- Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): N/A
- Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): N/A
Corresponding HTML AAM & ARIA in HTML issues & PRs: N/A
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.)

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 This also adds an assertion to the start of 'set the dialog close watcher' that dialog's close watcher is null.

Address review comments

…e changed steps

This reverts commit e7096df.

Dialog setup steps: - Move set the dialog close watcher to step 1 - Add early return when dialog is not connected as a new step 2 (before adding to dialog light dismiss list). Dialog cleanup steps: - Remove assertion - Remove disposing of close watcher. Dialog attribute change steps: - Remove connected check from steps 3 and 4. Dialog insertion steps: - Remove connected check added in previous commit.

keithamus · 2025-05-21T10:06:39Z

@mfreed7 would you kindly check through this - I believe this closely aligns to the Chrome behaviour, but it would be great if you could confirm.

@annevk you also might want to take a look as we've discussed this in #11230 & #11278.

Additionally @domenic you might want to take a look, this involves close watchers.

source

lukewarlow · 2025-05-22T14:18:47Z

Does https://github.com/web-platform-tests/wpt/pull/50393/files provide anything new that's not already tested?

source

…clear-list

mfreed7

Thanks for putting this together! I believe this matches the Chromium implementation, modulo the few comments I added.

source

…clear-list

source

domenic · 2025-06-18T06:32:00Z

source

+   <li><p>If <var>removedNode</var> has an <code data-x="attr-dialog-open">open</code> attribute,
+   then run the <span>dialog cleanup steps</span> given <var>removedNode</var>.</p></li>

   <li><p>If <var>removedNode</var>'s <span>node document</span>'s <span>top layer</span> <span


Can you remind me why we only do these two steps for element removal, and not for open attribute removal?

Things get a little messy here to be honest. Chrome implements #10124 which effectively does the necessary steps. I don't know what would be simpler for us - to try to explain those steps in this PR, or to try to work on getting #10124 landed?

Note that although Chromium implements #10124, it's not shipped yet, just behind experimental web platform features. And #10124 does more than just cleanup steps + top layer removal + is modal -> false; it does the whole "close a dialog" algorithm.

So it sounds like we should merge this PR as-is, as it aligns with what Chromium currently ships and is an improvement over the current spec which is buggy in lots of ways. Then, we should work on making the behavior more sane, which would include at a minimum aligning removal + attribute changed, but could go all the way to making removal and/or attribute changed do a full close (per #10124).

source

domenic

This LGTM with one more typography fix. However, can you help reassure me that everything has web platform tests? For one example, I don't see fully active checks in the Chromium implementation. (They might be hidden in a base class or something.)

I'll also give people a few more days to review in case, and plan on merging next Monday Japan time if you reassure me on the test coverage.

domenic · 2025-06-19T04:49:31Z

source

+   <li><p>If <var>removedNode</var> has an <code data-x="attr-dialog-open">open</code> attribute,
+   then run the <span>dialog cleanup steps</span> given <var>removedNode</var>.</p></li>

   <li><p>If <var>removedNode</var>'s <span>node document</span>'s <span>top layer</span> <span


Note that although Chromium implements #10124, it's not shipped yet, just behind experimental web platform features. And #10124 does more than just cleanup steps + top layer removal + is modal -> false; it does the whole "close a dialog" algorithm.

So it sounds like we should merge this PR as-is, as it aligns with what Chromium currently ships and is an improvement over the current spec which is buggy in lots of ways. Then, we should work on making the behavior more sane, which would include at a minimum aligning removal + attribute changed, but could go all the way to making removal and/or attribute changed do a full close (per #10124).

source

keithamus · 2025-06-19T10:11:53Z

However, can you help reassure me that everything has web platform tests?

The tests have somewhat organically grown due to the lack of specification around this, and in addition a lot of this is "action at a distance", so they're not neatly organised but:

https://github.com/web-platform-tests/wpt/blob/master/html/semantics/interactive-elements/the-dialog-element/dialog-showModal-remove.html this tests that a dialog in the modal state can be removed.
https://github.com/web-platform-tests/wpt/blob/master/html/semantics/interactive-elements/the-dialog-element/dialog-remove-open-attr-crash.html this tests that a dialog in the modal state doesn't crash when the open attribute is removed (the current spec would hit this)
https://github.com/web-platform-tests/wpt/blob/master/html/semantics/interactive-elements/the-dialog-element/dialog-requestclose-2-crash.html, https://github.com/web-platform-tests/wpt/blob/master/html/semantics/interactive-elements/the-dialog-element/dialog-requestclose-3-crash.html,https://github.com/web-platform-tests/wpt/blob/master/html/semantics/interactive-elements/the-dialog-element/dialog-requestclose-4-crash.html, https://github.com/web-platform-tests/wpt/blob/master/html/semantics/interactive-elements/the-dialog-element/dialog-requestclose-crash.html all test the various combinations of attribute/insertions while calling requestClose which - due to its close watcher integration - hits various asserts around the state of the internal close watcher.

We could probably improve coverage, or at least organise it so it is more explicitly aligned to these spec changes, but overall I feel confident with the tests in place, but happy to take suggestions or guidance if you feel it's insufficient.

domenic · 2025-06-19T12:48:40Z

Well, I'm especially curious if there are tests for fa98a1c.

domenic · 2025-07-15T04:11:49Z

@zcorpan can you re-review to clear the "changes requested" that is blocking this from landing?

lukewarlow · 2025-08-27T11:24:58Z

Is there anything left needed to get this landed (have been out of the loop so not sure what the state of this PR is). It would be good to get the spec fixed up so further implementations can be done (e.g. webkit, servo, ladybird) in a way that we can be more confident.

domenic · 2025-08-28T00:42:43Z

I would prefer to have tests for the latest changes, in particular fa98a1c. @lukewarlow, is that something you can help with?

lukewarlow · 2025-08-28T08:57:31Z

Yeah as part of the WebKit implementation I can add additional tests. I'll be honest though I'm not immediately clear on what a "non fully-active" document actually is that like document.implementation stuff?

domenic · 2025-08-28T23:57:24Z

The three major sources of non-fully active are: synthetic documents (e.g. document.implementation), documents from disconnected iframes (i.e. const doc = iframe.contentDocument; iframe.remove();), and documents in bfcache (i.e. const doc = iframe.contentDocument; iframe.contentWindow.location.href = "..."). The latter is perhaps the most important to test.

lukewarlow and others added 17 commits May 21, 2025 09:25

Replace assert in set the dialog close watcher with an early return

9d690df

Add dialog attribute change steps when adding open attribute

3ed047e

Address review comments

Run specfmt

d947ee7

Define dialog cleanup and setup steps and call them from the attribut…

533bd82

…e changed steps

Call dialog cleanup steps from dialog removing steps

634dfa9

Define dialog HTML element insertion steps and call dialog setup steps

6e790b2

Replace early return with assert

a38e217

Revert "Replace early return with assert"

f4fb3cf

This reverts commit e7096df.

Add connected check to dialog insertion steps

9df1928

Add back destroy of close watcher to cleanup steps

286a561

remove errenous div

78867ee

add many asserts

a72c188

return early if dialog is not connected during setup

9bf69d7

return early in requestclose if dialog is not connected

7e7905f

specfmt

d4081cf

keithamus requested a review from zcorpan May 21, 2025 10:03

keithamus added topic: dialog The <dialog> element topic: close watchers labels May 21, 2025

keithamus changed the title ~~Remove dialog open clear list~~ Fix dialog closedby and requestClose() (take 2) May 21, 2025

zcorpan requested changes May 21, 2025

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

s/active document/node document

4a28393

domenic reviewed May 22, 2025

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

keithamus added 3 commits May 22, 2025 09:29

revert CloseWatcher requestClose changes, add to dialog requestclose

cdea193

add then

268ba08

collapse single item lists

5f10244

domenic requested changes May 23, 2025

View reviewed changes

source Outdated Show resolved Hide resolved

keithamus added 4 commits May 23, 2025 15:48

clean up var/span/subject/this

383e523

remove commas

d9d538d

Merge remote-tracking branch 'upstream/main' into remove-dialog-open-…

bb220c7

…clear-list

add note for why element connected check isnt before dialog cleanup

f830c2b

mfreed7 approved these changes May 23, 2025

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

keithamus mentioned this pull request Jun 6, 2025

Fix dialog closedby and requestClose() #10954

Closed

5 tasks

domfarolino reviewed Jun 6, 2025

View reviewed changes

source Outdated Show resolved Hide resolved

keithamus added 4 commits June 17, 2025 11:36

Merge remote-tracking branch 'upstream/main' into remove-dialog-open-…

d26c8e5

…clear-list

switch to assert the dialog close watcher is not null

2044edd

remove open attribute guard

066ff6e

link concepts in note

1700ef0

domenic reviewed Jun 18, 2025

View reviewed changes

keithamus added 4 commits June 18, 2025 09:47

add active checks for attribute changed/insertion

fa98a1c

s/CloseWatcher/close watcher

3da983b

s/code/span

9338867

remove erroneous "and subject"

513b7ac

domenic approved these changes Jun 19, 2025

View reviewed changes

s/code/span

f1f9c67

domenic requested a review from zcorpan June 24, 2025 04:27

zcorpan approved these changes Aug 27, 2025

View reviewed changes

zcorpan merged commit 4e71f6e into whatwg:main Aug 27, 2025
2 checks passed

keithamus deleted the remove-dialog-open-clear-list branch September 3, 2025 14:50

keithamus mentioned this pull request Nov 28, 2025

Dialog requestClose causes several issues with regard to interplay of CloseWatcher #11230

Closed

Conversation

keithamus commented May 21, 2025 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keithamus commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukewarlow commented May 22, 2025

Uh oh!

Uh oh!

mfreed7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

domenic Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

keithamus Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

domenic Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

domenic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keithamus commented Jun 19, 2025

Uh oh!

domenic commented Jun 19, 2025

Uh oh!

domenic commented Jul 15, 2025

Uh oh!

lukewarlow commented Aug 27, 2025

Uh oh!

Uh oh!

domenic commented Aug 28, 2025

Uh oh!

lukewarlow commented Aug 28, 2025

Uh oh!

domenic commented Aug 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

keithamus commented May 21, 2025 •

edited by pr-preview bot

Loading

keithamus commented May 21, 2025 •

edited

Loading

domenic left a comment •

edited

Loading