[popover] Make element.togglePopover() return a boolean

https://bugs.webkit.org/show_bug.cgi?id=257769 Reviewed by Tim Nguyen. Implement togglePopover API change: whatwg/html#9393 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/togglePopover-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/togglePopover.html: Added. * Source/WebCore/html/HTMLElement.cpp: (WebCore::HTMLElement::togglePopover): * Source/WebCore/html/HTMLElement.h: * Source/WebCore/html/HTMLElement.idl: Canonical link: https://commits.webkit.org/265064@main
WebKit · Jun 12, 2023 · 37def53 · 37def53
1 parent ecb76a4
commit 37def53
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 11 deletions.
diff --git a/...tTests/imported/w3c/web-platform-tests/html/semantics/popovers/togglePopover-expected.txt b/...tTests/imported/w3c/web-platform-tests/html/semantics/popovers/togglePopover-expected.txt
@@ -0,0 +1,4 @@
+
+PASS togglePopover should toggle the popover and return true or false as specified.
+PASS togglePopover's return value should reflect what the end state is, not just the force parameter.
+
diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/togglePopover.html b/LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/togglePopover.html
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<link rel=author href="mailto:jarhar@chromium.org">
+<link rel=help href="https://github.com/whatwg/html/issues/9043">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<div id=popover popover=auto>popover</div>
+<div id=popover2 popover=auto>popover</div>
+
+<script>
+test(() => {
+  assert_false(popover.matches(':popover-open'),
+    'Popover should be closed when the test starts.');
+
+  assert_true(popover.togglePopover(),
+    'togglePopover() should return true.');
+  assert_true(popover.matches(':popover-open'),
+    'togglePopover() should open the popover.');
+
+  assert_true(popover.togglePopover(/*force=*/true),
+    'togglePopover(true) should return true.');
+  assert_true(popover.matches(':popover-open'),
+    'togglePopover(true) should open the popover.');
+
+  assert_false(popover.togglePopover(),
+    'togglePopover() should return false.');
+  assert_false(popover.matches(':popover-open'),
+    'togglePopover() should close the popover.');
+
+  assert_false(popover.togglePopover(/*force=*/false),
+    'togglePopover(false) should return false.');
+  assert_false(popover.matches(':popover-open'),
+    'togglePopover(false) should close the popover.');
+}, 'togglePopover should toggle the popover and return true or false as specified.');
+
+test(() => {
+  const popover = document.getElementById('popover2');
+  popover.addEventListener('beforetoggle', event => event.preventDefault(), {once: true});
+  assert_false(popover.togglePopover(/*force=*/true),
+    'togglePopover(true) should return false when the popover does not get opened.');
+  assert_false(popover.matches(':popover-open'));
+
+  // We could also add a test for the return value of togglePopover(false),
+  // but every way to prevent that from hiding the popover also throws an
+  // exception, so the return value is not testable.
+}, `togglePopover's return value should reflect what the end state is, not just the force parameter.`);
+</script>
+
diff --git a/Source/WebCore/html/HTMLElement.cpp b/Source/WebCore/html/HTMLElement.cpp
@@ -1478,15 +1478,18 @@ ExceptionOr<void> HTMLElement::hidePopover()
     return hidePopoverInternal(FocusPreviousElement::Yes, FireEvents::Yes);
 }
 
-ExceptionOr<void> HTMLElement::togglePopover(std::optional<bool> force)
-{
-    if (isPopoverShowing() && !force.value_or(false))
-        return hidePopover();
-
-    if ((!popoverData() || popoverData()->visibilityState() == PopoverVisibilityState::Hidden) && force.value_or(true))
-        return showPopover();
-
-    return { };
+ExceptionOr<bool> HTMLElement::togglePopover(std::optional<bool> force)
+{
+    if (isPopoverShowing() && !force.value_or(false)) {
+        auto returnValue = hidePopover();
+        if (returnValue.hasException())
+            return returnValue.releaseException();
+    } else if (!isPopoverShowing() && force.value_or(true)) {
+        auto returnValue = showPopover();
+        if (returnValue.hasException())
+            return returnValue.releaseException();
+    }
+    return isPopoverShowing();
 }
 
 void HTMLElement::popoverAttributeChanged(const AtomString& value)

diff --git a/Source/WebCore/html/HTMLElement.h b/Source/WebCore/html/HTMLElement.h
@@ -152,7 +152,7 @@ class HTMLElement : public StyledElement {
     ExceptionOr<void> showPopover(const HTMLFormControlElement* = nullptr);
     ExceptionOr<void> hidePopover();
     ExceptionOr<void> hidePopoverInternal(FocusPreviousElement, FireEvents);
-    ExceptionOr<void> togglePopover(std::optional<bool> force);
+    ExceptionOr<bool> togglePopover(std::optional<bool> force);
 
     PopoverState popoverState() const;
     const AtomString& popover() const;

diff --git a/Source/WebCore/html/HTMLElement.idl b/Source/WebCore/html/HTMLElement.idl
@@ -53,7 +53,7 @@
     // The popover API
     [EnabledBySetting=PopoverAttributeEnabled, DisabledByQuirk=shouldDisablePopoverAttribute] undefined showPopover();
     [EnabledBySetting=PopoverAttributeEnabled, DisabledByQuirk=shouldDisablePopoverAttribute] undefined hidePopover();
-    [EnabledBySetting=PopoverAttributeEnabled, DisabledByQuirk=shouldDisablePopoverAttribute] undefined togglePopover(optional boolean force);
+    [EnabledBySetting=PopoverAttributeEnabled, DisabledByQuirk=shouldDisablePopoverAttribute] boolean togglePopover(optional boolean force);
     [CEReactions=Needed, EnabledBySetting=PopoverAttributeEnabled, DisabledByQuirk=shouldDisablePopoverAttribute] attribute [AtomString] DOMString? popover;
 
     // Non-standard: IE extension. May get added to the specification (https://github.com/whatwg/html/issues/668).