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 · 60 comments · May be fixed by #10737
Open

Add light dismiss functionality to <dialog> #9373

mfreed7 opened this issue Jun 1, 2023 · 60 comments · May be fixed by #10737
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
Contributor

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
Contributor

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

@mfreed7
Copy link
Contributor 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.

@garygreen
Copy link

Not sure how far along this is, but what if individual attributes was used, rather than bikeshedding them into a single string?

<!-- Dialogs are escapable by default, so disable if needed, much like novalidate on forms -->
<!-- if `backdropclose` is present, dialog will call .close() on backdrop click -->
<dialog noescapeclose backdropclose>

Alternatively, as modals can ONLY be opened by Javascript showModal() (not sure why) then maybe configuring behaviour by parameter makes sense?

dialog.showModal({
  closers: { // HTMLDialogCloser?
    escape: true, // true is the default
    backdrop: true // false is default
  }
})

Another option could be to create a dedicated DialogCloseEvent, which inherits the default Event but additionally adds:

  • escaped - whether the dialog was closed by escape key
  • backdrop - whether dialog was closed by clicking on the backdrop

The event should be preventable, so that would make this possible:

<dialog onclose="event.escaped && event.preventDefault()">

This isn't as pretty, but as modals need js and close behaviour is subjective - it might be easier to use if it could be seen what caused the default behaviour, and then to be able to prevent it if needed.

@lukewarlow
Copy link
Member

lukewarlow commented May 18, 2024

Modals will soon ™️ be able to be open and closed declaratively fwiw. See
#9841

One thing to also consider is that non-modal non-popover dialogs have no automatic close by default so using 'nocloserequest' isn't necessarily a good API design.

That's not to say separate attributes aren't possible we would just need to think on how they work given the different states.

@mayank99
Copy link

mayank99 commented Jun 18, 2024

While I'm glad that this functionality is being added to the platform, I have concerns around the current design.

If we're modeling the behavior of tomorrow's closedby=any after today's popover=auto, that means it only closes when pressing Esc or when clicking outside it. Similarly, if we're modeling the behavior of closedby=closerequest after today's modal <dialog>, that means it only closes when pressing Esc. Both of these cases are insufficient.

  • When using closedby=closerequest, I would like my dialogs (and popovers) to also close when using the Back gesture (on Android).
  • When using closedby=any, I would also expect it to close when moving focus out of it (this is mostly relevant for popovers, rather than modal dialogs). For some (not all) popovers, I would also like to close them when the page is scrolled away.

Since <dialog> and popover are already shipped in all three browsers, I'm not sure that it makes sense to add new implicit behaviors to them. For this reason, I think the closedby attribute should apply to both <dialog> and popover.


Further, I think there definitely needs to be more customizability. closedby=any is a good, convenient starting point, but it will not fit all cases. Authors should be able to pick and choose which events will close the dialog/popover.

If you think about how custom JavaScript-based dialogs/popovers are implemented today, authors have almost full control over which events will close the dialog/popover. If we want to satisfy all their advanced use cases, the standard closedby attribute should also offer this level of control. Otherwise authors will be forced to re-implement these common behaviors in JavaScript.

From an API perspective, the closedby attribute could probably accept a list of event types (as an alternative to any). Consider this snippet:

<dialog closedby="closerequest backdropclick focusout">

In this hypothetical example, I'm listing out three event types, one of which (focusout) already exists today, and the other two would be new event types.

  • closerequest event would be fired when pressing Esc and also when using the Back gesture.
  • backdropclick event would be fired when the ::backdrop is clicked.

I don't particularly care about the exact API, but hopefully this example illustrates how we can empowers authors, while still complementing the more convenient closedby=any API.

@lukewarlow
Copy link
Member

lukewarlow commented Jun 18, 2024

Just to provide some quick clarifications (not responding to the core of the comment)

Similarly, if we're modeling the behavior of closedby=closerequest after today's modal , that means it only closes when pressing Esc.

That's not the case it would follow how close watchers (and dialog/popover) work in supporting browsers. E.g. android back gesture or accesibility commands would also close it.

When using closedby=closerequest, I would like my dialogs (and popovers) to also close when using the Back gesture (on Android).

This is already what the html spec says (as much as platform specific behaviours can be specced), for modal dialogs and popovers, and also as mentioned above how closedby would work.

@keithamus
Copy link
Contributor

keithamus commented Jun 18, 2024

Thanks for commenting @mayank99 - it's important to get different perspectives captured here!

If you think about how custom JavaScript-based dialogs/popovers are implemented today, authors have almost full control over which events will close the dialog/popover.

I just want to pick out this part because I think it's important. Given all the customisability JavaScript dialogs/popovers have, I only really see two distinct behaviours from a huge volume of dialogs; those that work with or without a click-backdrop-to-dismiss behaviour or those that don't. I don't see many (or any) where there are significant other differences such as close-on-scroll or close by another keypress or some such.

Could you provide some concrete examples of a dialog or popover being closed during focusout or when scrolled away please? I imagine there won't be any for dialog as this only effects modal dialogs and so focus is trapped - it cannot leave the dialog.

@mayank99
Copy link

mayank99 commented Jun 18, 2024

@keithamus Sure, here are two examples, both really concerned with popover rather than modal <dialog>s (which is why I think this attribute should be added to popover too).

  1. There are plenty of valid use-cases for closing popovers when focus moves out. The most common ones are "composite" widgets that manage their own focus, i.e. custom listboxes, comboboxes, menus (I believe Github's menus do this; try the "create new" menu at the top).
  2. close-on-scroll is less common. I would definitely expect a tooltip to be closed when scrolling away. I've also seen some larger popovers and comboboxes close when scrolling (I believe Github's hover card does this).

Since 1 is so common and is directly tied to WCAG SC 2.4.11, I would expect the platform to help with it.

I was looking at openui/open-ui#1047 and it definitely sounds like it's not a good default because popover is a lower-level API that can be used to build non-modal dialogs that should not close when focus leaves. But I'm thinking that closedby=any is an opt-in thing anyway, so it's probably safe to close on focus out.

Now, I know this thread is specifically about <dialog>, but these things are closely related so I thought it was on topic. It's also directly relevant if you use something like <dialog role="menu"> to build a menu that should close on focusout.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 5, 2024

As I'm working to spec and implement a prototype of this behavior, an interesting case arose:

<dialog closedby=any> I'm a dialog
  <button popovertarget=p>Click me</button>
<dialog>
<div popover id=p>I'm a popover, "nested inside" the dialog</div>

In this case, the developer wants the dialog to "light dismiss", but the definition of "clicking outside" becomes interesting, much in the same way that nested popover are interesting. My assumption would be that clicking on the popover, even if that popover is located outside the bounds of the dialog, does not light dismiss the dialog (or the popover). An example use case would be a modal dialog that contains a <select>. If the picker for that <select> hangs over the edge of the dialog, I don't think the user would expect the dialog to close, and the picker to be left open.

Feedback appreciated. Assuming the consensus is that I'm right about the desired behavior, then I'll work on integrating dialogs into the popover stack in some way, so that this behavior works. Suggestions for that are also appreciated, since it feels a bit tricky.

Another related question:

<dialog id=d1 closedby=any>Dialog 1</dialog>
<dialog id=d2 closedby=any>Dialog 2</dialog>
<dialog id=d3 closedby=any>Dialog 3</dialog>
<script>
  d1.showModal();
  d2.showModal();
  d3.showModal();
</script>

At this point, there are three light-dismissible modal dialogs open. If I click on d3, should d1 and d2 both close? My assumption would be yes, because dialogs don't really imply any kind of nesting or ordering, per-se. So just because d3 opened last, it's still ok to close the "earlier" dialiogs. I think most typical use cases for <dialog> only have a single dialog at a time, but I just want to make sure we shouldn't be trying to impose some kind of "dialog stack" concept like popovers have.

@annevk
Copy link
Member

annevk commented Nov 5, 2024

What's the use case for disabling pressing Esc? Is #9373 (comment) still the proposal or did things change?

@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 5, 2024

What's the use case for disabling pressing Esc?

Some folks want to require an action within the dialog, without a "soft" way to exit back out. See https://issues.chromium.org/issues/40944802 for one such request, related to the connection between dialog and CloseWatcher.

Is #9373 (comment) still the proposal or did things change?

Yep, that's still the proposal. The spec PR (#10737) has all the details.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 13, 2024

I've updated the spec PR (#10737) with these behaviors, which seemed natural enough:

<dialog closedby=any> I'm a dialog
  <button popovertarget=p>Click me</button>
<dialog>
<div popover id=p>I'm a popover, "nested inside" the dialog</div>

It became really tangled to support "nesting" popovers this way inside dialogs, so I just went with standard DOM nesting, which is roughly how dialogs work today anyway. E.g. the inert attribute makes everything outside the DOM tree of the <dialog> inert, so the popover above would be inert. This feels roughly right.

<dialog id=d1 closedby=any>Dialog 1</dialog>
<dialog id=d2 closedby=any>Dialog 2</dialog>
<dialog id=d3 closedby=any>Dialog 3</dialog>
<script>
  d1.showModal();
  d2.showModal();
  d3.showModal();
</script>

Here, I didn't support the concept of "nesting". That's really a popover thing, and it felt weird to try to impose it back on dialogs. In addition, there are many fewer use cases (perhaps no "good" use cases?) for having multiple modal dialogs open at the same time. Anyway, in the above case, the current PR closes d1 and d2 when d3 is clicked.

@yisibl
Copy link

yisibl commented Nov 18, 2024

I think there definitely needs to be more customizability. closedby=any is a good, convenient starting point, but it will not fit all cases. Authors should be able to pick and choose which events will close the dialog/popover.

I'm basically with @mayank99 on this one. Overall, I think more granular behavioral controls are needed.

@domenic
Copy link
Member

domenic commented Nov 21, 2024

In addition, there are many fewer use cases (perhaps no "good" use cases?) for having multiple modal dialogs open at the same time.

This is relatively common in my experience, e.g. a modal save dialog and then an error message dialog if you use invalid characters in your filename.

Anyway, in the above case, the current PR closes d1 and d2 when d3 is clicked.

This seems a bit unfortunate to me.

My expectation would be that light dismiss behaves the same as a close request, and only closes the topmost dialog---at least, in the subset of dialogs that you clicked "outside of". So in your example where all three dialogs are overlapping:

  • Clicking d3 would do nothing (since you're "inside" all three)
  • Clicking the backdrop would close d3 but leave d1 and d2 open.

Whereas in this example: https://jsbin.com/vicuquceji/edit?html,output

  • Clicking in d2 would do nothing (since you're "inside" both d1 and d2)
  • Clicking in d1 but outside d2 would close d2 but leave d1 open.
  • Clicking outside d1 would close d2 and leave d1 open. Doing so a second time would close d1.

Has OpenUI or anyone else done an investigation of dialog light dismiss? I'd have expected this to have come up, e.g. looking at what other design systems do.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 22, 2024

@domenic very good points. And thanks for the example use case. I've opened openui/open-ui#1128 to give this more discussion. Upon putting that issue together, I think I'm coming around to your view, that it would be better to leave the non-topmost dialogs open. But let's discuss. Hopefully you can attend the meeting in two weeks to discuss live.

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 4, 2024
Per the discussions at:
- openui/open-ui#1128
- whatwg/html#9373 (comment)

both developer and editor feedback is that the prior behavior was
a bit odd. In the case where there are "nested" dialogs, the desired
behavior is for light dismiss clicks to just pop the topmost dialog
off the stack, rather than trying to close all non-clicked dialogs.

This CL implements that behavior and updates the test. After
implementing this behavior and playing with it a bit, I agree
with the feedback - this feels more like what users would expect.

Once this lands and I get a bit of feedback, I'll incorporate this
into the spec PR.

Bug: 376516550
Change-Id: I4b5cfb7dd4a27fc304e52759d882b47394e55524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6068807
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1391794}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 4, 2024
Per the discussions at:
- openui/open-ui#1128
- whatwg/html#9373 (comment)

both developer and editor feedback is that the prior behavior was
a bit odd. In the case where there are "nested" dialogs, the desired
behavior is for light dismiss clicks to just pop the topmost dialog
off the stack, rather than trying to close all non-clicked dialogs.

This CL implements that behavior and updates the test. After
implementing this behavior and playing with it a bit, I agree
with the feedback - this feels more like what users would expect.

Once this lands and I get a bit of feedback, I'll incorporate this
into the spec PR.

Bug: 376516550
Change-Id: I4b5cfb7dd4a27fc304e52759d882b47394e55524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6068807
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1391794}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 4, 2024
Per the discussions at:
- openui/open-ui#1128
- whatwg/html#9373 (comment)

both developer and editor feedback is that the prior behavior was
a bit odd. In the case where there are "nested" dialogs, the desired
behavior is for light dismiss clicks to just pop the topmost dialog
off the stack, rather than trying to close all non-clicked dialogs.

This CL implements that behavior and updates the test. After
implementing this behavior and playing with it a bit, I agree
with the feedback - this feels more like what users would expect.

Once this lands and I get a bit of feedback, I'll incorporate this
into the spec PR.

Bug: 376516550
Change-Id: I4b5cfb7dd4a27fc304e52759d882b47394e55524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6068807
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1391794}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 10, 2024
…close one at a time, a=testonly

Automatic update from web-platform-tests
Update dialog closedby behavior to only close one at a time

Per the discussions at:
- openui/open-ui#1128
- whatwg/html#9373 (comment)

both developer and editor feedback is that the prior behavior was
a bit odd. In the case where there are "nested" dialogs, the desired
behavior is for light dismiss clicks to just pop the topmost dialog
off the stack, rather than trying to close all non-clicked dialogs.

This CL implements that behavior and updates the test. After
implementing this behavior and playing with it a bit, I agree
with the feedback - this feels more like what users would expect.

Once this lands and I get a bit of feedback, I'll incorporate this
into the spec PR.

Bug: 376516550
Change-Id: I4b5cfb7dd4a27fc304e52759d882b47394e55524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6068807
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1391794}

--

wpt-commits: e96c4c3d5c6510202598f558ea520b80f9507b31
wpt-pr: 49528
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Dec 12, 2024
…close one at a time, a=testonly

Automatic update from web-platform-tests
Update dialog closedby behavior to only close one at a time

Per the discussions at:
- openui/open-ui#1128
- whatwg/html#9373 (comment)

both developer and editor feedback is that the prior behavior was
a bit odd. In the case where there are "nested" dialogs, the desired
behavior is for light dismiss clicks to just pop the topmost dialog
off the stack, rather than trying to close all non-clicked dialogs.

This CL implements that behavior and updates the test. After
implementing this behavior and playing with it a bit, I agree
with the feedback - this feels more like what users would expect.

Once this lands and I get a bit of feedback, I'll incorporate this
into the spec PR.

Bug: 376516550
Change-Id: I4b5cfb7dd4a27fc304e52759d882b47394e55524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6068807
Commit-Queue: David Baron <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1391794}

--

wpt-commits: e96c4c3d5c6510202598f558ea520b80f9507b31
wpt-pr: 49528
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Dec 13, 2024
…close one at a time, a=testonly

Automatic update from web-platform-tests
Update dialog closedby behavior to only close one at a time

Per the discussions at:
- openui/open-ui#1128
- whatwg/html#9373 (comment)

both developer and editor feedback is that the prior behavior was
a bit odd. In the case where there are "nested" dialogs, the desired
behavior is for light dismiss clicks to just pop the topmost dialog
off the stack, rather than trying to close all non-clicked dialogs.

This CL implements that behavior and updates the test. After
implementing this behavior and playing with it a bit, I agree
with the feedback - this feels more like what users would expect.

Once this lands and I get a bit of feedback, I'll incorporate this
into the spec PR.

Bug: 376516550
Change-Id: I4b5cfb7dd4a27fc304e52759d882b47394e55524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6068807
Commit-Queue: David Baron <dbaronchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1391794}

--

wpt-commits: e96c4c3d5c6510202598f558ea520b80f9507b31
wpt-pr: 49528

UltraBlame original commit: 4079bdd253f749bea402e22f1929ca53a25c228b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Dec 13, 2024
…close one at a time, a=testonly

Automatic update from web-platform-tests
Update dialog closedby behavior to only close one at a time

Per the discussions at:
- openui/open-ui#1128
- whatwg/html#9373 (comment)

both developer and editor feedback is that the prior behavior was
a bit odd. In the case where there are "nested" dialogs, the desired
behavior is for light dismiss clicks to just pop the topmost dialog
off the stack, rather than trying to close all non-clicked dialogs.

This CL implements that behavior and updates the test. After
implementing this behavior and playing with it a bit, I agree
with the feedback - this feels more like what users would expect.

Once this lands and I get a bit of feedback, I'll incorporate this
into the spec PR.

Bug: 376516550
Change-Id: I4b5cfb7dd4a27fc304e52759d882b47394e55524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6068807
Commit-Queue: David Baron <dbaronchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1391794}

--

wpt-commits: e96c4c3d5c6510202598f558ea520b80f9507b31
wpt-pr: 49528

UltraBlame original commit: 4079bdd253f749bea402e22f1929ca53a25c228b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Dec 13, 2024
…close one at a time, a=testonly

Automatic update from web-platform-tests
Update dialog closedby behavior to only close one at a time

Per the discussions at:
- openui/open-ui#1128
- whatwg/html#9373 (comment)

both developer and editor feedback is that the prior behavior was
a bit odd. In the case where there are "nested" dialogs, the desired
behavior is for light dismiss clicks to just pop the topmost dialog
off the stack, rather than trying to close all non-clicked dialogs.

This CL implements that behavior and updates the test. After
implementing this behavior and playing with it a bit, I agree
with the feedback - this feels more like what users would expect.

Once this lands and I get a bit of feedback, I'll incorporate this
into the spec PR.

Bug: 376516550
Change-Id: I4b5cfb7dd4a27fc304e52759d882b47394e55524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6068807
Commit-Queue: David Baron <dbaronchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Reviewed-by: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1391794}

--

wpt-commits: e96c4c3d5c6510202598f558ea520b80f9507b31
wpt-pr: 49528

UltraBlame original commit: 4079bdd253f749bea402e22f1929ca53a25c228b
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