Close dialog elements when the open attribute is removed #10124

josepharhar · 2024-02-05T22:13:10Z

This patch adds attribute change steps to run the dialog closing steps whenever the open attribute is removed in order to prevent a bad state where the dialog is hidden but modal which renders the rest of the page inert.

At least two implementers are interested (and none opposed):
- Chrome
- …
Tests are written and can be reviewed and commented upon at:
- Close the dialog element when the open attribute is removed web-platform-tests/wpt#44408
Implementation bugs are filed:
- Chromium: https://issues.chromium.org/issues/341935362
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1879306
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=285242
MDN issue is filed: MDN already discourages modifying the open attribute, and I think its a good idea to keep it that way.
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

/form-control-infrastructure.html ( diff )
/interactive-elements.html ( diff )

Fixes whatwg#5802 This patch adds attribute change steps to run the dialog closing steps whenever the open attribute is removed in order to prevent a bad state where the dialog is hidden but modal which renders the rest of the page inert.

domenic

Very excited to see you tackle this! I hope other implementers are interested and that it's web-compatible. Pinging @emilio @zcorpan @annevk @nt1m for their thoughts.

domenic · 2024-02-06T04:56:49Z

source

+   return.</p></li>
+
+   <li><p>If <var>value</var> is null, then <span>close the dialog</span> given
+   <var>element</var>.</p></li>


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

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

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

Ok, I factored out the steps into a separate algorithm

source

josepharhar · 2024-02-06T21:19:58Z

I hope other implementers are interested and that it's web-compatible.

I will ship this behind a flag, and I will let yall know if any issues come up. I don't have a usecounter for it but I don't see why this change would be a problem.

domenic

Generally LGTM. I'll agenda+ this in the hopes of getting quick feedback.

domenic · 2024-02-07T02:37:35Z

source

+   return.</p></li>
+
+   <li><p>If <var>value</var> is null, then <span>close the dialog</span> given
+   <var>element</var>.</p></li>


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

source

annevk · 2024-02-08T08:47:33Z

Seems reasonable modulo web compatibility. @lukewarlow might also be interested in this.

source

keithamus

As a web developer (currently using <dialog> quite a bit), I'd be interested in seeing this change. (I've raised https://phabricator.services.mozilla.com/D201076 for Firefox).

source

smaug---- · 2024-02-08T22:00:28Z

source

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



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

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

@dbaron too if you have thoughts

Related for popovers: #10129

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

Scheduling a task sounds good to me too

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

I made it schedule a task to focus for this case

https: //github.com/whatwg/html/pull/10124#discussion_r1483640232 Change-Id: Iab20699b345d96a8cffc83f7e7e44f4a03feb833

lukewarlow · 2024-02-23T23:25:22Z

This feels like a big win for developers and having looked at the changes I don't think it's likely to cause any issues. Especially because it's unlikely developers will be relying on this behaviour (seems like its always undesirable)

mfreed7 · 2024-11-19T16:05:38Z

Any updates on this PR? It's mildly related to #10737 inasmuch as the close watcher logic is a little weirder (implementation-wise) when the open attribute can just be yanked off without calling close(). @josepharhar did you happen to try shipping the new behavior to see about web compat? (I'm happy to try, if there's consensus that this PR's behavior is desirable.)

josepharhar · 2024-12-10T19:09:49Z

@josepharhar did you happen to try shipping the new behavior to see about web compat? (I'm happy to try, if there's consensus that this PR's behavior is desirable.)

No, I haven't shipped this yet.

I'm still waiting for feedback from @smaug---- in this thread: #10124 (comment)
It sounds like posting a task to focus is the right thing to do so far, so perhaps I'll just do that.

When the dialog's open attribute is removed which closes the dialog, we should make the focus async in order to prevent more script from running inside attribute removal. Context: whatwg/html#10124 (comment) Bug: 3419353 Change-Id: I1b76f003e04b802b1868b427a0faddf5f19a3c5e

When the dialog's open attribute is removed which closes the dialog, we should make the focus async in order to prevent more script from running inside attribute removal. Context: whatwg/html#10124 (comment) Bug: 341935362 Change-Id: I1b76f003e04b802b1868b427a0faddf5f19a3c5e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6085725 Reviewed-by: Di Zhang <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1402551}

When the dialog's open attribute is removed which closes the dialog, we should make the focus async in order to prevent more script from running inside attribute removal. Context: whatwg/html#10124 (comment) Bug: 3419353 Change-Id: I1b76f003e04b802b1868b427a0faddf5f19a3c5e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6085725 Reviewed-by: Di Zhang <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1402551}

…en attribute, a=testonly Automatic update from web-platform-tests Make dialog focus async when removing open attribute When the dialog's open attribute is removed which closes the dialog, we should make the focus async in order to prevent more script from running inside attribute removal. Context: whatwg/html#10124 (comment) Bug: 341935362 Change-Id: I1b76f003e04b802b1868b427a0faddf5f19a3c5e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6085725 Reviewed-by: Di Zhang <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1402551} -- wpt-commits: 58fd4f845204b88a3ae42b72b613a66e84cbb9f8 wpt-pr: 49929

When the dialog's open attribute is removed which closes the dialog, we should make the focus async in order to prevent more script from running inside attribute removal. Context: whatwg/html#10124 (comment) Bug: 3419353 Change-Id: I1b76f003e04b802b1868b427a0faddf5f19a3c5e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6085725 Reviewed-by: Di Zhang <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1402551}

…en attribute, a=testonly Automatic update from web-platform-tests Make dialog focus async when removing open attribute When the dialog's open attribute is removed which closes the dialog, we should make the focus async in order to prevent more script from running inside attribute removal. Context: whatwg/html#10124 (comment) Bug: 341935362 Change-Id: I1b76f003e04b802b1868b427a0faddf5f19a3c5e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6085725 Reviewed-by: Di Zhang <dizhanggchromium.org> Commit-Queue: Joey Arhar <jarharchromium.org> Cr-Commit-Position: refs/heads/main{#1402551} -- wpt-commits: 58fd4f845204b88a3ae42b72b613a66e84cbb9f8 wpt-pr: 49929 UltraBlame original commit: 17e5e580ddfe60cd7d062ca9cbe0722342cc5638

lukewarlow · 2025-01-27T19:25:16Z

Are there any updates on this change? It's possible that this is actually required by the spec (and definitely seems to be required by some tests) due to the recent closedby attribute additions.

Specifically the assertion on step 13 that the open dialogs list doesn't already contain this isn't always true without this change.

Edit: See #10953 and #10954 for a minimal attempt to fix this issue.

josepharhar · 2025-02-06T23:46:45Z

From WHATNOT:

we should look at popover to decide whether to fire the synchronous beforetoggle event or not in this case (we probably shouldn't)
there might be some other focus fixup thing that changes the focused element when the open attribute is removed. it might be better to just not focus the previously focused element at all. would also be good to look at popover to see what it does

josepharhar · 2025-02-12T22:13:07Z

Based on the last comment, I removed the focusing of the previous element completely in the case that the open attribute is removed.

As for toggle/beforetoggle events, I looked at removing the popover attribute from an open popover and I actually found that all browsers fire a beforetoggle event, and chrome fires both a beforetoggle and a toggle: https://codepen.io/jarhar/pen/VYwYjKb

Assuming that firing events during attribute removal is a bad thing, I still think that we should not fire beforetoggle events. I haven't made any changes to toggle events in this PR yet.

lukewarlow · 2025-02-12T22:29:06Z

Slightly concerning that chrome behaves different to Firefox for those events on popover? I wonder which is correct per spec?

mfreed7 · 2025-02-14T20:39:51Z

Based on the last comment, I removed the focusing of the previous element completely in the case that the open attribute is removed.

Awesome, thanks!

For others, see some discussion starting at this comment. It sounds like the discussion is trending in this direction, but isn't quite concluded yet. Please chime in there if you have opinions.

Assuming that firing events during attribute removal is a bad thing, I still think that we should not fire beforetoggle events. I haven't made any changes to toggle events in this PR yet.

Can you clarify why firing events while changing attributes is a bad thing? This is materially different from removing elements from the tree, and I think it might be ok? E.g. we do this when changing the tabindex attribute today, for focus events. Or for the details element, removing the open attribute.

I added more cases to your test case, see here: https://jsbin.com/jayocud/edit?html,output

Results by engine:

	Blink	WebKit	Gecko
popover beforetoggle (sync)	fired	fired	fired
popover toggle (async)	fired	not fired	not fired
details beforetoggle (sync)	not fired	not fired	not fired
details toggle (async)	fired	fired	fired
tabindex removal blur event (sync)	fired	fired	fired

(I guess nobody has shipped details beforetoggle?) Anyway, since we do fire the async toggle event for <details> I don't see why we wouldn't fire it for popover also. And given the other sync and async events we're currently firing for the removal of other attributes, I don't see why it's an issue here. I think we should fire both toggle and beforetoggle for the removal of the popover attribute. And both events for the removal of the <dialog> open attribute, in the same way.

keithamus · 2025-02-14T21:20:51Z

(I guess nobody has shipped details beforetoggle?)

beforetoggle on details hasn't been specced yet. Refs #9743.

mfreed7 · 2025-02-14T21:28:16Z

(I guess nobody has shipped details beforetoggle?)

beforetoggle on details hasn't been specced yet. Refs #9743.

Right. Thanks for the reminder and pointer.

josepharhar mentioned this pull request Feb 5, 2024

Dialog should be better-behaved on misuse, probably #5802

Open

domenic added normative change topic: dialog The <dialog> element labels Feb 6, 2024

domenic requested changes Feb 6, 2024

View reviewed changes

domenic self-assigned this Feb 6, 2024

josepharhar added 2 commits February 6, 2024 13:14

add ignoreOpenAttribute parameter

9cdcfdd

delete notes

ae5c95d

domenic reviewed Feb 7, 2024

View reviewed changes

domenic added the agenda+ To be discussed at a triage meeting label Feb 7, 2024

josepharhar added 2 commits February 7, 2024 11:28

fix closing tag

6f65d03

split into two algorithms

d84add3

smaug---- reviewed Feb 8, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

keithamus reviewed Feb 8, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

past removed the agenda+ To be discussed at a triage meeting label Feb 8, 2024

past mentioned this pull request Feb 8, 2024

Upcoming WHATNOT meeting on 2/8/2024 #10094

Closed

smaug---- reviewed Feb 8, 2024

View reviewed changes

remove ignoreopenattribute

42df787

josepharhar mentioned this pull request Feb 9, 2024

attribute change steps for popover may fire events synchronously #10129

Open

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 9, 2024

Run focus async when removing open attribute

425499b

https: //github.com/whatwg/html/pull/10124#discussion_r1483640232 Change-Id: Iab20699b345d96a8cffc83f7e7e44f4a03feb833

chromium-wpt-export-bot mentioned this pull request Feb 9, 2024

Run focus async when removing open attribute web-platform-tests/wpt#44510

Closed

josepharhar added 2 commits December 10, 2024 11:10

Merge branch 'main' into dialogattrclose

5a93291

Make focus async when removing open attribute

8fa9712

domenic mentioned this pull request Dec 11, 2024

Add dialog light dismiss behavior #10737

Merged

5 tasks

chromium-wpt-export-bot mentioned this pull request Jan 6, 2025

Make dialog focus async when removing open attribute web-platform-tests/wpt#49929

Merged

keithamus mentioned this pull request Jan 8, 2025

Dispatching Toggle Events for Dialog open/close w3ctag/design-reviews#1005

Closed

1 task

domenic mentioned this pull request Jan 29, 2025

Missing attribute changed steps for dialog can cause assertions to be hit #10953

Open

remove async focus

f26d007

mfreed7 mentioned this pull request Feb 14, 2025

Add attribute changed steps to dialog element #10954

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close dialog elements when the open attribute is removed #10124

Close dialog elements when the open attribute is removed #10124

josepharhar commented Feb 5, 2024 •

edited by pr-preview bot

Loading

domenic left a comment

domenic Feb 6, 2024

josepharhar Feb 6, 2024

domenic Feb 7, 2024

josepharhar Feb 7, 2024

josepharhar commented Feb 6, 2024

domenic left a comment

domenic Feb 7, 2024

annevk commented Feb 8, 2024

keithamus left a comment •

edited

Loading

smaug---- Feb 8, 2024

josepharhar Feb 9, 2024

josepharhar Feb 9, 2024

josepharhar Feb 9, 2024

keithamus Feb 9, 2024

josepharhar Feb 9, 2024

josepharhar Mar 27, 2024

josepharhar Dec 10, 2024

lukewarlow commented Feb 23, 2024

mfreed7 commented Nov 19, 2024 •

edited

Loading

josepharhar commented Dec 10, 2024

lukewarlow commented Jan 27, 2025 •

edited

Loading

josepharhar commented Feb 6, 2025

josepharhar commented Feb 12, 2025

lukewarlow commented Feb 12, 2025

mfreed7 commented Feb 14, 2025

keithamus commented Feb 14, 2025

mfreed7 commented Feb 14, 2025

Close dialog elements when the open attribute is removed #10124

Are you sure you want to change the base?

Close dialog elements when the open attribute is removed #10124

Conversation

josepharhar commented Feb 5, 2024 • edited by pr-preview bot Loading

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josepharhar commented Feb 6, 2024

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Feb 8, 2024

keithamus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukewarlow commented Feb 23, 2024

mfreed7 commented Nov 19, 2024 • edited Loading

josepharhar commented Dec 10, 2024

lukewarlow commented Jan 27, 2025 • edited Loading

josepharhar commented Feb 6, 2025

josepharhar commented Feb 12, 2025

lukewarlow commented Feb 12, 2025

mfreed7 commented Feb 14, 2025

keithamus commented Feb 14, 2025

mfreed7 commented Feb 14, 2025

josepharhar commented Feb 5, 2024 •

edited by pr-preview bot

Loading

keithamus left a comment •

edited

Loading

mfreed7 commented Nov 19, 2024 •

edited

Loading

lukewarlow commented Jan 27, 2025 •

edited

Loading