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

emilio · 2020-08-10T19:35:20Z

https://html.spec.whatwg.org/#the-dialog-element:

Removing the open attribute will usually hide the dialog. However, doing so has a number of strange additional consequences:

The close event will not be fired.

The close() method, and any user-agent provided cancelation interface, will no longer be able to close the dialog.

If the dialog was shown using its showModal() method, the Document will still be blocked.
For these reasons, it is generally better to never remove the open attribute manually. Instead, use the close() method to close the dialog, or the hidden attribute to hide it.

This seems very odd. The "leaves the document blocked" part in particular. That seems that it's super-easy to leave the user without being able to interact with the website by doing dialog.showModal(); then dialog.removeAttribute("close");. I guess the same is true of a mispositioned dialog though? However removing the open attribute seems like a legit mistake someone could make and it just leaves the browser in an inconsistent state.

Should we implicitly close the dialog when removing open? I think that's what details does effectively. And if so should we implicitly call show() when adding the attribute?

cc @annevk

The text was updated successfully, but these errors were encountered:

domenic · 2020-08-10T19:37:14Z

I'm a bit confused how this contrasts with ~~your~~@annevk's stance on details in #4500.

emilio · 2020-08-10T22:00:28Z

Yeah, to be clear I don't have a super-strong opinion here.

It's just weird that removing an attribute can leave the page in a fully broken state. I was reviewing a <dialog> patch in Gecko, and we couldn't add some sanity-checks to the code because of this (because given the current spec it's totally legit to get to showModal() when you're already modal, for example, and so on.

domenic · 2020-08-10T22:08:53Z

I tend to agree the spec is weird. Speaking with my HTML editor hat on (not my Chrome hat; @mfreed7 is the point of contact for that), I think it'd be best if open="" were completely synced with show()/close(). (Although I haven't looked at how doable that is.)

I think the main obstacle is whether we consider it acceptable to fire events during parsing, which is why I referenced #4500. If we don't want to fire such events, then complete syncing won't be possible. And it's less clear to me whether partial syncing (e.g., un-block the document without firing the event) would be a good idea.

annevk · 2020-08-17T14:49:33Z

How is the parser impacted here? (I don't think it removes attributes.)

domenic · 2020-08-17T15:17:43Z

It's impacted if we want to maintain a synchronization between show/close and open="", e.g. per @emilio's

And if so should we implicitly call show() when adding the attribute?

This ties into my conjecture partially synchronizing (e.g., synchronizing only removal but not addition) could be worse than the current state where there's no synchronization at all.

domenic · 2021-03-17T18:07:47Z

Per #6371 (comment), @annevk, myself, @mfreed7, and @melanierichards were tasked with figuring something out here.

Here is my proposal. The guiding principles are:

The dialog can have three states: shown, shown-modal, and closed.
If the dialog has the open attribute, it must be in the shown or shown-modal states.
If the dialog does not have the open attribute, it must be in the closed state.
Being in the top layer means the dialog must be in the shown-modal state.
Being in the shown or shown-modal states does not mean you are shown to the user or in the top layer. In particular disconnected dialogs have an open attribute (and thus are in the shown or shown-modal state) but are not shown to the user or in the top layer.
- In this framework, @sefeng211's Unset is_modal flag when modal dialog is removed from the document #6216 proposes that disconnected dialogs are always shown instead of shown-modal.
Being in the shown or shown-modal states does not mean that the dialog focusing steps have necessarily run. So, if you do dialog.toggleAttribute("open", true), this is different than dialog.show() because only the latter runs the dialog focusing steps.

Then:

show() and showModal() behave as they do today.
close() behaves as it does today.
Dialog gains attribute change steps, which are:
- If the attribute is open and it's being removed, then run the following subset of the close algorithm:
  - Set the is modal flag to false.
  - Remove the dialog from the top layer.
  - Queue an element task on the user interaction task source given the dialog to fire an event named close at the dialog.

melanierichards · 2021-03-31T21:48:20Z

The proposal SGTM, looping in @BoCupp-Microsoft in case there's any implementation considerations I haven't thought of.

mfreed7 · 2021-03-31T22:43:58Z

I also think the proposal looks good/rational. Thanks @domenic for putting this together so concisely.

Dialog gains attribute change steps, which are:

If the attribute is open and it's being removed, then run the following subset of the close algorithm:

I agree with this set of changes - this is parallel to what the spec already says about removing a <dialog> from the Document. I would hope that there aren't too many compat issues from this change, given this warning in the spec.

Being in the shown or shown-modal states does not mean that the dialog focusing steps have necessarily run. So, if you do dialog.toggleAttribute("open", true), this is different than dialog.show() because only the latter runs the dialog focusing steps.

Can you help me understand why we want/need this difference? Any reason not to make dialog.toggleAttribute("open",true) just run the same algorithms as dialog.show()/dialog.close()?

I also want to point out that I would love to hear more use cases for the open attribute. I opened an issue on <popup> because it gets even more complex there due to light-dismiss behavior. I know that this ship has likely sailed for <dialog>, but I'm wondering if we need to maintain this attribute for <popup>. In the meantime, pushing authors toward the .show()/.close() APIs, and away from mucking with the open attribute, seems like a good thing to do.

domenic · 2021-03-31T22:53:42Z

this is parallel to what the spec already says about removing a from the Document

Note that removing currently doesn't fire a close event. Maybe it should...

Can you help me understand why we want/need this difference? Any reason not to make dialog.toggleAttribute("open",true) just run the same algorithms as dialog.show()/dialog.close()?

Interesting question... I'm trying to remember why I proposed that a couple weeks ago. Maybe it was just the general feeling that "side effects" should be minimized.

One potential problem is the interaction with showModal(). showModal() currently does (roughly):

Add open attribute
Set up modal rendering (top layer, CSS-centering, inerting and thus unfocusing other stuff)
Run dialog focusing steps

If we made adding the open attribute always run the focusing steps, we would necessarily switch the order of (2) and (3); i.e. by coupling (1) to (3) we can no longer interleave (2).

Maybe that's fine? It would change observable behavior in interesting ways, e.g. when the blur/focus events run, the dialog would not be CSS-centered or on the top layer. I dunno.

(We could also try to change the ordering to (2) (1) (3); that has similar maybe-fine-but-maybe-weird consequences.)

mfreed7 · 2021-04-23T22:28:20Z

Checking back in on this issue. One development is that the <popup> issue (which was moved here) was discussed live in OpenUI last week. We came to a resolution that does not include an open attribute at all. Instead, we added a (subject-to-bikeshedding) initiallyopen attribute that only controls visibility upon page load. Most people (save one) seemed very happy with this proposal. The dissenter's point was that initiallyopen is something new (along with several other "new" things for <popup>) and we should strive for uniformity on the platform.

It doesn't appear that we have use counters for usage of the open attribute on <dialog>. But given that this is still a Chromium-only element, I'm wondering what the compat story would be for removing the open attribute completely, and perhaps replacing it with something closer to initiallyopen? Plus a CSS pseudo class (:open?) that can be used for styling? That may be too bold of a suggestion at this point.

Dialog's use counter is very high, at 6%, but I question the number of times those are actually shown. All of the "show" type counters that we do have seem very low. Perhaps I'll try to add a counter for access to the <dialog open> attribute.

annevk · 2021-04-26T12:06:54Z

There's also the details element, right? Anyway, this seems okay to me, but would be nice to resolve soonish as we're getting close to being able to ship this in Firefox.

josepharhar · 2024-02-05T21:40:51Z

Yeah this is weird, and I agree that we should add attribute change steps to at least remove the dialog from the top layer when the open attribute is removed. I will open a spec PR and implement in chromium

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.

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

#10124
https://chromium-review.googlesource.com/c/chromium/src/+/5269905

This patch adds HTMLDialogElement::ParseAttribute which runs HTMLDialogElement::close when the open attribute is removed to prevent a bad state where the dialog is modal but hidden and inerting the rest of the document. Spec discussion is happening here: whatwg/html#5802 Change-Id: Ib90736ced952d7aeadc791c6863c3ac2a55deb62

This patch adds HTMLDialogElement::ParseAttribute which runs HTMLDialogElement::close when the open attribute is removed to prevent a bad state where the dialog is modal but hidden and inerting the rest of the document. Spec discussion is happening here: whatwg/html#5802 Change-Id: Ib90736ced952d7aeadc791c6863c3ac2a55deb62 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5269905 Reviewed-by: David Baron <dbaron@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1258629}

…ttribute is removed, a=testonly Automatic update from web-platform-tests Close the dialog element when the open attribute is removed This patch adds HTMLDialogElement::ParseAttribute which runs HTMLDialogElement::close when the open attribute is removed to prevent a bad state where the dialog is modal but hidden and inerting the rest of the document. Spec discussion is happening here: whatwg/html#5802 Change-Id: Ib90736ced952d7aeadc791c6863c3ac2a55deb62 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5269905 Reviewed-by: David Baron <dbaron@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1258629} -- wpt-commits: c0538d5500ef425b051f7dcb37772af5025324d0 wpt-pr: 44408

…ttribute is removed, a=testonly Automatic update from web-platform-tests Close the dialog element when the open attribute is removed This patch adds HTMLDialogElement::ParseAttribute which runs HTMLDialogElement::close when the open attribute is removed to prevent a bad state where the dialog is modal but hidden and inerting the rest of the document. Spec discussion is happening here: whatwg/html#5802 Change-Id: Ib90736ced952d7aeadc791c6863c3ac2a55deb62 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5269905 Reviewed-by: David Baron <dbaronchromium.org> Commit-Queue: Joey Arhar <jarharchromium.org> Cr-Commit-Position: refs/heads/main{#1258629} -- wpt-commits: c0538d5500ef425b051f7dcb37772af5025324d0 wpt-pr: 44408 UltraBlame original commit: 1ad3db0d81f20d2319adf0d009fabe2d4b1b8ca0

This patch adds HTMLDialogElement::ParseAttribute which runs HTMLDialogElement::close when the open attribute is removed to prevent a bad state where the dialog is modal but hidden and inerting the rest of the document. Spec discussion is happening here: whatwg/html#5802 Change-Id: Ib90736ced952d7aeadc791c6863c3ac2a55deb62 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5269905 Reviewed-by: David Baron <dbaron@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1258629}

…ttribute is removed, a=testonly Automatic update from web-platform-tests Close the dialog element when the open attribute is removed This patch adds HTMLDialogElement::ParseAttribute which runs HTMLDialogElement::close when the open attribute is removed to prevent a bad state where the dialog is modal but hidden and inerting the rest of the document. Spec discussion is happening here: whatwg/html#5802 Change-Id: Ib90736ced952d7aeadc791c6863c3ac2a55deb62 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5269905 Reviewed-by: David Baron <dbaronchromium.org> Commit-Queue: Joey Arhar <jarharchromium.org> Cr-Commit-Position: refs/heads/main{#1258629} -- wpt-commits: c0538d5500ef425b051f7dcb37772af5025324d0 wpt-pr: 44408 UltraBlame original commit: 1ad3db0d81f20d2319adf0d009fabe2d4b1b8ca0

This patch adds HTMLDialogElement::ParseAttribute which runs HTMLDialogElement::close when the open attribute is removed to prevent a bad state where the dialog is modal but hidden and inerting the rest of the document. Spec discussion is happening here: whatwg/html#5802 Change-Id: Ib90736ced952d7aeadc791c6863c3ac2a55deb62 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5269905 Reviewed-by: David Baron <dbaron@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1258629}

annevk added the topic: dialog The <dialog> element. label Aug 17, 2020

css-meeting-bot mentioned this issue Oct 23, 2020

[dialog element] <dialog> positioning should be describable in CSS w3c/csswg-drafts#4645

Closed

domenic mentioned this issue Feb 1, 2021

[Popup] Feedback from the perspective of HTML Standard editor MicrosoftEdge/MSEdgeExplainers#455

Closed

domenic mentioned this issue Feb 23, 2021

[Popup] Add declarative invocation and move event bubbling to an appendix MicrosoftEdge/MSEdgeExplainers#459

Merged

melanierichards mentioned this issue Feb 25, 2021

HTMLPopupElement - <popup> w3ctag/design-reviews#599

Closed

1 task

past mentioned this issue Mar 11, 2021

Upcoming HTML standard issue triage meeting on 3/4/2021 #6371

Closed

past mentioned this issue Apr 1, 2021

Upcoming HTML standard issue triage meeting on 4/1/2021 #6481

Closed

domenic mentioned this issue Apr 12, 2021

HTML: IDL attributes and HTML attributes should be kept in sync w3ctag/design-principles#279

Closed

davatron5000 mentioned this issue Apr 20, 2021

[Popup] Do we need the "open" content attribute? openui/open-ui#311

Closed

OmidHekayati mentioned this issue Jun 2, 2021

[Suggestion] dialog element #6730

Closed

OmidHekayati mentioned this issue Aug 11, 2021

[Suggestion] Global state attribute #6953

Closed

mfreed7 mentioned this issue Apr 11, 2022

[selectors] Add :modal-dialog pseudo-class w3c/csswg-drafts#6965

Closed

domenic mentioned this issue Nov 11, 2022

New feature proposal: Popover API #7785

Closed

domenic mentioned this issue Dec 16, 2022

Should open dialog elements match :open? #8625

Open

domenic mentioned this issue May 27, 2023

"check popover algorithm" check for open dialogs is weak #9335

Closed

josepharhar linked a pull request Feb 5, 2024 that will close this issue

Close dialog elements when the open attribute is removed #10124

Open

5 tasks

chromium-wpt-export-bot mentioned this issue Feb 5, 2024

Close the dialog element when the open attribute is removed web-platform-tests/wpt#44408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

emilio commented Aug 10, 2020

domenic commented Aug 10, 2020 •

edited

emilio commented Aug 10, 2020

domenic commented Aug 10, 2020

annevk commented Aug 17, 2020

domenic commented Aug 17, 2020

domenic commented Mar 17, 2021

melanierichards commented Mar 31, 2021

mfreed7 commented Mar 31, 2021

domenic commented Mar 31, 2021

mfreed7 commented Apr 23, 2021

annevk commented Apr 26, 2021

josepharhar commented Feb 5, 2024

josepharhar commented Feb 5, 2024

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

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

Comments

emilio commented Aug 10, 2020

domenic commented Aug 10, 2020 • edited

emilio commented Aug 10, 2020

domenic commented Aug 10, 2020

annevk commented Aug 17, 2020

domenic commented Aug 17, 2020

domenic commented Mar 17, 2021

melanierichards commented Mar 31, 2021

mfreed7 commented Mar 31, 2021

domenic commented Mar 31, 2021

mfreed7 commented Apr 23, 2021

annevk commented Apr 26, 2021

josepharhar commented Feb 5, 2024

josepharhar commented Feb 5, 2024

domenic commented Aug 10, 2020 •

edited