Skip to content

Commit 1c9711b

Browse files
rwlbuisrwlbuis
committed
[popover] Don't throw when popover/dialog is in requested state
https://bugs.webkit.org/show_bug.cgi?id=255879 Reviewed by Tim Nguyen. Implement the spec changes from: whatwg/html#9142 The dialog related changes are tested by dialog-no-throw-requested-state.html. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-attribute-basic.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/resources/popover-utils.js: (async sendTab): (async sendEscape): (async sendEnter): (assertPopoverVisibility): * LayoutTests/platform/ios/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt: * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::show): (WebCore::HTMLDialogElement::showModal): * Source/WebCore/html/HTMLElement.cpp: (WebCore::checkPopoverValidity): (WebCore::HTMLElement::showPopover): (WebCore::HTMLElement::hidePopoverInternal): Canonical link: https://commits.webkit.org/263957@main
1 parent e911b9b commit 1c9711b

9 files changed

Lines changed: 83 additions & 36 deletions

File tree

‎LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state-expected.txt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
hello
2+
3+
PASS dialog-no-throw-requested-state
4+

‎LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<!DOCTYPE html>
2+
<link rel=author href="mailto:jarhar@chromium.org">
3+
<link rel=help href="https://github.com/whatwg/html/pull/9142">
4+
<script src="/resources/testharness.js"></script>
5+
<script src="/resources/testharnessreport.js"></script>
6+
<dialog>hello</dialog>
7+
<script>
8+
test(() => {
9+
const dialog = document.querySelector('dialog');
10+
// calling close() on a dialog that is already closed should not throw.
11+
dialog.close();
12+
dialog.show();
13+
// calling show() on a dialog that is already showing non-modal should not throw.
14+
dialog.show();
15+
assert_throws_dom('InvalidStateError', () => dialog.showModal(),
16+
'Calling showModal() on a dialog that is already showing non-modal should throw.');
17+
dialog.close();
18+
dialog.showModal();
19+
assert_throws_dom('InvalidStateError', () => dialog.show(),
20+
'Calling show() on a dialog that is already showing modal should throw.');
21+
// calling showModal() on a dialog that is already showing modal should not throw.
22+
dialog.showModal();
23+
});
24+
</script>

‎LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-attribute-basic.html‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,10 +254,10 @@
254254
},{once: true});
255255
assert_true(popover.matches(':popover-open'));
256256
assert_true(other_popover.matches(':popover-open'));
257-
assert_throws_dom('InvalidStateError', () => popover.hidePopover());
257+
popover.hidePopover();
258258
assert_false(other_popover.matches(':popover-open'),'unrelated popover is hidden');
259259
assert_false(popover.matches(':popover-open'),'popover is still hidden if its type changed during hide event');
260-
assert_throws_dom("InvalidStateError",() => other_popover.hidePopover(),'Nested popover should already be hidden');
260+
other_popover.hidePopover();
261261
},`Changing the popover type in a "beforetoggle" event handler should throw an exception (during hidePopover())`);
262262

263263
function interpretedType(typeString,method) {

‎LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@
531531
p14.hidePopover();
532532
},{once:true});
533533
assert_true(p13.matches(':popover-open') && p14.matches(':popover-open') && p15.matches(':popover-open'),'all three should be open');
534-
assert_throws_dom('InvalidStateError',() => p14.hidePopover(),'should throw because the event listener has already hidden the popover');
534+
p14.hidePopover();
535535
assert_true(p13.matches(':popover-open'),'p13 should still be open');
536536
assert_false(p14.matches(':popover-open'));
537537
assert_false(p15.matches(':popover-open'));

‎LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents.html‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,7 @@
2727
assert_true(p2.matches(':popover-open'),
2828
'The popover should be open after calling showPopover()');
2929

30-
// Because the `beforetoggle` handler changes the document,
31-
// and that action closes the popover, the call to hidePopover()
32-
// will result in an exception.
33-
assert_throws_dom('InvalidStateError',() => p2.hidePopover());
30+
p2.hidePopover();
3431
assert_false(p2.matches(':popover-open'),
3532
'The popover should be closed after moving it between documents.');
3633
}, 'Moving popovers between documents while hiding should not throw an exception.');

‎LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/resources/popover-utils.js‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ async function clickOn(element) {
1313
async function sendTab() {
1414
await waitForRender();
1515
const kTab = '\uE004';
16-
await new test_driver.send_keys(document.body,kTab);
16+
await new test_driver.send_keys(document.documentElement,kTab);
1717
await waitForRender();
1818
}
1919
// Waiting for crbug.com/893480:
@@ -31,12 +31,12 @@ async function sendTab() {
3131
// }
3232
async function sendEscape() {
3333
await waitForRender();
34-
await new test_driver.send_keys(document.body,'\uE00C'); // Escape
34+
await new test_driver.send_keys(document.documentElement,'\uE00C'); // Escape
3535
await waitForRender();
3636
}
3737
async function sendEnter() {
3838
await waitForRender();
39-
await new test_driver.send_keys(document.body,'\uE007'); // Enter
39+
await new test_driver.send_keys(document.documentElement,'\uE007'); // Enter
4040
await waitForRender();
4141
}
4242
function isElementVisible(el) {
@@ -114,7 +114,6 @@ function popoverHintSupported() {
114114
testElement.popover = 'hint';
115115
return testElement.popover === 'hint';
116116
}
117-
118117
function assertPopoverVisibility(popover, isPopover, expectedVisibility, message) {
119118
const isVisible = isElementVisible(popover);
120119
assert_equals(isVisible, expectedVisibility,`${message}: Expected this element to be ${expectedVisibility ? "visible" : "not visible"}`);
@@ -127,15 +126,14 @@ function assertPopoverVisibility(popover, isPopover, expectedVisibility, message
127126
assert_false(popover.matches(':popover-open'),`${message}: Non-showing popovers should *not* match :popover-open`);
128127
}
129128
}
130-
131129
function assertIsFunctionalPopover(popover, checkVisibility) {
132130
assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/false, 'A popover should start out hidden');
133131
popover.showPopover();
134132
if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/true, 'After showPopover(), a popover should be visible');
135-
assert_throws_dom("InvalidStateError",() => popover.showPopover(),'Calling showPopover on a showing popover should throw InvalidStateError');
133+
popover.showPopover(); // Calling showPopover on a showing popover should not throw.
136134
popover.hidePopover();
137135
if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/false, 'After hidePopover(), a popover should be hidden');
138-
assert_throws_dom("InvalidStateError",() => popover.hidePopover(),'Calling hidePopover on a hidden popover should throw InvalidStateError');
136+
popover.hidePopover(); // Calling hidePopover on a hidden popover should not throw.
139137
popover.togglePopover();
140138
if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/true, 'After togglePopover() on hidden popover, it should be visible');
141139
popover.togglePopover();
@@ -151,11 +149,10 @@ function assertIsFunctionalPopover(popover, checkVisibility) {
151149
const parent = popover.parentElement;
152150
popover.remove();
153151
assert_throws_dom("InvalidStateError",() => popover.showPopover(),'Calling showPopover on a disconnected popover should throw InvalidStateError');
154-
assert_throws_dom("InvalidStateError",() => popover.hidePopover(),'Calling hidePopover on a disconnected popover should throw InvalidStateError');
152+
popover.hidePopover(); // Calling hidePopover on a disconnected popover should not throw.
155153
assert_throws_dom("InvalidStateError",() => popover.togglePopover(),'Calling hidePopover on a disconnected popover should throw InvalidStateError');
156154
parent.appendChild(popover);
157155
}
158-
159156
function assertNotAPopover(nonPopover) {
160157
// If the non-popover element nonetheless has a 'popover' attribute, it should
161158
// be invisible. Otherwise, it should be visible.

‎LayoutTests/platform/ios/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@ PASS Clicking inside a child popover shouldn't close either popover
1010
FAIL Clicking inside a parent popover should close child popover assert_false: expected false got true
1111
PASS Clicking on invoking element, after using it for activation, shouldn't close its popover
1212
FAIL Clicking on invoking element, after using it for activation, shouldn't close its popover (nested case) assert_true: button2 should activate popover2 expected true got false
13-
FAIL Clicking on invoking element, after using it for activation, shouldn't close its popover (nested case, not used for invocation) promise_test: Unhandled rejection with value: object "InvalidStateError: Element has unexpected visibility state"
14-
FAIL Clicking on invoking element, even if it wasn't used for activation, shouldn't close its popover promise_test: Unhandled rejection with value: object "InvalidStateError: Element has unexpected visibility state"
15-
FAIL Clicking on popovertarget element, even if it wasn't used for activation, should hide it exactly once promise_test: Unhandled rejection with value: object "InvalidStateError: Element has unexpected visibility state"
16-
FAIL Clicking on anchor element (that isn't an invoking element) shouldn't prevent its popover from being closed promise_test: Unhandled rejection with value: object "InvalidStateError: Element has unexpected visibility state"
17-
FAIL Dragging from an open popover outside an open popover should leave the popover open promise_test: Unhandled rejection with value: object "InvalidStateError: Element has unexpected visibility state"
13+
PASS Clicking on invoking element, after using it for activation, shouldn't close its popover (nested case, not used for invocation)
14+
PASS Clicking on invoking element, even if it wasn't used for activation, shouldn't close its popover
15+
FAIL Clicking on popovertarget element, even if it wasn't used for activation, should hide it exactly once assert_false: popover1 should be hidden by popovertarget expected false got true
16+
FAIL Clicking on anchor element (that isn't an invoking element) shouldn't prevent its popover from being closed assert_false: popover1 should close expected false got true
17+
PASS Dragging from an open popover outside an open popover should leave the popover open
1818
FAIL A popover inside an invoking element doesn't participate in that invoker's ancestor chain assert_true: invoking element should open popover expected true got false
1919
PASS An invoking element that was not used to invoke the popover can still be part of the ancestor chain
2020
FAIL Scrolling within a popover should not close the popover promise_test: Unhandled rejection with value: object "Error: Unknown source type "wheel"."

‎Source/WebCore/html/HTMLDialogElement.cpp‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,11 @@ HTMLDialogElement::HTMLDialogElement(const QualifiedName& tagName, Document& doc
5353
ExceptionOr<void> HTMLDialogElement::show()
5454
{
5555
// If the element already has an open attribute, then return.
56-
if (isOpen())
57-
return { };
56+
if (isOpen()) {
57+
if (!isModal())
58+
return { };
59+
return Exception { InvalidStateError, "Cannot call show() on an open modal dialog."_s };
60+
}
5861

5962
if (popoverData() && popoverData()->visibilityState() == PopoverVisibilityState::Showing)
6063
return Exception { InvalidStateError, "Element is already an open popover."_s };
@@ -72,8 +75,11 @@ ExceptionOr<void> HTMLDialogElement::show()
7275
ExceptionOr<void> HTMLDialogElement::showModal()
7376
{
7477
// If subject already has an open attribute, then throw an "InvalidStateError" DOMException.
75-
if (isOpen())
76-
return Exception { InvalidStateError, "Element is already open."_s };
78+
if (isOpen()) {
79+
if (isModal())
80+
return { };
81+
return Exception { InvalidStateError, "Cannot call showModal() on an open non-modal dialog."_s };
82+
}
7783

7884
// If subject is not connected, then throw an "InvalidStateError" DOMException.
7985
if (!isConnected())

‎Source/WebCore/html/HTMLElement.cpp‎

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1239,20 +1239,20 @@ ExceptionOr<Ref<ElementInternals>> HTMLElement::attachInternals()
12391239
return ElementInternals::create(*this);
12401240
}
12411241

1242-
static ExceptionOr<void> checkPopoverValidity(HTMLElement& element, PopoverVisibilityState expectedState, Document* expectedDocument = nullptr)
1242+
static ExceptionOr<bool> checkPopoverValidity(HTMLElement& element, PopoverVisibilityState expectedState, Document* expectedDocument = nullptr)
12431243
{
12441244
if (element.popoverState() == PopoverState::None)
12451245
return Exception { NotSupportedError, "Element does not have the popover attribute"_s };
12461246

1247+
if (element.popoverData()->visibilityState() != expectedState)
1248+
return false;
1249+
12471250
if (!element.isConnected())
12481251
return Exception { InvalidStateError, "Element is not connected"_s };
12491252

12501253
if (expectedDocument && element.document() != *expectedDocument)
12511254
return Exception { InvalidStateError, "Invalid when the document changes while showing or hiding a popover element"_s };
12521255

1253-
if (element.popoverData()->visibilityState() != expectedState)
1254-
return Exception { InvalidStateError, "Element has unexpected visibility state"_s };
1255-
12561256
if (is<HTMLDialogElement>(element) && element.hasAttributeWithoutSynchronization(HTMLNames::openAttr))
12571257
return Exception { InvalidStateError, "Element is an open <dialog> element"_s };
12581258

@@ -1261,7 +1261,7 @@ static ExceptionOr<void> checkPopoverValidity(HTMLElement& element, PopoverVisib
12611261
return Exception { InvalidStateError, "Element is fullscreen"_s };
12621262
#endif
12631263

1264-
return { };
1264+
return true;
12651265
}
12661266

12671267
// https://html.spec.whatwg.org/#topmost-popover-ancestor
@@ -1375,8 +1375,11 @@ void HTMLElement::queuePopoverToggleEventTask(PopoverVisibilityState oldState, P
13751375

13761376
ExceptionOr<void> HTMLElement::showPopover()
13771377
{
1378-
if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden); check.hasException())
1378+
auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden);
1379+
if (check.hasException())
13791380
return check.releaseException();
1381+
if (!check.returnValue())
1382+
return { };
13801383

13811384
ASSERT(!isInTopLayer());
13821385

@@ -1386,8 +1389,11 @@ ExceptionOr<void> HTMLElement::showPopover()
13861389
if (event->defaultPrevented() || event->defaultHandled())
13871390
return { };
13881391

1389-
if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr()); check.hasException())
1392+
check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr());
1393+
if (check.hasException())
13901394
return check.releaseException();
1395+
if (!check.returnValue())
1396+
return { };
13911397

13921398
ASSERT(popoverData());
13931399

@@ -1399,8 +1405,11 @@ ExceptionOr<void> HTMLElement::showPopover()
13991405
if (popoverState() != originalState)
14001406
return Exception { InvalidStateError, "The value of the popover attribute was changed while hiding the popover."_s };
14011407

1402-
if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr()); check.hasException())
1408+
check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr());
1409+
if (check.hasException())
14031410
return check.releaseException();
1411+
if (!check.returnValue())
1412+
return { };
14041413
}
14051414

14061415
bool shouldRestoreFocus = !document->topmostAutoPopover();
@@ -1425,25 +1434,35 @@ ExceptionOr<void> HTMLElement::showPopover()
14251434

14261435
ExceptionOr<void> HTMLElement::hidePopoverInternal(FocusPreviousElement focusPreviousElement, FireEvents fireEvents)
14271436
{
1428-
if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Showing); check.hasException())
1437+
auto check = checkPopoverValidity(*this, PopoverVisibilityState::Showing);
1438+
if (check.hasException())
14291439
return check.releaseException();
1440+
if (!check.returnValue())
1441+
return { };
1442+
14301443

14311444
ASSERT(popoverData());
14321445

14331446
if (popoverState() == PopoverState::Auto) {
14341447
document().hideAllPopoversUntil(this, focusPreviousElement, fireEvents);
14351448

1436-
if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Showing); check.hasException())
1449+
check = checkPopoverValidity(*this, PopoverVisibilityState::Showing);
1450+
if (check.hasException())
14371451
return check.releaseException();
1452+
if (!check.returnValue())
1453+
return { };
14381454
}
14391455

14401456
popoverData()->setInvoker(nullptr);
14411457

14421458
if (fireEvents == FireEvents::Yes)
14431459
dispatchEvent(ToggleEvent::create(eventNames().beforetoggleEvent, { EventInit { }, "open"_s, "closed"_s }, Event::IsCancelable::No));
14441460

1445-
if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Showing); check.hasException())
1461+
check = checkPopoverValidity(*this, PopoverVisibilityState::Showing);
1462+
if (check.hasException())
14461463
return check.releaseException();
1464+
if (!check.returnValue())
1465+
return { };
14471466

14481467
ASSERT(popoverData());
14491468

0 commit comments

Comments
 (0)