Skip to content

Commit 7ed6b2f

Browse files
mfreed7chromium-scoped@luci-project-accounts.iam.gserviceaccount.com
mfreed7
authored and
[email protected]
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: 499019927 Change-Id: I41d8d96be71ba60ec5e7aa640c935258d1e33431 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7727959 Auto-Submit: Mason Freed <[email protected]> Reviewed-by: David Baron <[email protected]> Commit-Queue: David Baron <[email protected]> Cr-Commit-Position: refs/heads/main@{#1615186}
1 parent b5b9a3c commit 7ed6b2f

15 files changed

Lines changed: 507 additions & 104 deletions

File tree

‎third_party/blink/renderer/core/dom/document.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9449,6 +9449,7 @@ void Document::Trace(Visitor* visitor) const {
94499449
visitor->Trace(top_layer_elements_pending_removal_);
94509450
visitor->Trace(popover_auto_stack_);
94519451
visitor->Trace(popover_hint_stack_);
9452+
visitor->Trace(popover_hint_stack_parent_);
94529453
visitor->Trace(popover_pointerdown_target_);
94539454
visitor->Trace(dialog_pointerdown_target_);
94549455
visitor->Trace(popover_picker_pointerdown_info_);

‎third_party/blink/renderer/core/dom/document.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1719,6 +1719,12 @@ class CORE_EXPORT Document : public ContainerNode,
17191719
const PopoverStack& PopoverHintStack() const { return popover_hint_stack_; }
17201720
PopoverStack& PopoverHintStack() { return popover_hint_stack_; }
17211721
bool PopoverHintShowing() const { return !popover_hint_stack_.empty(); }
1722+
HTMLElement* PopoverHintStackParent() const {
1723+
return popover_hint_stack_parent_.Get();
1724+
}
1725+
void SetPopoverHintStackParent(HTMLElement* parent) {
1726+
popover_hint_stack_parent_ = parent;
1727+
}
17221728
PopoverStack& PopoverAutoStack() { return popover_auto_stack_; }
17231729
const PopoverStack& PopoverAutoStack() const { return popover_auto_stack_; }
17241730
bool PopoverAutoShowing() const { return !popover_auto_stack_.empty(); }
@@ -2973,6 +2979,10 @@ class CORE_EXPORT Document : public ContainerNode,
29732979
// stack is the same as for `popover_auto_stack_`. This stack will only ever
29742980
// contain `popover=hint` elements, and nothing else.
29752981
HeapVector<Member<HTMLElement>> popover_hint_stack_;
2982+
// Tracks the auto popover (if any) that serves as the root/parent for the
2983+
// current hint popover stack. Null if the hint stack is empty or not anchored
2984+
// to an auto popover.
2985+
Member<HTMLElement> popover_hint_stack_parent_;
29762986
// The popover (if any) that received the most recent pointerdown event.
29772987
Member<const HTMLElement> popover_pointerdown_target_;
29782988
// The dialog (if any) that received the most recent pointerdown event. This

‎third_party/blink/renderer/core/html/html_element.cc‎

Lines changed: 229 additions & 59 deletions
Large diffs are not rendered by default.

‎third_party/blink/renderer/core/html/html_element.h‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -314,12 +314,11 @@ class CORE_EXPORT HTMLElement : public Element {
314314
HidePopoverTransitionBehavior event_firing,
315315
ExceptionState* exception_state);
316316
void PopoverHideFinishIfNeeded(bool immediate);
317-
static const HTMLElement* FindTopmostPopoverAncestor(
317+
static HTMLElement* FindTopmostPopoverAncestor(
318318
Element& new_popover_or_top_layer_element,
319-
HeapVector<Member<HTMLElement>>& stack_to_check,
319+
HeapVector<Member<HTMLElement>>* stack_to_check,
320320
Element* new_popovers_invoker,
321-
TopLayerElementType top_layer_element_type =
322-
TopLayerElementType::kPopover);
321+
TopLayerElementType top_layer_element_type);
323322
static const HTMLElement* TopLayerElementPopoverAncestor(
324323
Element& top_layer_element,
325324
TopLayerElementType top_layer_element_type);
@@ -334,6 +333,8 @@ class CORE_EXPORT HTMLElement : public Element {
334333
// pointerup events.
335334
static void HandlePopoverLightDismiss(const PointerEvent& event,
336335
const Node& node);
336+
static void HidePopoversForLightDismiss(const HTMLElement* target_popover,
337+
Document& document);
337338
static void HandlePopoverLightDismissForClick(const Node& pointer_down_target,
338339
const Node& pointer_up_target);
339340
void InvokePopover(Element& invoker);

‎third_party/blink/renderer/platform/runtime_enabled_features.json5‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4355,6 +4355,12 @@
43554355
name: "PointerRawUpdateOnlyInSecureContext",
43564356
status: "stable",
43574357
},
4358+
{
4359+
// Simplifies the behavior and stacking interactions of popover=hint and
4360+
// popover=auto. See https://crbug.com/499019927.
4361+
name: "PopoverHintNewBehavior",
4362+
status: "experimental",
4363+
},
43584364
{
43594365
name: "PositionOutsideTabSpanCheckSiblingNode",
43604366
status: "stable",

‎third_party/blink/web_tests/VirtualTestSuites‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6387,5 +6387,20 @@
63876387
"[email protected]"
63886388
],
63896389
"expires": "Jan 1, 2027"
6390+
},
6391+
{
6392+
"prefix": "popover-hint-new-behavior-disabled",
6393+
"platforms": ["Linux", "Mac", "Win"],
6394+
"bases": [
6395+
"external/wpt/html/semantics/popovers",
6396+
"inspector-protocol/dom/dom-forceShowPopover.js"
6397+
],
6398+
"args": [
6399+
"--disable-blink-features=PopoverHintNewBehavior"
6400+
],
6401+
"expires": "Dec 1, 2026",
6402+
"owners": [
6403+
"[email protected]"
6404+
]
63906405
}
63916406
]

‎third_party/blink/web_tests/external/wpt/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:[email protected]">
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>

‎third_party/blink/web_tests/external/wpt/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>

‎third_party/blink/web_tests/external/wpt/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

‎third_party/blink/web_tests/external/wpt/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>

0 commit comments

Comments
 (0)