[popover] Fix popover-move-documents.html

https://bugs.webkit.org/show_bug.cgi?id=255780 Reviewed by Tim Nguyen. The beforetoggle event handlers can move popover elements between documents, take this into account when checking popover validity [1]. [1] whatwg/html#9182 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents.html: Added. * Source/WebCore/html/HTMLElement.cpp: (WebCore::checkPopoverValidity): (WebCore::HTMLElement::showPopover): (WebCore::HTMLElement::hidePopoverInternal): Canonical link: https://commits.webkit.org/263449@main
WebKit · Apr 27, 2023 · a277ac2 · a277ac2
1 parent a8afcd9
commit a277ac2
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 6 deletions.
diff --git a/...ported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents-expected.txt b/...ported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents-expected.txt
@@ -0,0 +1,6 @@
+   p3
+
+PASS Moving popovers between documents while showing should throw an exception.
+PASS Moving popovers between documents while hiding should not throw an exception.
+PASS Moving popovers between documents during light dismiss should throw an exception.
+
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
@@ -0,0 +1,70 @@
+<!DOCTYPE html>
+<link rel=author href="mailto:jarhar@chromium.org">
+<link rel=help href="https://github.com/whatwg/html/issues/9177">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<iframe id=myframe srcdoc="<p>iframe</p>"></iframe>
+<div id=p1 popover=auto>p1</div>
+<script>
+test(() => {
+  p1.addEventListener('beforetoggle', () => {
+    myframe.contentWindow.document.body.appendChild(p1);
+  });
+  assert_throws_dom('InvalidStateError', () => p1.showPopover());
+}, 'Moving popovers between documents while showing should throw an exception.');
+</script>
+
+<iframe id=myframe2 srcdoc="<p>iframe</p>"></iframe>
+<div id=p2 popover=auto>p2</div>
+<script>
+test(() => {
+  const p2 = document.getElementById('p2');
+  p2.showPopover();
+  p2.addEventListener('beforetoggle', () => {
+    myframe2.contentWindow.document.body.appendChild(p2);
+  });
+  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());
+  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.');
+</script>
+
+<iframe id=myframe3 srcdoc="<p>iframe</p>"></iframe>
+<div id=p3 popover=auto>
+  p3
+  <div id=p4 popover=auto>p4</div>
+  <div id=p5 popover=auto>p5</div>
+</div>
+<script>
+test(() => {
+  p3.showPopover();
+  p4.showPopover();
+  p4.addEventListener('beforetoggle', event => {
+    if (event.newState === 'closed') {
+      assert_true(p3.matches(':popover-open'),
+        'p3 should be showing in the event handler.');
+      assert_true(p4.matches(':popover-open'),
+        'p4 should be showing in the event handler.');
+      assert_equals(event.target, p4,
+        'The events target should be p4.');
+      myframe3.contentWindow.document.body.appendChild(p5);
+    }
+  });
+  assert_true(p3.matches(':popover-open'),
+    'p3 should be open after calling showPopover on it.');
+  assert_true(p4.matches(':popover-open'),
+    'p4 should be open after calling showPopover on it.');
+
+  const p5 = document.getElementById('p5');
+  assert_throws_dom('InvalidStateError', () => p5.showPopover());
+  assert_false(p5.matches(':popover-open'),
+    'p5 should be closed after moving it between documents.');
+}, 'Moving popovers between documents during light dismiss should throw an exception.');
+</script>
diff --git a/Source/WebCore/html/HTMLElement.cpp b/Source/WebCore/html/HTMLElement.cpp
@@ -1237,14 +1237,17 @@ ExceptionOr<Ref<ElementInternals>> HTMLElement::attachInternals()
     return ElementInternals::create(*this);
 }
 
-static ExceptionOr<void> checkPopoverValidity(HTMLElement& element, PopoverVisibilityState expectedState)
+static ExceptionOr<void> 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.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 };
 
@@ -1370,30 +1373,31 @@ ExceptionOr<void> HTMLElement::showPopover()
 
     ASSERT(!isInTopLayer());
 
+    Ref document = this->document();
     auto event = ToggleEvent::create(eventNames().beforetoggleEvent, { EventInit { }, "closed"_s, "open"_s }, Event::IsCancelable::Yes);
     dispatchEvent(event);
     if (event->defaultPrevented() || event->defaultHandled())
         return { };
 
-    if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden); check.hasException())
+    if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr()); check.hasException())
         return check.releaseException();
 
     ASSERT(popoverData());
 
     if (popoverState() == PopoverState::Auto) {
         auto originalState = popoverState();
         RefPtr ancestor = topmostPopoverAncestor(*this);
-        document().hideAllPopoversUntil(ancestor.get(), FocusPreviousElement::No, FireEvents::Yes);
+        document->hideAllPopoversUntil(ancestor.get(), FocusPreviousElement::No, FireEvents::Yes);
 
         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); check.hasException())
+        if (auto check = checkPopoverValidity(*this, PopoverVisibilityState::Hidden, document.ptr()); check.hasException())
             return check.releaseException();
     }
 
-    bool shouldRestoreFocus = !document().topmostAutoPopover();
-    RefPtr previouslyFocusedElement = document().focusedElement();
+    bool shouldRestoreFocus = !document->topmostAutoPopover();
+    RefPtr previouslyFocusedElement = document->focusedElement();
 
     addToTopLayer();