Skip to content
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

Jump to bottom

Close dialog elements when the open attribute is removed #10124

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Close dialog elements when the open attribute is removed #10124

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
128a87e
Close dialog elements when the open attribute is removed
josepharhar Feb 5, 2024
9cdcfdd
add ignoreOpenAttribute parameter
josepharhar Feb 6, 2024
ae5c95d
delete notes
josepharhar Feb 6, 2024
6f65d03
fix closing tag
josepharhar Feb 7, 2024
d84add3
split into two algorithms
josepharhar Feb 7, 2024
42df787
remove ignoreopenattribute
josepharhar Feb 9, 2024
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
64 changes: 34 additions & 30 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59316,7 +59316,8 @@ fur
<li><p>Otherwise, if <var>submitter</var> has a <span data-x="concept-fe-value">value</span>,
then set <var>result</var> to that <span data-x="concept-fe-value">value</span>.</p></li>

<li><p><span>Close the dialog</span> <var>subject</var> with <var>result</var>.</p></li>
<li><p><span>Close the dialog with attribute changes</span> given <var>subject</var> and
<var>result</var>.</p></li>

<li><p>Return.</p></li>
</ol>
Expand Down Expand Up @@ -61051,26 +61052,19 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {

</div>

<div class="note" id="note-dialog-remove-open-attribute">
<p>Removing the <code data-x="attr-dialog-open">open</code> attribute will usually hide the
dialog. However, doing so has a number of strange additional consequences:

<ul>
<li><p>The <code data-x="event-close">close</code> event will not be fired.</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>

<li><p>The <code data-x="dom-dialog-close">close()</code> method, and any <span data-x="close
request">close requests</span>, will no longer be able to close the dialog.</p></li>
<ol>
<li><p>If <var>namespace</var> is not null, then return.</p></li>

<li><p>If the dialog was shown using its <code data-x="dom-dialog-showModal">showModal()</code>
method, the <code>Document</code> will still be <span data-x="blocked by a modal
dialog">blocked</span>.</p></li>
</ul>
<li><p>If <var>localName</var> is not <code data-x="attr-dialog-open">open</code>, then
return.</p></li>

<p>For these reasons, it is generally better to never remove the <code
data-x="attr-dialog-open">open</code> attribute manually. Instead, use the <code
data-x="dom-dialog-close">close()</code> method to close the dialog, or the <code
data-x="attr-hidden">hidden</code> attribute to hide it.</p>
</div>
<li><p>If <var>value</var> is null, then <span>close the dialog</span> given
Copy link
Member

Choose a reason for hiding this comment

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

This won't work, because it will return in step 1, because the open attribute has already been removed.

I guess we need to factor out steps 3-9 into a separate algorithm. Maybe "process 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.

Whoops I forgot that, I added a parameter to close in chromium. I just added it here. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's slightly harder to understand than factoring out the steps would be, but I don't feel too strongly. (Thanks for updating the algorithm declaration style while you were there!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I factored out the steps into a separate algorithm

<var>element</var> and null.</p></li>
</ol>

<p>The <code data-x="attr-tabindex">tabindex</code> attribute must not be specified on
<code>dialog</code> elements.</p>
josepharhar marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -61257,19 +61251,33 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
<ol>
<li><p>If <var>returnValue</var> is not given, then set it to null.</p></li>

<li><p><span>Close the dialog</span> <span>this</span> with <var>returnValue</var>.</p></li>
<li><p><span>Close the dialog with attribute changes</span> given <span>this</span> and
<var>returnValue</var>.</p></li>
</ol>

<p>When a <code>dialog</code> element <var>subject</var> is to be <dfn data-x="close the
dialog">closed</dfn>, with null or a string <var>result</var>, run these steps:</p>
<p>To <dfn>close the dialog with attribute changes</dfn> given a <code>dialog</code> element
<var>subject</var> and a string or null <var>result</var>:</p>
josepharhar marked this conversation as resolved.
Show resolved Hide resolved
josepharhar marked this conversation as resolved.
Show resolved Hide resolved

<ol>
<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>ignoreOpenAttribute</var> is false:</p>
josepharhar marked this conversation as resolved.
Show resolved Hide resolved

<li><p>Remove <var>subject</var>'s <code data-x="attr-dialog-open">open</code>
attribute.</p></li>
<ol>
<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>Remove <var>subject</var>'s <code data-x="attr-dialog-open">open</code>
attribute.</p></li>
</ol>
</li>

<li><p>Run <span>close the dialog</span> given <var>subject</var> and <var>result</var>.</p></li>
</ol>

<p>To <dfn>close the dialog</dfn> given a <code>dialog</code> element <var>subject</var> and a
string or null <var>result</var>:</p>

<ol>
<li><p>If the <span>is modal</span> flag of <var>subject</var> is true, then <span>request an
element to be removed from the top layer</span> given <var>subject</var>.</p></li>

Copy link

Choose a reason for hiding this comment

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

(github doesn't seem to let me add comment in the right place outside the PR changes)
Don't "focusing steps" possibly end up firing an event at problematic time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the best thing would either be to not focus the previously focused element in this case, or to post a task/microtask to focus that element. Thoughts? @domenic @keithamus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbaron too if you have thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related for popovers: #10129

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to schedule a task. We should avoid losing focus on the document if we can help it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scheduling a task sounds good to me too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaug---- what do you think of scheduling a task?

Expand Down Expand Up @@ -61332,11 +61340,7 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
<li><p>Hiding a dialog is different from closing one. Closing a dialog gives it a return value,
fires an event, unblocks the page for other dialogs, and so on. Whereas hiding a dialog is a
purely visual property, and is something you can already do with the <code
data-x="attr-hidden">hidden</code> attribute or by removing the <code
data-x="attr-dialog-open">open</code> attribute. (See also the <a
href="#note-dialog-remove-open-attribute">note above</a> about removing the <code
data-x="attr-dialog-open">open</code> attribute, and how hiding the dialog in that way is
generally not desired.)</p></li>
data-x="attr-hidden">hidden</code> attribute.</p></li>

<li><p>Showing a dialog is different from opening one. Opening a dialog consists of creating
and showing that dialog (similar to how <code data-x="dom-open">window.open()</code> both
Expand Down
Loading