Skip to content

Commit cf48cdc

Browse files
mfreed7chromium-wpt-export-bot
mfreed7
authored and
chromium-wpt-export-bot
committed
Implement new/revised popover=hint behavior
See this discussion for more context: whatwg/html#12304 whatwg/html#12345 This CL implements a simplified behavioral model for popover=hint and popover=auto, cleanly separating the autoRootedPopoverStack from the hintRootedPopoverStack and introducing a `hintStackParent` to track where the hint stack branches off the auto stack. The new behavior resolves the following inconsistencies (gated behind the `PopoverHintNewBehavior` experimental runtime flag): 1. Opening a hint popover will not hide unrelated auto popovers. 2. Opening a hint popover closes only other non-ancestor hint popovers. 3. Clicking outside consistently closes both auto and hint popovers. 4. Hiding an auto popover closes only its child popovers. 5. Opening an auto popover inside a hint popover is disallowed and will fail. A new WPT test (`popover-hint-hierarchy.html`) is added to explicitly assert these 5 rules, and existing popover WPTs are updated to reflect the new behavior. A virtual test suite (`popover-hint-old-behavior`) is also added to continue testing the legacy behavior. I'm not sure how to best handle the changed tests (and I guess the new test too) in the context of a spec PR that is forthcoming. I'd like to be able to publish (as a WPT PR at least) the changes, to help with the PR effort. Suggestions appreciated. A note about the diff: I made all of the feature flag checks look like `if (!feature enabled) {` in the hopes that the old code would show as unchanged in the diff, but gerrit's diff algorithm isn't great. It's at least a little better like this than if I put the new code first, so I left it. Bug: 4990199 Change-Id: I41d8d96be71ba60ec5e7aa640c935258d1e33431 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7727959 Auto-Submit: Mason Freed <masonf@chromium.org> Reviewed-by: David Baron <dbaron@chromium.org> Commit-Queue: David Baron <dbaron@chromium.org> Cr-Commit-Position: refs/heads/main@{#1615186}
1 parent dd588c0 commit cf48cdc

5 files changed

Lines changed: 205 additions & 41 deletions

File tree

‎html/semantics/popovers/popover-hint-hierarchy.html‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
<!DOCTYPE html>
2+
<meta charset="utf-8" />
3+
<link rel="author" href="mailto:masonf@chromium.org">
4+
<link rel="help" href="https://open-ui.org/components/popover-hint.research.explainer">
5+
<script src="/resources/testharness.js"></script>
6+
<script src="/resources/testharnessreport.js"></script>
7+
<script src="/resources/testdriver.js"></script>
8+
<script src="/resources/testdriver-actions.js"></script>
9+
<script src="/resources/testdriver-vendor.js"></script>
10+
<script src="resources/popover-utils.js"></script>
11+
12+
<div id="auto1" popover="auto">Auto 1</div>
13+
<div id="auto2" popover="auto">Auto 2</div>
14+
<div id="hint1" popover="hint">Hint 1</div>
15+
<div id="hint2" popover="hint">Hint 2</div>
16+
<div id="hint3" popover="hint">Hint 3</div>
17+
18+
<style>
19+
[popover] {
20+
inset: auto;
21+
}
22+
#auto1 { top: 0; }
23+
#auto2 { top: 50px; }
24+
#hint1 { top: 100px; }
25+
#hint2 { top: 150px; }
26+
#hint3 { top: 200px; }
27+
</style>
28+
29+
30+
<script>
31+
const popovers = Array.from(document.querySelectorAll('[popover]'));
32+
function hideAll() {
33+
popovers.forEach(p => {
34+
try { p.hidePopover(); } catch {}
35+
});
36+
}
37+
function assertOpen(msg, ...list) {
38+
const openPopovers = popovers.filter((p) => p.matches(':popover-open'));
39+
assert_array_equals(openPopovers,list,msg);
40+
}
41+
42+
// this is "weirdness 1" from https://github.com/whatwg/html/issues/12304
43+
test((t) => {
44+
t.add_cleanup(hideAll);
45+
46+
auto1.showPopover();
47+
auto2.showPopover({source: auto1});
48+
hint1.showPopover({source: auto1});
49+
assertOpen('',auto1, auto2, hint1);
50+
}, 'Opening a hint popover will not hide unrelated auto popovers.');
51+
52+
// this is "weirdness 2" from https://github.com/whatwg/html/issues/12304
53+
test((t) => {
54+
t.add_cleanup(hideAll);
55+
56+
auto1.showPopover();
57+
hint1.showPopover({source: auto1});
58+
hint2.showPopover({source: hint1});
59+
assertOpen('initial',auto1, hint1, hint2);
60+
hint3.showPopover();
61+
assertOpen('after hint3 show',auto1, hint3);
62+
hint1.showPopover({source: auto1});
63+
assertOpen('after hint1 show',auto1, hint1);
64+
}, 'Opening a hint popover closes only other non-ancestor hint popovers.');
65+
66+
// this is "weirdness 3" from https://github.com/whatwg/html/issues/12304
67+
promise_test(async (t) => {
68+
t.add_cleanup(hideAll);
69+
70+
auto1.showPopover();
71+
hint1.showPopover();
72+
assertOpen('initial',auto1, hint1);
73+
74+
await clickOn(hint1);
75+
assertOpen('after hint click',hint1);
76+
77+
auto1.showPopover();
78+
assertOpen('auto reopen hides hint',auto1);
79+
hint1.showPopover();
80+
assertOpen('reopen hint',auto1,hint1);
81+
82+
await clickOn(auto1);
83+
assertOpen('after auto click',auto1);
84+
}, 'Clicking outside consistently closes both auto and hint popovers.');
85+
86+
// this is "weirdness 4" from https://github.com/whatwg/html/issues/12304
87+
test((t) => {
88+
t.add_cleanup(hideAll);
89+
90+
auto1.showPopover();
91+
hint1.showPopover();
92+
assertOpen('initial',auto1, hint1);
93+
94+
auto1.hidePopover();
95+
assertOpen('after auto hide',hint1);
96+
97+
auto1.showPopover();
98+
assertOpen('auto reopen hides hint',auto1);
99+
hint1.showPopover();
100+
assertOpen('reopen hint',auto1,hint1);
101+
102+
hint1.hidePopover();
103+
assertOpen('after hint hide',auto1);
104+
}, 'Hiding an auto popover closes only its child popovers.');
105+
106+
// this is "weirdness 5" from https://github.com/whatwg/html/issues/12304
107+
promise_test(async (t) => {
108+
t.add_cleanup(hideAll);
109+
110+
hint1.showPopover();
111+
assertOpen('initial',hint1);
112+
113+
// Imperative
114+
assert_throws_dom('InvalidStateError', () => {
115+
auto1.showPopover({source: hint1});
116+
}, 'Opening auto popover inside hint popover is not allowed');
117+
assertOpen('after show',hint1);
118+
119+
// Declarative
120+
const invoker = document.createElement('button');
121+
invoker.popoverTargetElement = auto1;
122+
t.add_cleanup(() => invoker.remove());
123+
hint1.appendChild(invoker);
124+
await clickOn(invoker);
125+
assertOpen('after click',hint1);
126+
}, 'Opening an auto popover inside a hint popover is disallowed and will fail.');
127+
</script>

‎html/semantics/popovers/popover-light-dismiss-hint.html‎

Lines changed: 44 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -43,65 +43,80 @@
4343
</style>
4444

4545
<script>
46-
const popovers = [
46+
const setA = [
4747
document.querySelector('#auto1'),
4848
document.querySelector('#auto2'),
4949
document.querySelector('#innerhint1'),
5050
document.querySelector('#innerhint2'),
51+
document.querySelector('#manual1'),
52+
];
53+
const setB = [
5154
document.querySelector('#hint1'),
5255
document.querySelector('#hint2'),
5356
document.querySelector('#manual1'),
5457
];
55-
function assertState(expectedState,description) {
58+
59+
function assertState(popovers, expectedState,description) {
5660
description = description || 'Error';
5761
const n = popovers.length;
5862
assert_equals(expectedState.length,n,'Invalid');
5963
for(let i=0;i<n;++i) {
6064
assert_equals(popovers[i].matches(':popover-open'),expectedState[i],`${description}, index ${i} (${popovers[i].id})`);
6165
}
6266
}
63-
function openall(t) {
64-
// All popovers can be open at once, if shown in order:
67+
68+
function openall(popovers, t) {
6569
popovers.forEach((p) => p.hidePopover());
6670
popovers.forEach((p) => p.showPopover());
67-
assertState(Array(popovers.length).fill(true),'All popovers should be able to be open at once');
71+
assertState(popovers, Array(popovers.length).fill(true),'All popovers should be able to be open at once');
6872
t.add_cleanup(() => popovers.forEach((p) => p.hidePopover()));
6973
}
74+
7075
function nvals(n,val) {
7176
return new Array(n).fill(val);
7277
}
73-
for(let i=0;i<(popovers.length-1);++i) {
78+
79+
for(let i=0;i<(setA.length-1);++i) {
80+
promise_test(async (t) => {
81+
openall(setA, t);
82+
await clickOn(setA[i]);
83+
let expectedState = [...nvals(i+1,true),...nvals(setA.length-i-2,false),true];
84+
assertState(setA, expectedState);
85+
}, 'Mixed auto/hint light dismiss behavior, click on ' + setA[i].id);
86+
}
87+
88+
for(let i=0;i<(setB.length-1);++i) {
7489
promise_test(async (t) => {
75-
openall(t);
76-
await clickOn(popovers[i]);
77-
let expectedState = [...nvals(i+1,true),...nvals(popovers.length-i-2,false),true];
78-
assertState(expectedState);
79-
},`Mixed auto/hint light dismiss behavior, click on ${popovers[i].id}`);
90+
openall(setB, t);
91+
await clickOn(setB[i]);
92+
let expectedState = [...nvals(i+1,true),...nvals(setB.length-i-2,false),true];
93+
assertState(setB, expectedState);
94+
}, 'Mixed auto/hint light dismiss behavior, click on ' + setB[i].id);
8095
}
8196

8297
promise_test(async (t) => {
83-
openall(t);
98+
openall(setA, t);
99+
await clickOn(outside);
100+
assertState(setA, [false,false,false,false,true]);
101+
},'Clicking outside closes all setA');
102+
103+
promise_test(async (t) => {
104+
openall(setB, t);
84105
await clickOn(outside);
85-
assertState([false,false,false,false,false,false,true]);
86-
},'Clicking outside closes all');
106+
assertState(setB, [false,false,true]);
107+
},'Clicking outside closes all setB');
87108

88109
promise_test(async (t) => {
89-
openall(t);
90-
invalidauto1.showPopover();
91-
assertState([true,true,false,false,false,false,true],'auto inside hint ignores the hints and gets nested within auto2');
92-
assert_true(invalidauto1.matches(':popover-open'),'the inner nested auto should be open');
93-
invalidauto1.hidePopover();
94-
assertState([true,true,false,false,false,false,true]);
95-
assert_false(invalidauto1.matches(':popover-open'));
110+
openall(setA, t);
111+
assert_throws_dom('InvalidStateError', () => {
112+
invalidauto1.showPopover();
113+
});
96114
},'Auto cannot be nested inside hint (invalidauto1)');
97115

98116
promise_test(async (t) => {
99-
openall(t);
100-
invalidauto2.showPopover();
101-
assertState([false,false,false,false,false,false,true],'auto inside hint works as an independent (non-nested) auto');
102-
assert_true(invalidauto2.matches(':popover-open'),'the inner nested auto should be open');
103-
invalidauto2.hidePopover();
104-
assertState([false,false,false,false,false,false,true]);
105-
assert_false(invalidauto2.matches(':popover-open'));
117+
openall(setB, t);
118+
assert_throws_dom('InvalidStateError', () => {
119+
invalidauto2.showPopover();
120+
});
106121
},'Auto cannot be nested inside hint (invalidauto2)');
107-
</script>
122+
</script>

‎html/semantics/popovers/popover-top-layer-nesting-hints.html‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,18 +25,18 @@
2525

2626
<div> Nested auto/hint ancestors, target is auto
2727
<div popover=auto class=target data-stay-open=true>
28-
<div popover=hint data-stay-open=false></div>
28+
<div popover=hint data-stay-open=true data-stay-open-fullscreen=false></div>
2929
</div>
3030
</div>
3131

3232
<div> Unrelated hint, target=hint
33-
<div popover=auto data-stay-open=true></div>
33+
<div popover=auto data-stay-open=true data-stay-open-fullscreen=false></div>
3434
<div popover=hint class=target data-stay-open=true></div>
3535
</div>
3636

3737
<div> Unrelated hint, target=auto
3838
<div popover=auto class=target data-stay-open=true></div>
39-
<div popover=hint data-stay-open=false></div>
39+
<div popover=hint data-stay-open=true data-stay-open-fullscreen=false></div>
4040
</div>
4141
</div>
4242

‎html/semantics/popovers/popover-types-with-hints.html‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@
5050
hint.showPopover();
5151
assertState([true,true,true,true]);
5252
auto.hidePopover();
53-
assertState([false,false,true,true]);
53+
assertState([false,true,true,true]);
5454
hint.hidePopover();
5555
manual.hidePopover();
5656
assertState([false,false,false,true]);
@@ -104,9 +104,11 @@
104104
const auto = getPopovers()[1];
105105
hint.showPopover();
106106
assertState([true,false]);
107-
auto.showPopover();
108-
assertState([false,true]);
109-
auto.hidePopover();
107+
assert_throws_dom('InvalidStateError', () => {
108+
auto.showPopover();
109+
});
110+
assertState([true,false]);
111+
hint.hidePopover();
110112
assertState([false,false]);
111113
},'If a popover=auto is shown, it should hide any open popover=hint, including if the popover=hint is an ancestral popover of the popover=auto. (You can\'t nest a popover=auto inside a popover=hint)');
112114
</script>
@@ -125,8 +127,8 @@
125127
auto.showPopover();
126128
auto2.showPopover();
127129
assertState([true,true,false]);
128-
hint.showPopover(); // This should hide auto2, since it is nested in auto1.
129-
assertState([true,false,true]);
130+
hint.showPopover(); // This should NOT hide auto2, since hint doesn't close unrelated autos.
131+
assertState([true,true,true]);
130132
auto.hidePopover(); // Should hide both auto and hint.
131133
assertState([false,false,false]);
132134
},'If you: a) show a popover=auto (call it D), then b) show a descendent popover=hint of D (call it T), then c) hide D, then T should be hidden. (A popover=hint can be nested inside a popover=auto)');
@@ -144,6 +146,8 @@
144146
hint.showPopover();
145147
assertState([true,true]);
146148
auto.hidePopover();
149+
assertState([false,true]);
150+
hint.hidePopover();
147151
assertState([false,false]);
148152
},'If you: a) show a popover=auto (call it D), then b) show a non-descendent popover=hint of D (call it T), then c) hide D, then T should be hidden. (Non-nested popover=hint gets hidden when unrelated popover=autos are hidden)');
149153
</script>

‎html/semantics/popovers/resources/popover-top-layer-nesting.js‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,36 @@ function runTopLayerTests(testCases) {
4242
// Activate the top layer element.
4343
await show(popovers[popovers.length-1]);
4444
assert_true(showing());
45-
popovers.forEach(popover => assert_equals(popover.matches(':popover-open'),popover.dataset.stayOpen==='true','Incorrect behavior'));
45+
popovers.forEach((popover, i) => {
46+
let expected = popover.dataset.stayOpen === 'true';
47+
if (topLayerType === 'fullscreen' && popover.dataset.stayOpenFullscreen !== undefined) {
48+
expected = popover.dataset.stayOpenFullscreen === 'true';
49+
}
50+
51+
});
4652

4753
// Add another popover within the top layer element and make sure entire stack stays open.
4854
const newPopover = document.createElement('div');
4955
t.add_cleanup(() => newPopover.remove());
5056
newPopover.popover = 'hint';
5157
element.appendChild(newPopover);
52-
popovers.forEach(popover => assert_equals(popover.matches(':popover-open'),popover.dataset.stayOpen==='true','Adding another popover shouldn\'t change anything'));
58+
popovers.forEach((popover, i) => {
59+
let expected = popover.dataset.stayOpen === 'true';
60+
if (topLayerType === 'fullscreen' && popover.dataset.stayOpenFullscreen !== undefined) {
61+
expected = popover.dataset.stayOpenFullscreen === 'true';
62+
}
63+
64+
});
5365
assert_true(showing(),'top layer element should still be top layer');
5466
newPopover.showPopover();
5567
assert_true(newPopover.matches(':popover-open'));
56-
popovers.forEach(popover => assert_equals(popover.matches(':popover-open'),popover.dataset.stayOpen==='true','Showing the popover shouldn\'t change anything'));
68+
popovers.forEach((popover, i) => {
69+
let expected = popover.dataset.stayOpen === 'true';
70+
if (topLayerType === 'fullscreen' && popover.dataset.stayOpenFullscreen !== undefined) {
71+
expected = popover.dataset.stayOpenFullscreen === 'true';
72+
}
73+
74+
});
5775
assert_true(showing(),'top layer element should still be top layer');
5876
},`${description} with ${topLayerType}`);
5977

0 commit comments

Comments
 (0)