Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 6f1654b

Browse files
mfreed7moz-wptsync-bot
mfreed7
authored and
moz-wptsync-bot
committedFeb 28, 2025
Bug 1949299 [wpt PR 50813] - Handle the open dialogs list and close watcher when removed, a=testonly
Automatic update from web-platform-tests Handle the open dialogs list and close watcher when removed See more discussion here: whatwg/html#10954 In particular, this comment should summarize the end-state after this CL lands: whatwg/html#10954 (comment) Essentially, the spec says to remove a dialog from the open dialogs list when the dialog is removed from the document: https://html.spec.whatwg.org/multipage/interactive-elements.html#dom-dialog-requestclose:~:text=Remove%20removedNode%20from%20removedNode%27s%20node%20document%27s%20open%20dialogs%20list But it does not say what to do when a dialog is inserted into the document that has the `open` attribute, however. It also doesn't correctly handle the distinction between connected and disconnected dialogs. This CL adds insertion steps that re-add dialogs to the open dialogs list, and re-create the close watcher. Plus tests. Lots of tests. Bug: 376516550 Change-Id: Ie70d63287e26c4e5f4a0a200205f78e914e6a220 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6259116 Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Auto-Submit: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1422181} -- wpt-commits: 68c405e14856d3a9f9264adc8b3df6de42af3fe3 wpt-pr: 50813
1 parent e51b2b3 commit 6f1654b

File tree

3 files changed

+275
-81
lines changed

3 files changed

+275
-81
lines changed
 

‎testing/web-platform/tests/html/semantics/interactive-elements/the-dialog-element/dialog-closedby-corner-cases.html

+154
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
<!doctype html>
2+
<meta charset="utf-8">
3+
<meta name="timeout" content="long">
4+
<link rel=help href="https://html.spec.whatwg.org/multipage/interactive-elements.html#dialog-light-dismiss">
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="../../popovers/resources/popover-utils.js"></script>
11+
12+
<button id="outside">Outside</button>
13+
<dialog id="dialog" closedby="any"></dialog>
14+
15+
<script>
16+
const dialog = document.getElementById('dialog');
17+
async function openDialog(openMethod) {
18+
dialog.close();
19+
if (!dialog.open) {
20+
assert_false(dialog.matches(':open'),'Should be closed to start');
21+
switch (openMethod) {
22+
case 'modeless' :
23+
dialog.show();
24+
break;
25+
case 'modal' :
26+
dialog.showModal();
27+
break;
28+
case 'open' :
29+
dialog.open = true;
30+
break;
31+
default:
32+
assert_unreached('Invalid open method');
33+
}
34+
}
35+
await waitForRender();
36+
assert_true(dialog.open,'Should be open now');
37+
assert_true(dialog.matches(':open'),'Should be open now (pseudo)');
38+
}
39+
40+
const changeMethods = [
41+
{
42+
description: 'focusin removes and reinserts',
43+
setup: (openMethod,signal) => {
44+
document.body.addEventListener('focusin',(e) => {
45+
if (!dialog.contains(e.target)) {
46+
const position = dialog.nextElementSibling;
47+
dialog.remove();
48+
document.body.insertBefore(dialog,position);
49+
}
50+
}, {signal});
51+
}
52+
},
53+
{
54+
description: 'focusin closes dialog',
55+
setup: (openMethod,signal) => {
56+
document.body.addEventListener('focusin',(e) => {
57+
if (!dialog.contains(e.target)) {
58+
dialog.close();
59+
}
60+
}, {signal});
61+
}
62+
},
63+
{
64+
description: 'focusin calls showModal',
65+
setup: (openMethod,signal) => {
66+
document.body.addEventListener('focusin',(e) => {
67+
if (!dialog.contains(e.target)) {
68+
try {
69+
dialog.showModal();
70+
} catch {}
71+
}
72+
}, {signal});
73+
// Since the closing steps will trigger another call to showModal
74+
// in this case, before we're done with closing, we should expect
75+
// that the ESC/light dismiss still results in a showing modal, if it
76+
// was originally modal.
77+
return openMethod !== 'modal';
78+
}
79+
},
80+
{
81+
description: 'requestIdleCallback calls showModal',
82+
setup: (openMethod,signal) => {
83+
requestIdleCallback(() => {
84+
try {
85+
dialog.showModal();
86+
} catch {}
87+
});
88+
}
89+
},
90+
{
91+
description: 'beforetoggle closes dialog',
92+
setup: (openMethod,signal) => {
93+
dialog.addEventListener('beforetoggle',() => dialog.close(), {signal});
94+
}
95+
},
96+
{
97+
description: 'beforetoggle calls showModal',
98+
setup: (openMethod,signal) => {
99+
let stackProtector = 0;
100+
dialog.addEventListener('beforetoggle',() => {
101+
if (++stackProtector > 20) {
102+
return;
103+
}
104+
try {
105+
dialog.showModal();
106+
} catch {}
107+
}, {signal});
108+
}
109+
},
110+
];
111+
112+
function runTest(openMethod, changeMethod) {
113+
promise_test(async (t) => {
114+
assert_false(dialog.open,'setup');
115+
assert_false(dialog.matches(':open'));
116+
117+
const controller = new AbortController();
118+
t.add_cleanup(() => {
119+
controller.abort();
120+
dialog.close();
121+
});
122+
const expectResponds = changeMethod.setup(openMethod,controller.signal) ?? true;
123+
124+
// Open the dialog
125+
await openDialog(openMethod);
126+
127+
// Try hitting ESC
128+
const ESC = '\uE00C';
129+
await test_driver.send_keys(document.documentElement,ESC);
130+
await waitForRender();
131+
const respondsToEsc = !dialog.open;
132+
assert_equals(!dialog.matches(':open'),respondsToEsc,':open should match dialog.open');
133+
dialog.close();
134+
135+
// Try clicking outside
136+
await openDialog(openMethod);
137+
await clickOn(outside);
138+
const respondsToLightDismiss = !dialog.open;
139+
assert_equals(!dialog.matches(':open'),respondsToLightDismiss,':open should match dialog.open');
140+
dialog.close();
141+
142+
// See if expectations match
143+
assert_equals(respondsToEsc,expectResponds,`Dialog ${expectResponds ? "should" : "should NOT"} respond to ESC`);
144+
assert_equals(respondsToLightDismiss,expectResponds,`Dialog ${expectResponds ? "should" : "should NOT"} respond to light dismiss`);
145+
}, `${changeMethod.description}, ${openMethod}`);
146+
}
147+
148+
// Run tests
149+
for(openMethod of ['modeless','modal','open']) {
150+
for (const changeMethod of changeMethods) {
151+
runTest(openMethod, changeMethod);
152+
}
153+
}
154+
</script>

‎testing/web-platform/tests/html/semantics/interactive-elements/the-dialog-element/dialog-closedby-start-open.html

+18-3
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,24 @@
3636

3737
<script>
3838
promise_test(async (t) => {
39-
// This test case is pulled from `dialog-closewatcher-crash.html`.
39+
// This test case is pulled from `dialog-closewatcher-crash.html`. It is
40+
// constructed such that this happens:
41+
// 1. The dialog `open` attribute is removed, which (depending on whether
42+
// https://github.com/whatwg/html/pull/10124 behavior is happening) calls
43+
// the close() steps.
44+
// 2. the last step of close() is to restore focus to the previously-focused
45+
// element.
46+
// 3. Changing focus triggers the `focusin` event, which calls `showModal()`.
47+
// 4. In showModal(), the dialog is again made modal, re-constructing the
48+
// close watcher and re-setting the `open` attribute.
49+
// 5. The call to close() ends.
50+
// After all of this, if things are working, the ESC key should still cause
51+
// the dialog to be closed.
4052
const dialog = document.querySelector('dialog#test2');
53+
const controller = new AbortController();
4154
document.querySelector('dl').addEventListener("focusin", () => {
4255
dialog.showModal();
43-
});
56+
},{signal:controller.signal});
4457
// This will trigger the focus-the-previous-element behavior, which will fire
4558
// the `focusin` event.
4659
dialog.open = false;
@@ -53,7 +66,9 @@
5366
});
5467
assert_true(dialog.open);
5568
assert_true(dialog.matches(':open'));
56-
await new test_driver.send_keys(document.documentElement,ESC);
69+
// Stop re-running showModal(), so we can check that the dialog closes with ESC:
70+
controller.abort();
71+
await test_driver.send_keys(document.documentElement,ESC);
5772
assert_false(dialog.open,'ESC should still work');
5873
assert_false(dialog.matches(':open'));
5974
}, `Opening and closing a dialog during the dialog focus fixup should still leave closedby functional`);

‎testing/web-platform/tests/html/semantics/interactive-elements/the-dialog-element/dialog-closedby.html

+103-78
Original file line numberDiff line numberDiff line change
@@ -12,103 +12,128 @@
1212
<button id="outside">Outside</button>
1313

1414
<!-- test cases: -->
15+
16+
<!-- normal cases: -->
1517
<dialog closedby="any" data-behavior="any"></dialog>
1618
<dialog closedby="closerequest" data-behavior="closerequest"></dialog>
1719
<dialog closedby="none" data-behavior="none"></dialog>
1820

21+
<!-- case sensitivity: -->
1922
<dialog closedby="AnY" data-behavior="any"></dialog>
2023
<dialog closedby="ClOsErEqUeSt" data-behavior="closerequest"></dialog>
2124
<dialog closedby="NoNe" data-behavior="none"></dialog>
2225

26+
<!-- invalid value, no value, missing attribute: -->
2327
<dialog closedby="invalid" data-behavior="auto"></dialog>
2428
<dialog closedby data-behavior="auto"></dialog>
2529
<dialog data-behavior="auto"></dialog>
2630

2731
<script>
28-
function openDialog(dialog,modal) {
29-
assert_false(dialog.open);
30-
assert_false(dialog.matches(':open'));
31-
if (modal) {
32-
dialog.showModal();
33-
} else {
34-
dialog.show();
32+
async function openDialog(dialog,openMethod) {
33+
assert_false(dialog.open,'Should be closed to start');
34+
assert_equals(dialog.matches(':open'),dialog.open,':open should match .open');
35+
switch (openMethod) {
36+
case 'modeless' :
37+
dialog.show();
38+
break;
39+
case 'modal' :
40+
dialog.showModal();
41+
break;
42+
case 'open' :
43+
dialog.open = true;
44+
break;
45+
default:
46+
assert_unreached('Invalid open method');
3547
}
36-
assert_true(dialog.open);
37-
assert_true(dialog.matches(':open'));
38-
assert_equals(dialog.matches(':modal'),modal);
48+
await waitForRender();
49+
assert_true(dialog.open,'Should be open now');
50+
assert_equals(dialog.matches(':open'),dialog.open,':open should match .open');
3951
}
40-
function runTest(dialog) {
41-
for(modal of [false,true]) {
42-
promise_test(async (t) => {
43-
assert_false(dialog.open);
44-
assert_false(dialog.matches(':open'));
45-
t.add_cleanup(() => dialog.close());
46-
// Try hitting ESC
47-
openDialog(dialog,modal);
48-
const closedByReflectionWhileOpen = dialog.closedBy;
49-
const ESC = '\uE00C';
50-
await new test_driver.send_keys(document.documentElement,ESC);
51-
const respondsToEsc = !dialog.open;
52-
const respondsToEsc2 = !dialog.matches(':open');
53-
dialog.close();
54-
// Try clicking outside
55-
openDialog(dialog,modal);
56-
await clickOn(outside);
57-
const respondsToLightDismiss = !dialog.open;
58-
const respondsToLightDismiss2 = !dialog.matches(':open');
59-
dialog.close();
60-
// See if expectations match
61-
let expectedReflectionWhileOpen = dialog.dataset.behavior;
62-
let expectedReflectionWhileClosed = dialog.dataset.behavior;
63-
switch (dialog.dataset.behavior) {
64-
case 'any':
65-
assert_true(respondsToEsc,'Dialog should respond to ESC');
66-
assert_true(respondsToEsc2,'Dialog should respond to ESC (:open)');
67-
assert_true(respondsToLightDismiss,'Dialog should respond to light dismiss');
68-
assert_true(respondsToLightDismiss2,'Dialog should respond to light dismiss (:open)');
69-
break;
70-
case 'closerequest':
71-
assert_true(respondsToEsc,'Dialog should respond to ESC');
72-
assert_true(respondsToEsc2,'Dialog should respond to ESC (:open)');
73-
assert_false(respondsToLightDismiss,'Dialog should NOT respond to light dismiss');
74-
assert_false(respondsToLightDismiss2,'Dialog should NOT respond to light dismiss (:open)');
75-
break;
76-
case 'none':
77-
assert_false(respondsToEsc,'Dialog should NOT respond to ESC');
78-
assert_false(respondsToEsc2,'Dialog should NOT respond to ESC (:open)');
79-
assert_false(respondsToLightDismiss,'Dialog should NOT respond to light dismiss');
80-
assert_false(respondsToLightDismiss2,'Dialog should NOT respond to light dismiss (:open)');
81-
break;
82-
case 'auto':
83-
if (modal) {
84-
assert_true(respondsToEsc,'Modal dialog in auto state should respond to ESC');
85-
assert_false(respondsToLightDismiss,'Modal dialog in auto state should NOT respond to light dismiss');
86-
expectedReflectionWhileOpen = 'closerequest';
87-
} else {
88-
assert_false(respondsToEsc,'Non-modal dialog in auto state should NOT respond to ESC');
89-
assert_false(respondsToLightDismiss,'Non-modal dialog in auto state should NOT respond to light dismiss');
90-
expectedReflectionWhileOpen = 'none';
91-
}
92-
expectedReflectionWhileClosed = 'none';
93-
break;
94-
default:
95-
assert_unreached('Invalid expectation');
52+
53+
function getDefaultExpectations(behavior, openMethod) {
54+
switch (behavior) {
55+
case 'any':
56+
return {
57+
respondsToEsc: true,
58+
respondsToLightDismiss: true,
59+
expectedReflectionWhileOpen: behavior,
60+
expectedReflectionWhileClosed: behavior,
61+
};
62+
case 'closerequest':
63+
return {
64+
respondsToEsc: true,
65+
respondsToLightDismiss: false,
66+
expectedReflectionWhileOpen: behavior,
67+
expectedReflectionWhileClosed: behavior,
68+
};
69+
case 'none':
70+
return {
71+
respondsToEsc: false,
72+
respondsToLightDismiss: false,
73+
expectedReflectionWhileOpen: behavior,
74+
expectedReflectionWhileClosed: behavior,
75+
};
76+
case 'auto':
77+
if (openMethod === 'modal') {
78+
return {
79+
respondsToEsc: true,
80+
respondsToLightDismiss: false,
81+
expectedReflectionWhileOpen: 'closerequest',
82+
expectedReflectionWhileClosed: 'none',
83+
};
84+
} else {
85+
return {
86+
respondsToEsc: false,
87+
respondsToLightDismiss: false,
88+
expectedReflectionWhileOpen: 'none',
89+
expectedReflectionWhileClosed: 'none',
90+
};
9691
}
97-
// Check reflection
98-
assert_equals(closedByReflectionWhileOpen,expectedReflectionWhileOpen,'Reflection should be limited to known values (open)');
99-
assert_equals(dialog.closedBy,expectedReflectionWhileClosed,'Reflection should be limited to known values (closed)');
100-
}, `closedby=${dialog.getAttribute('closedby')}, ${modal ? 'Modal' : 'Non-modal'}`);
92+
default:
93+
assert_unreached('Invalid expectation');
10194
}
10295
}
10396

104-
// Add close button, in case of manual testing
105-
const testDialogs = document.querySelectorAll('dialog');
106-
testDialogs.forEach(dialog => {
107-
const button = dialog.appendChild(document.createElement('button'));
108-
button.innerText = 'Close';
109-
button.addEventListener('click',() => dialog.close());
110-
});
97+
function runTest(dialog, openMethod) {
98+
promise_test(async (t) => {
99+
assert_false(dialog.open,'setup');
100+
assert_false(dialog.matches(':open'));
101+
t.add_cleanup(() => dialog.close());
102+
103+
// Open the dialog
104+
await openDialog(dialog,openMethod);
105+
assert_equals(dialog.matches(':modal'),openMethod === 'modal',':modal incorrect');
106+
const closedByReflectionWhileOpen = dialog.closedBy;
107+
108+
// Try hitting ESC
109+
const ESC = '\uE00C';
110+
await test_driver.send_keys(document.documentElement,ESC);
111+
await waitForRender();
112+
const respondsToEsc = !dialog.open;
113+
assert_equals(!dialog.matches(':open'),respondsToEsc,':open should match dialog.open');
114+
dialog.close();
115+
116+
// Try clicking outside
117+
await openDialog(dialog,openMethod);
118+
assert_equals(dialog.matches(':modal'),openMethod === 'modal',':modal incorrect');
119+
await clickOn(outside);
120+
const respondsToLightDismiss = !dialog.open;
121+
assert_equals(!dialog.matches(':open'),respondsToLightDismiss,':open should match dialog.open');
122+
dialog.close();
123+
124+
// See if expectations match
125+
let expectations = getDefaultExpectations(dialog.dataset.behavior, openMethod);
126+
assert_equals(respondsToEsc,expectations.respondsToEsc,`Dialog ${expectations.respondsToEsc ? "should" : "should NOT"} respond to ESC`);
127+
assert_equals(respondsToLightDismiss,expectations.respondsToLightDismiss,`Dialog ${expectations.respondsToLightDismiss ? "should" : "should NOT"} respond to light dismiss`);
128+
assert_equals(closedByReflectionWhileOpen,expectations.expectedReflectionWhileOpen,'Reflection should be limited to known values (open)');
129+
assert_equals(dialog.closedBy,expectations.expectedReflectionWhileClosed,'Reflection should be limited to known values (closed)');
130+
}, `closedby=${dialog.getAttribute('closedby')}, ${openMethod}`);
131+
}
111132

112133
// Run tests
113-
testDialogs.forEach(runTest);
134+
document.querySelectorAll('dialog').forEach((dialog) => {
135+
for(openMethod of ['modeless','modal','open']) {
136+
runTest(dialog, openMethod);
137+
}
138+
});
114139
</script>

0 commit comments

Comments
 (0)
Please sign in to comment.