Skip to content

Commit

Permalink
Update dialog closedby behavior to only close one at a time
Browse files Browse the repository at this point in the history
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 <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1391794}
  • Loading branch information
mfreed7 authored and chromium-wpt-export-bot committed Dec 4, 2024
1 parent c4c6722 commit e96c4c3
Showing 1 changed file with 19 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#popoverA { top: 150px; bottom: auto; padding:0; }
#dialogB { top: 200px; bottom: auto; padding:0; }
#popoverB { top: 250px; bottom: auto; padding:0; }
dialog { position: fixed; }
</style>

<script>
Expand Down Expand Up @@ -71,38 +72,43 @@
await clickOn(unrelated);
// Clicking outside all is actually a click on a dialog backdrop.
// If dialogB is modal, it'll be dialogB, which is nested inside popoverA.
assertStates(false,modalB,false,false);
// Either way, both popoverB and dialogB should close.
assertStates(true,modalB,false,false);
await clickOn(unrelated);
// Clicking outside again should close the remaining two.
assertStates(false,false,false,false);
},`clicking outside all with ${modalAString} and ${modalBString}`);

promise_test(async (t) => {
openDialogPopoverStack(t,modalA,modalB);
await clickOn(popoverB);
// Clicking popoverB will keep both popovers plus the intervening dialogB
// open, because they're a stack.
assertStates(false,true,true,true);
// Clicking popoverB should keep everything open.
assertStates(true,true,true,true);
},`clicking popoverB with ${modalAString} and ${modalBString}`);

promise_test(async (t) => {
openDialogPopoverStack(t,modalA,modalB);
await clickOn(dialogB);
// dialogB is nested inside popoverA.
assertStates(false,true,true,false);
// Only popoverB should be light dismissed.
assertStates(true,true,true,false);
},`clicking dialogB with ${modalAString} and ${modalBString}`);

promise_test(async (t) => {
openDialogPopoverStack(t,modalA,modalB);
await clickOn(popoverA);
// If dialogB is modal, then clicking popoverA is actually a backdrop
// click on dialogB, which will close it. PopoverA stays open because
// dialogB is nested inside popoverA.
assertStates(false,true,!modalB,false);
// Both dialogB and popoverB should be light dismissed.
assertStates(true,true,false,false);
},`clicking popoverA with ${modalAString} and ${modalBString}`);

promise_test(async (t) => {
openDialogPopoverStack(t,modalA,modalB);
await clickOn(dialogB);
// Again, this is a backdrop click on dialogB.
assertStates(false,true,true,false);
await clickOn(dialogA);
// If dialogB is modal, clicking on dialogA is actually clicking on dialogB,
// which means popoverB will stay open.
assertStates(true,modalB,false,false);
await clickOn(dialogA);
// The next click on dialogA should light dismiss popoverA.
assertStates(true,false,false,false);
},`clicking dialogA with ${modalAString} and ${modalBString}`);
});
});
Expand Down

0 comments on commit e96c4c3

Please sign in to comment.