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.

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

Add light dismiss functionality to <dialog> #9373

Open
mfreed7 opened this issue Jun 1, 2023 · 47 comments · May be fixed by #10157
Open

Add light dismiss functionality to <dialog> #9373

mfreed7 opened this issue Jun 1, 2023 · 47 comments · May be fixed by #10157
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: close watchers topic: dialog The <dialog> element.

Comments

@mfreed7
Copy link
Collaborator

mfreed7 commented Jun 1, 2023

One of the nice features of the Popover API is its light dismiss behavior. In several of the demos of Popover that I've seen, developers are doing something like this:

<button popovertarget=foo>Click me</button>
<dialog popover id=foo>I'm a dialog!</dialog>
<style>
dialog[popover]::backdrop {
  background-color: black;
}
</style>

Using <dialog> with a popover attribute is perfectly fine semantically here, since the content represents a dialog. However, this pattern is being used almost entirely because of the features provided by the Popover API which are missing from the <dialog> element itself. Note the usage of ::backdrop to obscure the backdrop entirely. That indicates that this really is meant to be a modal dialog, because the intent is to focus attention only on the dialog and keep the user from "seeing" the rest of the page. However, popovers aren't modal and as such they don't inert the rest of the page. So in the above example, keyboard users are free to tab-navigate to other content they can't see. Mouse users are free to click "through" the opaque background onto unseen elements. Generally, it'd be better if this was a plain old modal <dialog> and not a popover.

To get around this usage pattern, let's bring the missing functionality to <dialog>. #3567 discusses one of those behaviors, namely declarative invocation of <dialog>. In this issue, I'd like to propose a mechanism to add light dismiss to <dialog>s.

Proposal (subject to bikeshedding):

<dialog lightdismiss> I'm a light dismiss dialog </dialog>

With the lightdismiss attribute present, clicking outside the dialog, or hitting ESC (or other close signals) will have the same affect as calling dialog.close().

Note one nuance, which is different from popover: since there's no concept of "nested" dialogs, if more than one dialog is open at a time, only the topmost (most recently opened) dialog will be closed on each light dismiss action. So if three dialogs are open and the user clicks outside all three of them, only the topmost dialog will close. Generally, nested dialogs is an anti-pattern, but even so, this feels the most natural to me anyway.

@tabatkins
Copy link
Collaborator

So if three dialogs are open and the user clicks outside all three of them, only the topmost dialog will close.

I think this is what I'd expect to happen anyway, so that's good.

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: dialog The <dialog> element. labels Jun 2, 2023
@domenic
Copy link
Member

domenic commented Jun 3, 2023

Note one nuance, which is different from popover: since there's no concept of "nested" dialogs

Can you expand on this? Intuitively, there's definitely a concept of nesting, e.g. a dialog opened because of an action taken inside another open dialog. I guess popover's notion of nesting is different than this, and that's how you're using the term "nesting"?

So if three dialogs are open and the user clicks outside all three of them, only the topmost dialog will close.

I agree this result does seem reasonable.

@zcorpan
Copy link
Member

zcorpan commented Jun 5, 2023

Supporting light dismiss for dialog seems nice. I wonder if we're able to make it work by default, or does that break content that implement their own light dismiss?

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jun 5, 2023

Note one nuance, which is different from popover: since there's no concept of "nested" dialogs

Can you expand on this? Intuitively, there's definitely a concept of nesting, e.g. a dialog opened because of an action taken inside another open dialog. I guess popover's notion of nesting is different than this, and that's how you're using the term "nesting"?

Sure. Dialogs can certainly be "stacked" like this, but different from Popover, there's no concept of "nested" popovers. The difference is mainly that Popover has complicated rules about how they're related. In the case of dialog, one dialog can open another dialog, but we don't have logic to connect them to each other. But I think that's ok: dialogs usually aren't used in a "nested" fashion, and when they are, the second dialog should be considered a standalone modal dialog. So one dialog should be dismissed at a time. Example: dialog 1 is "Do you want to save this file?", and on a "no" click, a second dialog shows "Are you sure??" which might cover dialog 1. Clicking outside should just light dismiss the topmost one, and leave the original modal visible.

Supporting light dismiss for dialog seems nice. I wonder if we're able to make it work by default, or does that break content that implement their own light dismiss?

I'm guessing that's very non-web-compatible. Since the existing dialog requires an explicit close, I bet there are dialogs that don't want to be light dismissed. A light dismiss dialog might not even be the majority use case for modal dialogs: I could see several types of modal dialog that really want explicit action. Like "Choose A or B" - cancel is not an option.

@scottaohara
Copy link
Collaborator

agreed. light dismiss for modal dialogs should be an opt in, not a default,

@una
Copy link

una commented Jun 15, 2023

+100 we need this feature. And combined with #3567 I think this will solve a lot of the issues right now with popover vs. dialog choices.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jun 15, 2023

One comment that came up in discussion was that the light dismiss behavior should differ from Popover in one respect: if the user clicks outside the dialog to light-dismiss it, that click should not "fall through" to the element underneath the backdrop. That might come for free depending on how we spec/implement this, and how it interacts with inert, but I think it's important to call out so we get that behavior right.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Sep 14, 2023

See openui/open-ui#834 for a quick summary of the TPAC discussion just now, and a new question to discuss the name of the attribute.

@lukewarlow
Copy link
Member

Apologies if this is mentioned in the TPAC discussion or elsewhere that I've missed but I was under the impression the new proposed close watcher concept had a scenario where the nested dialogs could in fact all be closed at once I think it was if they were created without user activation they'd be grouped. I think it's worth maintaining this behaviour for the lightdismiss attribute.

@domenic
Copy link
Member

domenic commented Sep 14, 2023

Apologies if this is mentioned in the TPAC discussion or elsewhere that I've missed but I was under the impression the new proposed close watcher concept had a scenario where the nested dialogs could in fact all be closed at once I think it was if they were created without user activation they'd be grouped. I think it's worth maintaining this behaviour for the lightdismiss attribute.

This is automatic for all "close requests", either popup or dialog or CloseWatcher. It's actually separate from the "light dismiss" behavior, which is triggered when clicking or touching outside of the dialog/popover.

In today's TPAC discussion, we called the Esc key behavior "heavy dismiss" (which all dialogs have), to contrast it with the clicking-outside behavior "light dismiss" (which only <dialog lightdismiss>, or whatever the better name is, would have).

Note that the HTML spec that got merged is a bit confused about this. https://html.spec.whatwg.org/#popover-light-dismiss lists the Esc key behavior under "canceling popovers"; that triggers "hide popover", not "light dismiss open popovers", which is correct. But the Note says that Esc is a subset of light dismiss, and the "canceling popovers" behavior is under the "Popover light dismiss" heading, so the non-normative aspects are confusing. I will add a fix to that to #9462, to make it clear that they are separate.

@lukewarlow
Copy link
Member

Ah yeah that makes sense. I guess in that case my question should be. Should that behaviour be matched by this lightdismiss definition. If you have multiple dialogs opened without user activation should their light dismiss group such that when the ::backdrop is clicked they all close?

It might be odd if escape or back swipe closes them all but you have to manually click out of all of them?

@lukewarlow
Copy link
Member

Made a demo page with a "polyfill" https://demo.lukewarlow.dev/lightdismiss/

@domenic
Copy link
Member

domenic commented Nov 22, 2023

In the poll, many web developers are suggesting enumerated variants like closetrigger="backdrop delay blur etc".

At first I thought this was not a good idea, because we wanted dialogs to have a baseline close trigger of a close request. So the idea was just to add something that says "also give me close-when-clicking-on-backdrop". For which a boolean attribute name was most helpful.

However, in https://bugs.chromium.org/p/chromium/issues/detail?id=1504283 I found out that there are developers using <dialog> today which do not want the dialog to respond to close requests at all. In retrospect, this makes some sense; some dialogs are truly in-your-face important and don't want to be easily dismissed.

So now I think a model something like the following might work?

  • closetrigger="" (no automatic close trigger, can only be closed programatically)
  • closetrigger="closerequest" (closes on a close request, i.e. Esc key / back gesture. Default behavior.)
  • closetrigger="backdropclick" (closes when clicking on the backdrop)
  • closetrigger="closerequest backdropclock" (closes on either)

I'm not 100% satisfied with this API design, because closetrigger="" is a weird way of saying "no close trigger". (But if we added an explicit token, e.g. none, then what does closetrigger="none closerequest" do?) And the default being a specific token is also a bit weird. Maybe there's a better precedent somewhere in the attribute index that avoids these weirdnesses? But it's at least a starting point.

(Regarding naming: as stated before, I think the name needs to include close. And, I quite like connecting up to the backdrop concept, which is exposed through ::backdrop. But I'm not certain on details like closetrigger="" vs. closeon="", or backdropclick vs. backdrop, or closerequest vs. request.)

@lukewarlow
Copy link
Member

lukewarlow commented Nov 22, 2023

I would want to avoid click in the name because it ties it to a specific input type.

I also would want to avoid closeon because it's too similar to onclose.

How about closes=""?

Then can do closes="closerequest backdrop"

closetrigger(s) I think is also fine.

While the empty list might be confusing I think it's probably fine (compared to any alternativs I can think of)?

@bkardell
Copy link
Contributor

bkardell commented Nov 22, 2023

I agree with @lukewarlow on avoiding closeon for the reason he stated... I think that's a strong argument against.

I think closes is appealingly short but also seems kinda wrong way around... it makes it sounds like the dialog is doing the closing of some other things, rather than respecting various close triggers. It's more like closers than closes if that makes sense... Idk, I guess closetrigger is ... accurate? But I kind of hate it.

@lukewarlow
Copy link
Member

closers is quite nice and short.

@keithamus
Copy link
Contributor

keithamus commented Nov 22, 2023

(But if we added an explicit token, e.g. none, then what does closetrigger="none closerequest" do?) And the default being a specific token is also a bit weird. Maybe there's a better precedent somewhere in the attribute index that avoids these weirdnesses? But it's at least a starting point.

I think an explicit closetrigger=none would be ideal, and closetrigger=closerequest being the default seems also fine. Precedent exists for mutually exclusive sets of DOMTokens in the iframe sandbox attribute (whether or not that's a good design pattern and/or one we want to propagate is up for debate):

The allow-top-navigation and allow-top-navigation-by-user-activation keywords must not both be specified, as doing so is redundant; only allow-top-navigation will have an effect in such non-conformant markup.

Similarly, the allow-top-navigation-to-custom-protocols keyword must not be specified if either allow-top-navigation or allow-popups are specified, as doing so is redundant.

I will say I'd estimate the order of popularity of these would be:

  • closetrigger="backdrop closerequest"
  • closetrigger="none"
  • closetrigger="closerequest"

Having the most desirable be the most boilerplate is not the best.

@hidde
Copy link
Member

hidde commented Nov 22, 2023

Agree re closeon, not very keen on closers or closes, they seem fairly vague and therefore not intuitive. I like closetrigger, agreed with Brian it's accurate, and it's clear what to expect as the value.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Nov 22, 2023

I'm not sure about "backdrop" either. This isn't how popovers work, and I'd like to keep the same "click/tap outside" behavior that popover has. For example, clicking outside this dialog should still close it, even though you're not clicking on the backdrop:

<dialog closetrigger="backdrop">
<style>
dialog::backdrop {
  width:10px;
  height:10px;
}
</style>

same with this:

<dialog closetrigger="outside">
<style>
dialog::backdrop {
  pointer-events:none;
}
</style>

Generally, as I mentioned here, I really don't want to start adding other ways to close the dialog. We spent considerable time ruling out "dismiss-on-scroll" or "dismiss-on-blur", so I'd hope we don't add those as values. They will be easy footguns for most people. For the same avoidance-of-footguns reason, I don't see any use case for a dialog that closes when you click/tap outside it, but does not close when you hit ESC.

So there really are just three values/cases, right?

  1. No automatic close at all (including ESC)
  2. Just ESC closes the dialog (current behavior)
  3. ESC or clicking/tapping outside the dialog closes it

How about:

  1. closetrigger=none
  2. closetrigger=closerequest
  3. closetrigger=any

@js-choi
Copy link

js-choi commented Nov 23, 2023

Has closedby="…" been considered as a name?

@domenic
Copy link
Member

domenic commented Nov 23, 2023

I'm not sure about "backdrop" either. This isn't how popovers work, and I'd like to keep the same "click/tap outside" behavior that popover has. For example, clicking outside this dialog should still close it, even though you're not clicking on the backdrop:

Interesting point. I agree backdrop is technically not correct in this way. I still think it might be a reasonable way for developers to think about it, because in the default case clicking outside = clicking the backdrop. But if you think it's too misleading, let's go with outside or something of that sort.

I don't see any use case for a dialog that closes when you click/tap outside it, but does not close when you hit ESC.

This is a good point, and is probably right. However I've already been surprised once by learning that developers sometimes want dialogs that don't respond to close requests. So if anyone disagrees, or can find a dialog of this sort in the web or native platforms, that'd be very helpful info!

How about:

LGTM. (And avoids us having to choose between backdrop and outside!)

The following possible tweaks come to mind, but I think I prefer your proposal as-is. I just want to write them down in case others have strong feelings.

  • closetrigger=request instead of closetrigger=closerequest
  • closetrigger=all instead of closetrigger=any
  • Different names than closetrigger, e.g. closedby, autoclose, closers, etc.

@lukewarlow
Copy link
Member

Am I correct in thinking that escape doesn't actually close a non-modal (non-popover) dialog currently. Is that gonna impact on the serialising of the default?

@lukewarlow
Copy link
Member

Should closetrigger allow you to control popovers too?

Manual popovers require JS currently but that might be heavy handed if you just wanted close request but not click outside?

Don't want to over complicate things though.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Nov 29, 2023

(2) seems pretty nice and natural. The downside, I guess, is that it contributes further to the confusing nature of modeless dialogs, given them even more capabilities.

I'm inclined to agree with you that (2) seems to be the most natural. It keeps the existing behavior for non-modal dialogs unless closetrigger is added, at which point it gives parity with modal dialog behavior. Even though we're hoping to maybe deprecate non-modal dialog, I don't think it's a problem to keep parity on this feature in the meantime. Is there precedent for a missing value default that depends on state? Or do you see any issue with adding that, if not?

There is also a complication about what does.

Here, I think the behavior is pretty clear. If the dialog is open as a dialog, closetrigger adds light dismiss behaviors. If it's open as a popover, then the light dismiss behavior is already there and shouldn't be modified by closetrigger. This is equivalent to the statement that the open attribute on <dialog popover open> doesn't change any popover behavior, it applies just to the dialog open as a dialog. Said another way, I don't think popovers that are dialogs should get special behavior - they should just get the normal popover behavior that any other element gets.

@domenic
Copy link
Member

domenic commented Nov 30, 2023

Is there precedent for a missing value default that depends on state? Or do you see any issue with adding that, if not?

There is precedent; in fact there are three separate types of precedent 😢.

1A: have the states be { none, closerequest, any, auto }, with an invalid value default and missing value default of auto, but no actual keyword for triggering the auto behavior. (The keywords are just { none, closerequest, any }.) Do the reflection "limited to only known values". In this version, whenever you are in the auto state (including when closetrigger="asdf"), you get dialogEl.closeTrigger === "". This is the model used by dir=""and shadowrootmode="".

1B: similar to 1A, but also declare the IDL attribute as nullable. Then instead of the empty string when in the auto state, you get null. This is the model used by popover="" and crossorigin="".

2: The other would be to have the states and keywords both match, as { none, closerequest, any }, and then do custom reflection. This reflection would allow dialogEl.closeTrigger to always return the "actual" close trigger currently in play, changing over time I guess so that by default closetrigger="asdf" maps to dialogEl.closeTrigger === "none", and then after dialogEl.showModal(), it starts returning dialogEl.closeTrigger === "closerequest". Some version of this custom-reflection model is used by translate="", hidden="", contenteditable="", spellcheck="", autocapitalize="", draggable=""

I think 1A is probably better than 1B because we reserve 1B for cases where the empty string is valid (<div popover> and <img crossorigin>). Whereas for closetrigger="" the empty string is not valid.

Between 1A and 2, I lean toward 1A. Although it's kind of nice that in 2 you get some insight into the "actual" close triggers, it's a bit strange how it causes dialogEl.closeTrigger to change over time. But I don't feel strongly.

I recall @annevk having stronger opinions on this, so I'd like his take.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Dec 4, 2023

Thanks for the thorough description of the options!

Between 1A and 2, I lean toward 1A. Although it's kind of nice that in 2 you get some insight into the "actual" close triggers, it's a bit strange how it causes dialogEl.closeTrigger to change over time. But I don't feel strongly.

Any of the options sound ok to me, but 1A does sound the best overall. Option 2 sounds somewhat odd, and the developer can already check dialog.matches(':modal') to see what the behavior would be if dialog.getAttribute('closetrigger')===null. So +1 to option 1A.

@lukewarlow
Copy link
Member

Is this something that would be ready for a spec patch or is more discussion needed on the exact specifics? It seems like there's agreement for option 1A right?

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Feb 22, 2024

Agenda+ to discuss this. It sounds like consensus is forming around this:

<dialog closedby=none> - No automatic close at all (including ESC)
<dialog closedby=closerequest> - Just ESC closes the dialog (current behavior, and new default)
<dialog closedby=any> - ESC or clicking/tapping outside the dialog closes it

with reflection behavior as described in "1A" from #9373 (comment).

The naming data comes from openui/open-ui#960 which has 46% of the vote for closedby, and openui/open-ui#961 which has 61% for closerequest.

@mfreed7 mfreed7 added the agenda+ To be discussed at a triage meeting label Feb 22, 2024
@lukewarlow
Copy link
Member

Something that would be good to get clarity on, if a dialog is showModal()ed with closedby="none" and then it's updated to closerequest or any does that then establish a close watcher or would it have to wait until closing and reshowing modally?

Likewise if the dialog is showModal()ed with closedby of closerequest or any (or auto) and then changed to closedby="none" what happens to the existing close watcher that was established? Is it effectively destroyed?

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Feb 22, 2024

In my opinion, any changes in the value of the closedby attribute should be "live". I.e. changing the value after opening the dialog should take effect immediately, in either direction (adding or removing a closewatcher).

@lukewarlow
Copy link
Member

I agree fwiw, but might be slightly tricky as it will be in the wrong place in the stack potentially. And if you opened the modal with user activation but then switch closedby to create a close watcher after the fact that won't neccesarily have the useractivation you need and again lead to weird behaviours.

Edge casey probably but just worth thinking about the specifics.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Feb 22, 2024

Right - we need to figure out how to implement that. But from a developer point of view, it should "just work". Implementation-wise, we should have access to the guts of the close watcher stack, so this should at least be possible, if perhaps ugly. @domenic would know more.

@lukewarlow lukewarlow linked a pull request Feb 22, 2024 that will close this issue
5 tasks
@lukewarlow
Copy link
Member

Changing popover attribute results in the popover being hidden. That's one possible solution here? Would be simpler to spec and implement that's for sure.

@domenic
Copy link
Member

domenic commented Mar 5, 2024

Ooh, this is tricky.

I think going back and inserting things into the close watcher stack after creation will be very difficult or impossible. So you'd probably need to have some sort of phantom close watcher that always gets placed there.

One sketch might be that all close watchers get an "active" boolean. We then modify various parts of the spec to account for it. E.g. we skip non-active close watchers when issuing a close request, or counting the number of current groups for anti-abuse purposes, or similar.

@lukewarlow
Copy link
Member

I think a phantom watcher could work, the reason I avoided it initially was because I think you can get in a state where rather than being wrong because it's on top of the stack but behind others.
You get an issue where you can end up closing dialogs behind it which equally feels wrong.

I guess to truly know we'd need to give it a go at speccing it and see what could end up being odd.

@lukewarlow
Copy link
Member

As a follow up from todays whatnot I've raised standards positions for WebKit and Mozilla. I'm assuming I can count this as having interest from Chromium given @mfreed7 raised the issue?

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 8, 2024

As a follow up from todays whatnot I've raised standards positions for WebKit and Mozilla. I'm assuming I can count this as having interest from Chromium given @mfreed7 raised the issue?

Chromium is officially supportive. 😊

@smaug----
Copy link
Collaborator

As I mentioned in Mozilla's s-p, it would be good to think whether the features would apply to other cases too, not only . Like, would it make sense to have the same attribute working with popover? And if so, does that affect the API shape?

@keithamus
Copy link
Contributor

keithamus commented Mar 8, 2024

Popover already has the behaviour, kind of. popover=auto has the same behaviour as closedby=any, while popover=manual is the equivalent of closedby=none. We could extend the closedby attribute to popovers but then it would make the value of popover a little redundant.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 8, 2024

Popover already has the behaviour, kind of. popover=auto has the same behaviour as closedby=any, while popover=manual is the equivalent of closedby=none. We could extend the closedby attribute to popovers but then it would make the value of popover a little redundant.

Right, I agree with this comment. I think this type of behavior is very API-specific, and the target is very specifically dialogs. Popover already has this behavior in a different form, and I haven't heard any requests for this type of feature on other elements/APIs. So I'd love to scope this to dialogs if possible.

@domenic
Copy link
Member

domenic commented Mar 9, 2024

While I agree this shouldn't apply to popover, I wonder about whether it could apply to future high-level elements built on popover. If anyone has a list of those, doing some analysis would be helpful.

E.g.: toast? Not sure what their default closedby would be, or whether it'd be reasonable to allow all three options... But even if we wanted to allow only 2 options, that would work fine with this design.

@past past removed the agenda+ To be discussed at a triage meeting label Mar 14, 2024
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 15, 2024

While I agree this shouldn't apply to popover, I wonder about whether it could apply to future high-level elements built on popover. If anyone has a list of those, doing some analysis would be helpful.

E.g.: toast? Not sure what their default closedby would be, or whether it'd be reasonable to allow all three options... But even if we wanted to allow only 2 options, that would work fine with this design.

The two concrete high level elements (obviously modulo name bikeshedding) I've heard discussed are:

  • <toast>
  • <tooltip>

In my head at least, both use the Popover API under the covers, and have an implicit popover=manual or popover=hint, respectively. As such, the toast gets "no light dismiss" functionality, and the tooltip gets normal popover=hint light dismiss, equivalent to closedby=any. And I would think that because these are high level elements, they shouldn't come with the ability to modify these behaviors, since those behaviors are precisely what is baked into the definition of the element.

@lukewarlow
Copy link
Member

since those behaviors are precisely what is baked into the definition of the element.

This I think is key, if we have a specific element built on top of popover and it makes sense to allow two different variants different enough, then they probably should be two separate elements. Of course the same argument could be made for Dialog so maybe I'm wrong.

But dialog is closer to popover than some <bikeshedmenu> component imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: close watchers topic: dialog The <dialog> element.
Development

Successfully merging a pull request may close this issue.