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}
web-platform-tests · May 16, 2023 · 0297d56 · 0297d56
1 parent 182f582
commit 0297d56
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 18 deletions.
diff --git a/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html b/html/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/html/semantics/popovers/popover-attribute-basic.html b/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/html/semantics/popovers/popover-light-dismiss.html b/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/html/semantics/popovers/popover-move-documents.html b/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/html/semantics/popovers/resources/popover-utils.js b/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);
 }