Skip to content

Commit 9c8a390

Browse files
mfreed7marcoscaceres
authored andcommitted
Allow top layer elements to be nested within popovers
See the issue raised at whatwg/html#9998 which was discussed at whatwg/html#9993 This CL makes the following changes: 1. Change `FindTopmostPopoverAncestor()` so that the provided element does not have to be a popover. The logic does not materially change - all of the same mechanisms can be used to connect a non-popover top layer element (dialog or fullscreen) to an ancestor popover. 2. Add a utility `TopLayerElementPopoverAncestor()` which finds the popover ancestor for a provided top layer element by calling `FindTopmostPopoverAncestor()` with the proper arguments. 3. In dialog and fullscreen code, where it previously called `HideAllPopoversUntil(nullptr,...)` to hide **all** open popovers, it now (with flag enabled) hides only up to the nearest popover ancestor. Tests are added, which are marked `.tentative`. Bug: 1520938 Change-Id: I8d2c4000ed3959ac4e8bf521e22e9dfd532c62d5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5229300 Auto-Submit: Mason Freed <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1254541}
1 parent c5c8964 commit 9c8a390

5 files changed

+245
-0
lines changed

html/semantics/popovers/popover-attribute-basic.html

+1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
<!DOCTYPE html>
22
<meta charset="utf-8">
33
<link rel="author" href="mailto:masonf@chromium.org">
4+
<link rel=help href="https://html.spec.whatwg.org/multipage/popover.html">
45
<link rel=help href="https://open-ui.org/components/popover.research.explainer">
56
<meta name="timeout" content="long">
67
<script src="/resources/testharness.js"></script>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<!DOCTYPE html>
2+
<meta charset="utf-8">
3+
<link rel="author" href="mailto:masonf@chromium.org">
4+
<link rel=help href="https://html.spec.whatwg.org/multipage/popover.html">
5+
<link rel=help href="https://open-ui.org/components/popover.research.explainer">
6+
<script src="/resources/testharness.js"></script>
7+
<script src="/resources/testharnessreport.js"></script>
8+
<script src="/resources/testdriver.js"></script>
9+
<script src="/resources/testdriver-vendor.js"></script>
10+
<script src="/common/top-layer.js"></script>
11+
<script src="resources/popover-utils.js"></script>
12+
<script src="resources/popover-top-layer-nesting.js"></script>
13+
14+
<div id=tests>
15+
<div> Single popover=auto ancestor
16+
<div popover=auto class=target data-stay-open=true></div>
17+
</div>
18+
19+
<div> Single popover=manual ancestor
20+
<div popover=auto class=target data-stay-open=true></div>
21+
</div>
22+
23+
<div> Nested popover=auto ancestors
24+
<div popover=auto data-stay-open=true>
25+
<div popover=auto class=target data-stay-open=true></div>
26+
</div>
27+
</div>
28+
29+
<div> Nested popover=auto ancestors, target is outer
30+
<div popover=auto class=target data-stay-open=true>
31+
<div popover=auto data-stay-open=false></div>
32+
</div>
33+
</div>
34+
35+
<div> Top layer inside of nested element
36+
<div popover=auto data-stay-open=true>
37+
<button class=target></button>
38+
</div>
39+
</div>
40+
</div>
41+
42+
<script>
43+
const tests = Array.from(document.querySelectorAll('#tests>div'));
44+
runTopLayerTests(tests,/*testAnchorAttribute*/true);
45+
</script>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<!DOCTYPE html>
2+
<meta charset="utf-8">
3+
<link rel="author" href="mailto:masonf@chromium.org">
4+
<link rel=help href="https://html.spec.whatwg.org/multipage/popover.html">
5+
<link rel=help href="https://open-ui.org/components/popover.research.explainer">
6+
<link rel=help href="https://open-ui.org/components/popover-hint.research.explainer">
7+
<script src="/resources/testharness.js"></script>
8+
<script src="/resources/testharnessreport.js"></script>
9+
<script src="/resources/testdriver.js"></script>
10+
<script src="/resources/testdriver-vendor.js"></script>
11+
<script src="/common/top-layer.js"></script>
12+
<script src="resources/popover-utils.js"></script>
13+
<script src="resources/popover-top-layer-nesting.js"></script>
14+
15+
<div id=tests>
16+
<div> Single popover=hint ancestor
17+
<div popover=hint class=target data-stay-open=true></div>
18+
</div>
19+
20+
<div> Nested auto/hint ancestors
21+
<div popover=auto data-stay-open=true>
22+
<div popover=hint class=target data-stay-open=true></div>
23+
</div>
24+
</div>
25+
26+
<div> Nested auto/hint ancestors, target is auto
27+
<div popover=auto class=target data-stay-open=true>
28+
<div popover=hint data-stay-open=false></div>
29+
</div>
30+
</div>
31+
32+
<div> Unrelated hint, target=hint
33+
<div popover=auto data-stay-open=true></div>
34+
<div popover=hint class=target data-stay-open=true></div>
35+
</div>
36+
37+
<div> Unrelated hint, target=auto
38+
<div popover=auto class=target data-stay-open=true></div>
39+
<div popover=hint data-stay-open=false></div>
40+
</div>
41+
</div>
42+
43+
<script>
44+
const tests = Array.from(document.querySelectorAll('#tests>div'));
45+
runTopLayerTests(tests);
46+
</script>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<!DOCTYPE html>
2+
<meta charset="utf-8">
3+
<link rel="author" href="mailto:masonf@chromium.org">
4+
<link rel=help href="https://html.spec.whatwg.org/multipage/popover.html">
5+
<link rel=help href="https://open-ui.org/components/popover.research.explainer">
6+
<script src="/resources/testharness.js"></script>
7+
<script src="/resources/testharnessreport.js"></script>
8+
<script src="/resources/testdriver.js"></script>
9+
<script src="/resources/testdriver-vendor.js"></script>
10+
<script src="/common/top-layer.js"></script>
11+
<script src="resources/popover-utils.js"></script>
12+
<script src="resources/popover-top-layer-nesting.js"></script>
13+
14+
<div id=tests>
15+
<div> Single popover=auto ancestor
16+
<div popover=auto class=target data-stay-open=true></div>
17+
</div>
18+
19+
<div> Single popover=manual ancestor
20+
<div popover=auto class=target data-stay-open=true></div>
21+
</div>
22+
23+
<div> Nested popover=auto ancestors
24+
<div popover=auto data-stay-open=true>
25+
<div popover=auto class=target data-stay-open=true></div>
26+
</div>
27+
</div>
28+
29+
<div> Nested popover=auto ancestors, target is outer
30+
<div popover=auto class=target data-stay-open=true>
31+
<div popover=auto data-stay-open=false></div>
32+
</div>
33+
</div>
34+
35+
<div> Top layer inside of nested element
36+
<div popover=auto data-stay-open=true>
37+
<button class=target></button>
38+
</div>
39+
</div>
40+
</div>
41+
42+
<script>
43+
const tests = Array.from(document.querySelectorAll('#tests>div'));
44+
runTopLayerTests(tests);
45+
</script>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
function createTopLayerElement(t,topLayerType) {
2+
let element, show, showing;
3+
switch (topLayerType) {
4+
case 'dialog':
5+
element = document.createElement('dialog');
6+
show = () => element.showModal();
7+
showing = () => element.matches(':modal');
8+
break;
9+
case 'fullscreen':
10+
element = document.createElement('div');
11+
show = async (topmostElement) => {
12+
// Be sure to add user activation to the topmost visible target:
13+
await blessTopLayer(topmostElement);
14+
await element.requestFullscreen();
15+
};
16+
showing = () => document.fullscreenElement === element;
17+
break;
18+
default:
19+
assert_unreached('Invalid top layer type');
20+
}
21+
t.add_cleanup(() => element.remove());
22+
return {element,show,showing};
23+
}
24+
function runTopLayerTests(testCases, testAnchorAttribute) {
25+
testAnchorAttribute = testAnchorAttribute || false;
26+
testCases.forEach(test => {
27+
const description = test.firstChild.data.trim();
28+
assert_equals(test.querySelectorAll('.target').length,1,'There should be exactly one target');
29+
const target = test.querySelector('.target');
30+
assert_true(!!target,'Invalid test case');
31+
const popovers = Array.from(test.querySelectorAll('[popover]'));
32+
assert_true(popovers.length > 0,'No popovers found');
33+
['dialog','fullscreen'].forEach(topLayerType => {
34+
promise_test(async t => {
35+
const {element,show,showing} = createTopLayerElement(t,topLayerType);
36+
target.appendChild(element);
37+
38+
// Show the popovers.
39+
t.add_cleanup(() => popovers.forEach(popover => popover.hidePopover()));
40+
popovers.forEach(popover => popover.showPopover());
41+
popovers.forEach(popover => assert_true(popover.matches(':popover-open'),'All popovers should be open'));
42+
43+
// Activate the top layer element.
44+
await show(popovers[popovers.length-1]);
45+
assert_true(showing());
46+
popovers.forEach(popover => assert_equals(popover.matches(':popover-open'),popover.dataset.stayOpen==='true','Incorrect behavior'));
47+
48+
// Add another popover within the top layer element and make sure entire stack stays open.
49+
const newPopover = document.createElement('div');
50+
t.add_cleanup(() => newPopover.remove());
51+
newPopover.popover = popoverHintSupported() ? 'hint' : 'auto';
52+
element.appendChild(newPopover);
53+
popovers.forEach(popover => assert_equals(popover.matches(':popover-open'),popover.dataset.stayOpen==='true','Adding another popover shouldn\'t change anything'));
54+
assert_true(showing(),'top layer element should still be top layer');
55+
newPopover.showPopover();
56+
assert_true(newPopover.matches(':popover-open'));
57+
popovers.forEach(popover => assert_equals(popover.matches(':popover-open'),popover.dataset.stayOpen==='true','Showing the popover shouldn\'t change anything'));
58+
assert_true(showing(),'top layer element should still be top layer');
59+
},`${description} with ${topLayerType}`);
60+
61+
promise_test(async t => {
62+
const {element,show,showing} = createTopLayerElement(t,topLayerType);
63+
element.popover = popoverHintSupported() ? 'hint' : 'auto';
64+
target.appendChild(element);
65+
66+
// Show the popovers.
67+
t.add_cleanup(() => popovers.forEach(popover => popover.hidePopover()));
68+
popovers.forEach(popover => popover.showPopover());
69+
popovers.forEach(popover => assert_true(popover.matches(':popover-open'),'All popovers should be open'));
70+
const targetWasOpenPopover = target.matches(':popover-open');
71+
72+
// Show the top layer element as a popover first.
73+
element.showPopover();
74+
assert_true(element.matches(':popover-open'),'element should be open as a popover');
75+
assert_equals(target.matches(':popover-open'),targetWasOpenPopover,'target shouldn\'t change popover state');
76+
77+
try {
78+
await show(element);
79+
assert_unreached('It is an error to activate a top layer element that is already a showing popover');
80+
} catch (e) {
81+
// We expect an InvalidStateError for dialogs, and a TypeError for fullscreens.
82+
// Anything else should fall through to the test harness.
83+
if (e.name !== 'InvalidStateError' && e.name !== 'TypeError') {
84+
throw e;
85+
}
86+
}
87+
},`${description} with ${topLayerType}, top layer element *is* a popover`);
88+
89+
if (testAnchorAttribute) {
90+
promise_test(async t => {
91+
const {element,show,showing} = createTopLayerElement(t,topLayerType);
92+
element.anchorElement = target;
93+
document.body.appendChild(element);
94+
95+
// Show the popovers.
96+
t.add_cleanup(() => popovers.forEach(popover => popover.hidePopover()));
97+
popovers.forEach(popover => popover.showPopover());
98+
popovers.forEach(popover => assert_true(popover.matches(':popover-open'),'All popovers should be open'));
99+
100+
// Activate the top layer element.
101+
await show(popovers[popovers.length-1]);
102+
assert_true(showing());
103+
popovers.forEach(popover => assert_equals(popover.matches(':popover-open'),popover.dataset.stayOpen==='true','Incorrect behavior'));
104+
},`${description} with ${topLayerType}, anchor attribute`);
105+
}
106+
});
107+
});
108+
}

0 commit comments

Comments
 (0)