Don't throw when popovers and dialogs are in requested state

This is being changed in the HTML spec here: whatwg/html#9142 Change-Id: Ib8aaaf314c2a1de5d082494e5172e029d531c8e8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4494823 Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1144952}
chromium · May 16, 2023 · 69a2d95 · 69a2d95
1 parent 7d0ccaa
commit 69a2d95
Show file tree

Hide file tree

Showing 8 changed files with 133 additions and 42 deletions.
diff --git a/third_party/blink/renderer/core/html/html_dialog_element.cc b/third_party/blink/renderer/core/html/html_dialog_element.cc
@@ -188,8 +188,21 @@ void HTMLDialogElement::ScheduleCloseEvent() {
 }
 
 void HTMLDialogElement::show(ExceptionState& exception_state) {
-  if (FastHasAttribute(html_names::kOpenAttr))
-    return;
+  if (RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) {
+    if (FastHasAttribute(html_names::kOpenAttr)) {
+      if (is_modal_) {
+        exception_state.ThrowDOMException(
+            DOMExceptionCode::kInvalidStateError,
+            "The dialog is already open as a modal dialog, and therefore "
+            "cannot be opened as a non-modal dialog.");
+      }
+      return;
+    }
+  } else {
+    if (FastHasAttribute(html_names::kOpenAttr)) {
+      return;
+    }
+  }
 
   if (RuntimeEnabledFeatures::HTMLPopoverAttributeEnabled(
           GetDocument().GetExecutionContext()) &&
@@ -247,12 +260,24 @@ class DialogCloseWatcherEventListener : public NativeEventListener {
 };
 
 void HTMLDialogElement::showModal(ExceptionState& exception_state) {
-  if (FastHasAttribute(html_names::kOpenAttr)) {
-    return exception_state.ThrowDOMException(
-        DOMExceptionCode::kInvalidStateError,
-        "The element already has an 'open' "
-        "attribute, and therefore cannot be "
-        "opened modally.");
+  if (RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) {
+    if (FastHasAttribute(html_names::kOpenAttr)) {
+      if (!is_modal_) {
+        exception_state.ThrowDOMException(
+            DOMExceptionCode::kInvalidStateError,
+            "The dialog is already open as a non-modal dialog, and therefore "
+            "cannot be opened as a modal dialog.");
+      }
+      return;
+    }
+  } else {
+    if (FastHasAttribute(html_names::kOpenAttr)) {
+      return exception_state.ThrowDOMException(
+          DOMExceptionCode::kInvalidStateError,
+          "The element already has an 'open' "
+          "attribute, and therefore cannot be "
+          "opened modally.");
+    }
   }
   if (!isConnected()) {
     return exception_state.ThrowDOMException(

diff --git a/third_party/blink/renderer/core/html/html_element.cc b/third_party/blink/renderer/core/html/html_element.cc
@@ -1330,31 +1330,35 @@ bool HTMLElement::IsPopoverReady(PopoverTriggerAction action,
                           "value for the 'popover' attribute.");
     return false;
   }
-  if (!isConnected()) {
-    maybe_throw_exception(DOMExceptionCode::kInvalidStateError,
-                          "Invalid on disconnected popover elements.");
-    return false;
-  }
-  if (expected_document && &GetDocument() != expected_document) {
-    maybe_throw_exception(DOMExceptionCode::kInvalidStateError,
-                          "Invalid when the document changes while showing or "
-                          "hiding a popover element.");
-    return false;
-  }
   if (action == PopoverTriggerAction::kShow &&
       GetPopoverData()->visibilityState() != PopoverVisibilityState::kHidden) {
-    maybe_throw_exception(DOMExceptionCode::kInvalidStateError,
-                          "Invalid on popover elements which aren't hidden.");
+    if (!RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) {
+      maybe_throw_exception(DOMExceptionCode::kInvalidStateError,
+                            "Invalid on popover elements which aren't hidden.");
+    }
     return false;
   }
   if (action == PopoverTriggerAction::kHide &&
       GetPopoverData()->visibilityState() != PopoverVisibilityState::kShowing) {
     // Important to check that visibility is not kShowing (rather than
     // popoverOpen()), because a hide transition might have been started on this
     // popover already, and we don't want to allow a double-hide.
-    maybe_throw_exception(
-        DOMExceptionCode::kInvalidStateError,
-        "Invalid on popover elements that aren't already showing.");
+    if (!RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) {
+      maybe_throw_exception(
+          DOMExceptionCode::kInvalidStateError,
+          "Invalid on popover elements that aren't already showing.");
+    }
+    return false;
+  }
+  if (!isConnected()) {
+    maybe_throw_exception(DOMExceptionCode::kInvalidStateError,
+                          "Invalid on disconnected popover elements.");
+    return false;
+  }
+  if (expected_document && &GetDocument() != expected_document) {
+    maybe_throw_exception(DOMExceptionCode::kInvalidStateError,
+                          "Invalid when the document changes while showing or "
+                          "hiding a popover element.");
     return false;
   }
   if (action == PopoverTriggerAction::kShow && IsA<HTMLDialogElement>(this) &&

diff --git a/third_party/blink/renderer/platform/runtime_enabled_features.json5 b/third_party/blink/renderer/platform/runtime_enabled_features.json5
@@ -2697,6 +2697,13 @@
       name: "PointerEventDeviceId",
       status: "test",
     },
+    // PopoverDialogDontThrow makes popover and dialog elements stop throwing
+    // exceptions when calling their show and hide methods when they are
+    // already in their requested state. See https://github.com/whatwg/html/pull/9142
+    {
+      name: "PopoverDialogDontThrow",
+      status: "stable",
+    },
     {
       name: "Portals",
       // Portals must be enabled by blink::features::kPortals as we require the

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,29 @@
+<!DOCTYPE html>
+<link rel=author href="mailto:jarhar@chromium.org">
+<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/...d_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-attribute-basic.html b/...d_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-attribute-basic.html
@@ -254,11 +254,21 @@
     },{once: true});
     assert_true(popover.matches(':popover-open'));
     assert_true(other_popover.matches(':popover-open'));
-    assert_throws_dom('InvalidStateError', () => popover.hidePopover());
+    popover.hidePopover(); // Calling hidePopover on a hidden popover should not throw.
     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');
-  },`Changing the popover type in a "beforetoggle" event handler should throw an exception (during hidePopover())`);
+    other_popover.hidePopover(); // Calling hidePopover on a hidden popover should not throw.
+  },`Changing the popover type in a "beforetoggle" event handler during hidePopover() should not throw an exception`);
+
+  test(t => {
+    const popover = document.createElement('div');
+    assert_throws_dom('NotSupportedError', () => popover.hidePopover(),
+      'Calling hidePopover on an element without a popover attribute should throw.');
+    popover.setAttribute('popover', 'auto');
+    popover.hidePopover(); // Calling hidePopover on a disconnected popover should not throw.
+    assert_throws_dom('InvalidStateError', () => popover.showPopover(),
+      'Calling showPopover on a disconnected popover should throw.');
+  },'Calling hidePopover on a disconnected popover should not throw.');
 
   function interpretedType(typeString,method) {
     if (validTypes.includes(typeString))

diff --git a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-light-dismiss.html b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-light-dismiss.html
@@ -532,7 +532,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'));
@@ -579,10 +579,7 @@
     p20.showPopover();
   });
   p20.addEventListener('beforetoggle', logEvents);
-  // Because the `beforetoggle` handler shows a different popover,
-  // and that action closes the p19 popover, the call to hidePopover()
-  // will result in an exception.
-  assert_throws_dom('InvalidStateError',() => p19.hidePopover());
+  p19.hidePopover();
   assert_array_equals(events,['hide p19','show p20'],'There should not be a second hide event for 19');
   assert_false(p19.matches(':popover-open'));
   assert_true(p20.matches(':popover-open'));

diff --git a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-move-documents.html b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-move-documents.html
@@ -4,10 +4,30 @@
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 
+<script>
+async function iframeLoaded(iframe) {
+  return new Promise(resolve => {
+    if (iframe.contentWindow.document.readyState == 'complete') {
+      resolve();
+    } else {
+      iframe.onload = resolve;
+    }
+  });
+}
+</script>
+
 <iframe id=myframe srcdoc="<p>iframe</p>"></iframe>
 <div id=p1 popover=auto>p1</div>
 <script>
-test(() => {
+promise_test(async () => {
+  await iframeLoaded(myframe);
+  await new Promise(resolve => {
+    if (myframe.contentWindow.document.readyState == 'complete') {
+      resolve();
+    } else {
+
+    }
+  });
   p1.addEventListener('beforetoggle', () => {
     myframe.contentWindow.document.body.appendChild(p1);
   });
@@ -18,7 +38,8 @@
 <iframe id=myframe2 srcdoc="<p>iframe</p>"></iframe>
 <div id=p2 popover=auto>p2</div>
 <script>
-test(() => {
+promise_test(async () => {
+  await iframeLoaded(myframe2);
   const p2 = document.getElementById('p2');
   p2.showPopover();
   p2.addEventListener('beforetoggle', () => {
@@ -27,10 +48,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.');
@@ -43,7 +61,8 @@
   <div id=p5 popover=auto>p5</div>
 </div>
 <script>
-test(() => {
+promise_test(async () => {
+  await iframeLoaded(myframe3);
   p3.showPopover();
   p4.showPopover();
   p4.addEventListener('beforetoggle', event => {

diff --git a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/resources/popover-utils.js b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/resources/popover-utils.js
@@ -153,10 +153,10 @@ 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();
@@ -172,7 +172,7 @@ 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);
 }