[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
rwlbuis · May 11, 2023 · 1c9711b · 1c9711b
1 parent e911b9b
commit 1c9711b
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 36 deletions.
diff --git a/...tics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state-expected.txt b/...tics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state-expected.txt
@@ -0,0 +1,4 @@
+hello
+
+PASS dialog-no-throw-requested-state
+
diff --git a/...ml/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html b/...ml/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<link rel=author href="mailto:[email protected]">
+<link rel=help href="https://github.com/whatwg/html/pull/9142">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<dialog>hello</dialog>
+<script>
+test(() => {
+  const dialog = document.querySelector('dialog');
+  // calling close() on a dialog that is already closed should not throw.
+  dialog.close();
+  dialog.show();
+  // calling show() on a dialog that is already showing non-modal should not throw.
+  dialog.show();
+  assert_throws_dom('InvalidStateError', () => dialog.showModal(),
+    'Calling showModal() on a dialog that is already showing non-modal should throw.');
+  dialog.close();
+  dialog.showModal();
+  assert_throws_dom('InvalidStateError', () => dialog.show(),
+    'Calling show() on a dialog that is already showing modal should throw.');
+  // calling showModal() on a dialog that is already showing modal should not throw.
+  dialog.showModal();
+});
+</script>
diff --git a/...ests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-attribute-basic.html b/...ests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-attribute-basic.html
@@ -254,10 +254,10 @@
     },{once: true});
     assert_true(popover.matches(':popover-open'));
     assert_true(other_popover.matches(':popover-open'));
-    assert_throws_dom('InvalidStateError', () => popover.hidePopover());
+    popover.hidePopover();
     assert_false(other_popover.matches(':popover-open'),'unrelated popover is hidden');
     assert_false(popover.matches(':popover-open'),'popover is still hidden if its type changed during hide event');
-    assert_throws_dom("InvalidStateError",() => other_popover.hidePopover(),'Nested popover should already be hidden');
+    other_popover.hidePopover();
   },`Changing the popover type in a "beforetoggle" event handler should throw an exception (during hidePopover())`);
 
   function interpretedType(typeString,method) {

diff --git a/...tTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss.html b/...tTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss.html
@@ -531,7 +531,7 @@
     p14.hidePopover();
   },{once:true});
   assert_true(p13.matches(':popover-open') && p14.matches(':popover-open') && p15.matches(':popover-open'),'all three should be open');
-  assert_throws_dom('InvalidStateError',() => p14.hidePopover(),'should throw because the event listener has already hidden the popover');
+  p14.hidePopover();
   assert_true(p13.matches(':popover-open'),'p13 should still be open');
   assert_false(p14.matches(':popover-open'));
   assert_false(p15.matches(':popover-open'));

diff --git a/...Tests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents.html b/...Tests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents.html
@@ -27,10 +27,7 @@
   assert_true(p2.matches(':popover-open'),
     'The popover should be open after calling showPopover()');
 
-  // Because the `beforetoggle` handler changes the document,
-  // and that action closes the popover, the call to hidePopover()
-  // will result in an exception.
-  assert_throws_dom('InvalidStateError',() => p2.hidePopover());
+  p2.hidePopover();
   assert_false(p2.matches(':popover-open'),
     'The popover should be closed after moving it between documents.');
 }, 'Moving popovers between documents while hiding should not throw an exception.');

diff --git a/...tTests/imported/w3c/web-platform-tests/html/semantics/popovers/resources/popover-utils.js b/...tTests/imported/w3c/web-platform-tests/html/semantics/popovers/resources/popover-utils.js
@@ -13,7 +13,7 @@ async function clickOn(element) {
 async function sendTab() {
   await waitForRender();
   const kTab = '\uE004';
-  await new test_driver.send_keys(document.body,kTab);
+  await new test_driver.send_keys(document.documentElement,kTab);
   await waitForRender();
 }
 // Waiting for crbug.com/893480:
@@ -31,12 +31,12 @@ async function sendTab() {
 // }
 async function sendEscape() {
   await waitForRender();
-  await new test_driver.send_keys(document.body,'\uE00C'); // Escape
+  await new test_driver.send_keys(document.documentElement,'\uE00C'); // Escape
   await waitForRender();
 }
 async function sendEnter() {
   await waitForRender();
-  await new test_driver.send_keys(document.body,'\uE007'); // Enter
+  await new test_driver.send_keys(document.documentElement,'\uE007'); // Enter
   await waitForRender();
 }
 function isElementVisible(el) {
@@ -114,7 +114,6 @@ function popoverHintSupported() {
   testElement.popover = 'hint';
   return testElement.popover === 'hint';
 }
-
 function assertPopoverVisibility(popover, isPopover, expectedVisibility, message) {
   const isVisible = isElementVisible(popover);
   assert_equals(isVisible, expectedVisibility,`${message}: Expected this element to be ${expectedVisibility ? "visible" : "not visible"}`);
@@ -127,15 +126,14 @@ function assertPopoverVisibility(popover, isPopover, expectedVisibility, message
     assert_false(popover.matches(':popover-open'),`${message}: Non-showing popovers should *not* match :popover-open`);
   }
 }
-
 function assertIsFunctionalPopover(popover, checkVisibility) {
   assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/false, 'A popover should start out hidden');
   popover.showPopover();
   if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/true, 'After showPopover(), a popover should be visible');
-  assert_throws_dom("InvalidStateError",() => popover.showPopover(),'Calling showPopover on a showing popover should throw InvalidStateError');
+  popover.showPopover(); // Calling showPopover on a showing popover should not throw.
   popover.hidePopover();
   if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/false, 'After hidePopover(), a popover should be hidden');
-  assert_throws_dom("InvalidStateError",() => popover.hidePopover(),'Calling hidePopover on a hidden popover should throw InvalidStateError');
+  popover.hidePopover(); // Calling hidePopover on a hidden popover should not throw.
   popover.togglePopover();
   if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/true, 'After togglePopover() on hidden popover, it should be visible');
   popover.togglePopover();
@@ -151,11 +149,10 @@ function assertIsFunctionalPopover(popover, checkVisibility) {
   const parent = popover.parentElement;
   popover.remove();
   assert_throws_dom("InvalidStateError",() => popover.showPopover(),'Calling showPopover on a disconnected popover should throw InvalidStateError');
-  assert_throws_dom("InvalidStateError",() => popover.hidePopover(),'Calling hidePopover on a disconnected popover should throw InvalidStateError');
+  popover.hidePopover(); // Calling hidePopover on a disconnected popover should not throw.
   assert_throws_dom("InvalidStateError",() => popover.togglePopover(),'Calling hidePopover on a disconnected popover should throw InvalidStateError');
   parent.appendChild(popover);
 }
-
 function assertNotAPopover(nonPopover) {
   // If the non-popover element nonetheless has a 'popover' attribute, it should
   // be invisible. Otherwise, it should be visible.

diff --git a/...mported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt b/...mported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt
@@ -10,11 +10,11 @@ PASS Clicking inside a child popover shouldn't close either popover
 FAIL Clicking inside a parent popover should close child popover assert_false: expected false got true
 PASS Clicking on invoking element, after using it for activation, shouldn't close its popover
 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
-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"
-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"
-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"
-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"
-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"
+PASS Clicking on invoking element, after using it for activation, shouldn't close its popover (nested case, not used for invocation)
+PASS Clicking on invoking element, even if it wasn't used for activation, shouldn't close its popover
+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
+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
+PASS Dragging from an open popover outside an open popover should leave the popover open
 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
 PASS An invoking element that was not used to invoke the popover can still be part of the ancestor chain
 FAIL Scrolling within a popover should not close the popover promise_test: Unhandled rejection with value: object "Error: Unknown source type "wheel"."

diff --git a/Source/WebCore/html/HTMLDialogElement.cpp b/Source/WebCore/html/HTMLDialogElement.cpp
@@ -53,8 +53,11 @@ HTMLDialogElement::HTMLDialogElement(const QualifiedName& tagName, Document& doc
 ExceptionOr<void> HTMLDialogElement::show()
 {
     // If the element already has an open attribute, then return.
-    if (isOpen())
-        return { };
+    if (isOpen()) {
+        if (!isModal())
+            return { };
+        return Exception { InvalidStateError, "Cannot call show() on an open modal dialog."_s };
+    }
 
     if (popoverData() && popoverData()->visibilityState() == PopoverVisibilityState::Showing)
         return Exception { InvalidStateError, "Element is already an open popover."_s };
@@ -72,8 +75,11 @@ ExceptionOr<void> HTMLDialogElement::show()
 ExceptionOr<void> HTMLDialogElement::showModal()
 {
     // If subject already has an open attribute, then throw an "InvalidStateError" DOMException.
-    if (isOpen())
-        return Exception { InvalidStateError, "Element is already open."_s };
+    if (isOpen()) {
+        if (isModal())
+            return { };
+        return Exception { InvalidStateError, "Cannot call showModal() on an open non-modal dialog."_s };
+    }
 
     // If subject is not connected, then throw an "InvalidStateError" DOMException.
     if (!isConnected())

diff --git a/Source/WebCore/html/HTMLElement.cpp b/Source/WebCore/html/HTMLElement.cpp
@@ -1239,20 +1239,20 @@ ExceptionOr<Ref<ElementInternals>> HTMLElement::attachInternals()
     return ElementInternals::create(*this);
 }
 
-static ExceptionOr<void> checkPopoverValidity(HTMLElement& element, PopoverVisibilityState expectedState, Document* expectedDocument = nullptr)
+static ExceptionOr<bool> checkPopoverValidity(HTMLElement& element, PopoverVisibilityState expectedState, Document* expectedDocument = nullptr)
 {
     if (element.popoverState() == PopoverState::None)
         return Exception { NotSupportedError, "Element does not have the popover attribute"_s };
 
+    if (element.popoverData()->visibilityState() != expectedState)
+        return false;
+
     if (!element.isConnected())
         return Exception { InvalidStateError, "Element is not connected"_s };
 
     if (expectedDocument && element.document() != *expectedDocument)
         return Exception { InvalidStateError, "Invalid when the document changes while showing or hiding a popover element"_s };
 
-    if (element.popoverData()->visibilityState() != expectedState)
-        return Exception { InvalidStateError, "Element has unexpected visibility state"_s };
-
     if (is<HTMLDialogElement>(element) && element.hasAttributeWithoutSynchronization(HTMLNames::openAttr))
         return Exception { InvalidStateError, "Element is an open <dialog> element"_s };
 
@@ -1261,7 +1261,7 @@ static ExceptionOr<void> checkPopoverValidity(HTMLElement& element, PopoverVisib
         return Exception { InvalidStateError, "Element is fullscreen"_s };
 #endif
 
-    return { };
+    return true;
 }
 
 // https://html.spec.whatwg.org/#topmost-popover-ancestor
@@ -1375,8 +1375,11 @@ void HTMLElement::queuePopoverToggleEventTask(PopoverVisibilityState oldState, P
 
 ExceptionOr<void> HTMLElement::showPopover()
 {
-    if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden); check.hasException())
+    auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden);
+    if (check.hasException())
         return check.releaseException();
+    if (!check.returnValue())
+        return { };
 
     ASSERT(!isInTopLayer());
 
@@ -1386,8 +1389,11 @@ ExceptionOr<void> HTMLElement::showPopover()
     if (event->defaultPrevented() || event->defaultHandled())
         return { };
 
-    if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr()); check.hasException())
+    check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr());
+    if (check.hasException())
         return check.releaseException();
+    if (!check.returnValue())
+        return { };
 
     ASSERT(popoverData());
 
@@ -1399,8 +1405,11 @@ ExceptionOr<void> HTMLElement::showPopover()
         if (popoverState() != originalState)
             return Exception { InvalidStateError, "The value of the popover attribute was changed while hiding the popover."_s };
 
-        if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr()); check.hasException())
+        check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr());
+        if (check.hasException())
             return check.releaseException();
+        if (!check.returnValue())
+            return { };
     }
 
     bool shouldRestoreFocus = !document->topmostAutoPopover();
@@ -1425,25 +1434,35 @@ ExceptionOr<void> HTMLElement::showPopover()
 
 ExceptionOr<void> HTMLElement::hidePopoverInternal(FocusPreviousElement focusPreviousElement, FireEvents fireEvents)
 {
-    if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Showing); check.hasException())
+    auto check = checkPopoverValidity(*this, PopoverVisibilityState::Showing);
+    if (check.hasException())
         return check.releaseException();
+    if (!check.returnValue())
+        return { };
+
 
     ASSERT(popoverData());
 
     if (popoverState() == PopoverState::Auto) {
         document().hideAllPopoversUntil(this, focusPreviousElement, fireEvents);
 
-        if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Showing); check.hasException())
+        check = checkPopoverValidity(*this, PopoverVisibilityState::Showing);
+        if (check.hasException())
             return check.releaseException();
+        if (!check.returnValue())
+            return { };
     }
 
     popoverData()->setInvoker(nullptr);
 
     if (fireEvents == FireEvents::Yes)
         dispatchEvent(ToggleEvent::create(eventNames().beforetoggleEvent, { EventInit { }, "open"_s, "closed"_s }, Event::IsCancelable::No));
 
-    if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Showing); check.hasException())
+    check = checkPopoverValidity(*this, PopoverVisibilityState::Showing);
+    if (check.hasException())
         return check.releaseException();
+    if (!check.returnValue())
+        return { };
 
     ASSERT(popoverData());