Skip to content

Fix dialog closedby and requestClose() (take 2) #11326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zcorpan merged 34 commits into from
Aug 27, 2025
Merged

Fix dialog closedby and requestClose() (take 2) #11326

Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
de8ec46
Add attribute changed steps to dialog element
lukewarlow Jan 27, 2025
9d690df
Replace assert in set the dialog close watcher with an early return
lukewarlow Feb 2, 2025
3ed047e
Add dialog attribute change steps when adding open attribute
lukewarlow Feb 12, 2025
d947ee7
Run specfmt
lukewarlow Feb 12, 2025
533bd82
Define dialog cleanup and setup steps and call them from the attribut…
lukewarlow Feb 25, 2025
634dfa9
Call dialog cleanup steps from dialog removing steps
lukewarlow Feb 25, 2025
6e790b2
Define dialog HTML element insertion steps and call dialog setup steps
lukewarlow Feb 25, 2025
a38e217
Replace early return with assert
lukewarlow Feb 25, 2025
f4fb3cf
Revert "Replace early return with assert"
lukewarlow Mar 7, 2025
9df1928
Add connected check to dialog insertion steps
lukewarlow Apr 15, 2025
f7d6325
Attempt at fixing issues
lukewarlow Apr 17, 2025
286a561
Add back destroy of close watcher to cleanup steps
lukewarlow Apr 17, 2025
78867ee
remove errenous div
keithamus May 21, 2025
a72c188
add many asserts
keithamus May 21, 2025
9bf69d7
return early if dialog is not connected during setup
keithamus May 21, 2025
7e7905f
return early in requestclose if dialog is not connected
keithamus May 21, 2025
d4081cf
specfmt
keithamus May 21, 2025
4a28393
s/active document/node document
keithamus May 21, 2025
cdea193
revert CloseWatcher requestClose changes, add to dialog requestclose
keithamus May 22, 2025
268ba08
add then
keithamus May 22, 2025
5f10244
collapse single item lists
keithamus May 22, 2025
383e523
clean up var/span/subject/this
keithamus May 23, 2025
d9d538d
remove commas
keithamus May 23, 2025
bb220c7
Merge remote-tracking branch 'upstream/main' into remove-dialog-open-…
keithamus May 23, 2025
f830c2b
add note for why element connected check isnt before dialog cleanup
keithamus May 23, 2025
d26c8e5
Merge remote-tracking branch 'upstream/main' into remove-dialog-open-…
keithamus Jun 17, 2025
2044edd
switch to assert the dialog close watcher is not null
keithamus Jun 17, 2025
066ff6e
remove open attribute guard
keithamus Jun 17, 2025
1700ef0
link concepts in note
keithamus Jun 17, 2025
fa98a1c
add active checks for attribute changed/insertion
keithamus Jun 18, 2025
3da983b
s/CloseWatcher/close watcher
keithamus Jun 18, 2025
9338867
s/code/span
keithamus Jun 18, 2025
513b7ac
remove erroneous "and subject"
keithamus Jun 18, 2025
f1f9c67
s/code/span
keithamus Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 95 additions & 47 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62304,14 +62304,8 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
<li><p>Add an <code data-x="attr-dialog-open">open</code> attribute to <span>this</span>, whose
value is the empty string.</p></li>

<li><p><span>Assert</span>: <span>this</span>'s <span>node document</span>'s <span>open
dialogs list</span> does not <span data-x="list contains">contain</span>
<span>this</span>.</p></li>

<li><p>Add <span>this</span> to <span>this</span>'s <span>node document</span>'s <span>open
dialogs list</span>.</p></li>

<li><p><span>Set the dialog close watcher</span> with <span>this</span>.</p></li>
<li><p><span>Assert</span>: <span>this</span>'s <span data-x="dialog-close-watcher">close
watcher</span> is not null.</p></li>

<li><p>Set <span>this</span>'s <span>previously focused element</span> to the
<span>focused</span> element.</p></li>
Expand Down Expand Up @@ -62451,31 +62445,58 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {

<hr>

<p>The <code>dialog</code> <span>HTML element removing steps</span>, given <var>removedNode</var>
and <var>oldParent</var>, are:</p>
<p>The <code>dialog</code> <span>HTML element insertion steps</span>, given
<var>insertedNode</var>, are:</p>

<ol>
<li>
<p>If <var>removedNode</var>'s <span data-x="dialog-close-watcher">close watcher</span> is not
null, then:</p>
<li><p>If <var>insertedNode</var>'s <span>node document</span> is not <span>fully active</span>,
then return.</p></li>
Comment thread
keithamus marked this conversation as resolved.
Outdated

<ol>
<li><p><span data-x="close-watcher-destroy">Destroy</span> <var>removedNode</var>'s <span
data-x="dialog-close-watcher">close watcher</span>.</p></li>
<li><p>If <var>insertedNode</var> has an <code data-x="attr-dialog-open">open</code> attribute
and is <span>connected</span>, then run the <span>dialog setup steps</span> given
<var>insertedNode</var>.</p></li>
</ol>

<li><p>Set <var>removedNode</var>'s <span data-x="dialog-close-watcher">close watcher</span> to
null.</p></li>
</ol>
</li>
<p>The <code>dialog</code> <span>HTML element removing steps</span>, given <var>removedNode</var>
and <var>oldParent</var>, are:</p>

<ol>
<li><p>If <var>removedNode</var> has an <code data-x="attr-dialog-open">open</code> attribute,
Copy link
Copy Markdown
Member

@domenic domenic Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@keithamus keithamus Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@domenic domenic Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Comment thread
keithamus marked this conversation as resolved.
Outdated
data-x="list contains">contains</span> <var>removedNode</var>, then <span>remove an element from
the top layer immediately</span> given <var>removedNode</var>.</p></li>

Comment thread
keithamus marked this conversation as resolved.
Outdated
<li><p>Set <span>is modal</span> of <var>removedNode</var> to false.</p></li>
</ol>

<p>The following <span data-x="concept-element-attributes-change-ext">attribute change
steps</span>, given <var>element</var>, <var>localName</var>, <var>oldValue</var>,
<var>value</var>, and <var>namespace</var> are used for <code>dialog</code> elements:</p>

Comment thread
keithamus marked this conversation as resolved.
Outdated
<ol>
<li><p>If <var>namespace</var> is not null, then return.</p></li>

Comment thread
keithamus marked this conversation as resolved.
Outdated
<li><p><span data-x="list remove">Remove</span> <var>removedNode</var> from
<var>removedNode</var>'s <span>node document</span>'s <span>open dialogs list</span>.</p></li>
<li><p>If <var>localName</var> is not <code data-x="attr-dialog-open">open</code>, then
return.</p></li>

<li><p>If <var>element</var>'s <span>node document</span> is not <span>fully active</span>, then
return.</p></li>

<li><p>If <var>value</var> is null and <var>oldValue</var> is not null, then run the
<span>dialog cleanup steps</span> given <var>element</var>.</p></li>
Comment thread
keithamus marked this conversation as resolved.
Outdated

Comment thread
keithamus marked this conversation as resolved.
Outdated
<li>
<p>If <var>element</var> is not <span>connected</span>, then return.</p>

<p class=note>This ensures that the dialog setup steps are not run on nodes that are
disconnected, which would result in a <span>close watcher</span> being established. The
Comment thread
keithamus marked this conversation as resolved.
Outdated
<span>dialog cleanup steps</span> need no such guard.</p>
</li>

<li><p>If <var>value</var> is not null and <var>oldValue</var> is null, then run the
<span>dialog setup steps</span> given <var>element</var>.</p></li>
</ol>

<p>To <dfn>show a modal dialog</dfn> given a <code>dialog</code> element <var>subject</var> and an
Expand Down Expand Up @@ -62521,14 +62542,10 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
<li><p>Add an <code data-x="attr-dialog-open">open</code> attribute to <var>subject</var>, whose
value is the empty string.</p></li>

<li><p>Set <span>is modal</span> of <var>subject</var> to true.</p></li>

<li><p><span>Assert</span>: <var>subject</var>'s <span>node document</span>'s <span>open
dialogs list</span> does not <span data-x="list contains">contain</span>
<var>subject</var>.</p></li>
<li><p><span>Assert</span>: <var>subject</var>'s <span data-x="dialog-close-watcher">close
watcher</span> is not null.</p></li>

<li><p>Add <var>subject</var> to <var>subject</var>'s <span>node document</span>'s <span>open
dialogs list</span>.</p></li>
<li><p>Set <span>is modal</span> of <var>subject</var> to true.</p></li>

<li>
<p>Set <var>subject</var>'s <span>node document</span> to be <span data-x="blocked by a modal
Expand All @@ -62546,8 +62563,6 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
already <span data-x="list contains">contain</span> <var>subject</var>, then <span>add an element
to the top layer</span> given <var>subject</var>.</p></li>

<li><p><span>Set the dialog close watcher</span> with <var>subject</var>.</p></li>

<li><p>Set <var>subject</var>'s <span>previously focused element</span> to the
<span>focused</span> element.</p></li>

Expand All @@ -62574,6 +62589,13 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
element <var>dialog</var>:</p>

<ol>
<li><p><span>Assert</span>: <span>dialog</span>'s
<span data-x="dialog-close-watcher">close watcher</span> is not null.</p></li>

<li><p><span>Assert</span>: <var>dialog</var> has an <code data-x="attr-dialog-open">open</code>
attribute and <var>dialog</var>'s <span>node document</span> is <span>fully active</span>.</p>
</li>

<li>
<p>Set <var>dialog</var>'s <span data-x="dialog-close-watcher">close watcher</span> to the
result of <span data-x="establish a close watcher">establishing a close watcher</span> given
Expand Down Expand Up @@ -62694,9 +62716,6 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {

<li><p>Set <span>is modal</span> of <var>subject</var> to false.</p></li>

<li><p><span data-x="list remove">Remove</span> <var>subject</var> from <var>subject</var>'s
<span>node document</span>'s <span>open dialogs list</span>.</p></li>

<li><p>If <var>result</var> is not null, then set <var>subject</var>'s <code
data-x="dom-dialog-returnValue">returnValue</code> attribute to <var>result</var>.</p></li>

Expand Down Expand Up @@ -62724,19 +62743,6 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
<li><p><span>Queue an element task</span> on the <span>user interaction task source</span> given the
<var>subject</var> element to <span data-x="concept-event-fire">fire an event</span> named
<code data-x="event-close">close</code> at <var>subject</var>.</p></li>

<li>
<p>If <var>subject</var>'s <span data-x="dialog-close-watcher">close watcher</span> is not null,
then:</p>

<ol>
<li><p><span data-x="close-watcher-destroy">Destroy</span> <var>subject</var>'s <span
data-x="dialog-close-watcher">close watcher</span>.</p></li>

<li><p>Set <var>subject</var>'s <span data-x="dialog-close-watcher">close watcher</span> to
null.</p></li>
</ol>
</li>
</ol>

<p>To <dfn data-x="dialog-request-close">request to close</dfn> <code>dialog</code> element
Expand All @@ -62747,6 +62753,9 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
<li><p>If <var>subject</var> does not have an <code data-x="attr-dialog-open">open</code>
attribute, then return.</p></li>

<li><p>If <var>subject</var> is not <span>connected</span> or <var>subject</var>'s
<span>node document</span> is not <span>fully active</span>, then return.</p></li>.

<li><p><span>Assert</span>: <var>subject</var>'s <span data-x="dialog-close-watcher">close
watcher</span> is not null.</p></li>

Expand Down Expand Up @@ -62872,6 +62881,45 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
<li><p>Set <var>topDocument</var>'s <span>autofocus processed flag</span> to true.</p></li>
</ol>
Comment thread
keithamus marked this conversation as resolved.
Outdated

<p>The <dfn>dialog setup steps</dfn>, given a <code>dialog</code> element <var>subject</var>, are
as follows:</p>

<ol>
<li><p><span>Assert</span>: <var>subject</var> has an <code data-x="attr-dialog-open">open</code>
attribute.</p></li>

<li><p><span>Assert</span>: <var>subject</var> is <span>connected</span>.</p></li>

<li><p><span>Assert</span>: <var>subject</var>'s <span>node document</span>'s <span>open dialogs
list</span> does not <span data-x="list contains">contain</span> <var>subject</var>.</p></li>

<li><p>Add <var>subject</var> to <var>subject</var>'s <span>node document</span>'s <span>open
dialogs list</span>.</p></li>

<li><p><span>Set the dialog close watcher</span> with <var>subject</var>.</p></li>
</ol>

<p>The <dfn>dialog cleanup steps</dfn>, given a <code>dialog</code> element <var>subject</var>,
are as follows:</p>

<ol>
Comment thread
keithamus marked this conversation as resolved.
Outdated
<li><p><span data-x="list remove">Remove</span> <var>subject</var> from <var>subject</var>'s
<span>node document</span>'s <span>open dialogs list</span>.</p></li>

<li>
<p>If <var>subject</var>'s <span data-x="dialog-close-watcher">close watcher</span> is not
null, then:</p>

<ol>
<li><p><span data-x="close-watcher-destroy">Destroy</span> <var>subject</var>'s <span
data-x="dialog-close-watcher">close watcher</span>.</p></li>

<li><p>Set <var>subject</var>'s <span data-x="dialog-close-watcher">close watcher</span> to
null.</p></li>
</ol>
</li>
</ol>

<h4><dfn>Dialog light dismiss</dfn></h4>

<p class="note">"Light dismiss" means that clicking outside of a <code>dialog</code> element whose <code
Expand Down