Dialog should be better-behaved on misuse, probably

[emilio]

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

[domenic]

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

[emilio]

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]

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]

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

[domenic]

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]

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]

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

[mfreed7]

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]

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

1. Add open attribute
2. Set up modal rendering (top layer, CSS-centering, inerting and thus unfocusing other stuff)
3. 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]

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]

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]

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

[josepharhar]

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

[lukewarlow]

Happy to raise a new issue if this seems like a separate conversation to this, but adding it here for now.

Currently removing a dialog from the document only removes it from top layer, and destroys its close watcher, and as of #10459 (and in pratice in implementations) sets is modal to false.

This leaves you with a dialog that's still "open", and when re-inserted into the document is open.

This means that modal dialogs get converted to not-modal dialogs, which is strange.

It also means that you end up in a potentially strange situation regarding focus. The dialog close steps run steps to put focus back into the document in an expected place.

Should removing the dialog from the document run close? Which would do everything it does today but also do the focusing fix up and fire the close event?

[mfreed7]

This leaves you with a dialog that's still "open", and when re-inserted into the document is open.

Yes, the open attribute is funny in this respect. This is one of the reasons we didn't want to add an open attribute to popovers, and instead used ::popover-open as a pseudo-class.

Should removing the dialog from the document run close? Which would do everything it does today but also do the focusing fix up and fire the close event?

That seems reasonable to me, as long as everything in those steps can run asynchronously. The close event should already be async, but I'm not sure the focus fixup can be done async?