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

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
+95 −47
Open

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

Changes from 21 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
131 changes: 84 additions & 47 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62283,14 +62283,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 @@ -62335,6 +62329,9 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
<li><p>If <span>this</span> does not have an <code data-x="attr-dialog-open">open</code>
attribute, then return.</p></li>

<li><p>If <var>this</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>: <span>this</span>'s <span data-x="dialog-close-watcher">close
watcher</span> is not null.</p></li>

Expand Down Expand Up @@ -62434,31 +62431,46 @@ 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> 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>

<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>
<p>The <code>dialog</code> <span>HTML element removing steps</span>, given <var>removedNode</var>
and <var>oldParent</var>, are:</p>

<li><p>Set <var>removedNode</var>'s <span data-x="dialog-close-watcher">close watcher</span> to
null.</p></li>
</ol>
</li>
<ol>
<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
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>

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

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

<ol>
<li><p>If <var>namespace</var> is not null, then return.</p></li>

<li><p>If <var>localName</var> is not <code data-x="attr-dialog-open">open</code>, then
Copy link
Member

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
Contributor Author

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
Member

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

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>

<li><p>If <var>element</var> is not <span>connected</span>, then return.</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>:</p>
Expand Down Expand Up @@ -62502,14 +62514,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>Let <var>subject</var>'s <span>node document</span> be <span data-x="blocked by a modal
Expand All @@ -62527,8 +62535,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 @@ -62555,6 +62561,13 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
element <var>dialog</var>:</p>

<ol>
<li><p>If <span>dialog</span>'s <span data-x="dialog-close-watcher">close watcher</span> is not
null, then return.</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 @@ -62672,9 +62685,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 the <code
data-x="dom-dialog-returnValue">returnValue</code> attribute to <var>result</var>.</p></li>

Expand All @@ -62700,19 +62710,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>

</div>
Expand Down Expand Up @@ -62818,6 +62815,46 @@ 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>

<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>
<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,
and <var>subject</var> does not have an <code data-x="attr-dialog-open">open</code> attribute,
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