Update the pop-up implementation to match hint/auto resolutions

See the set of behaviors described here: openui/open-ui#525 (comment) which were resolved here: openui/open-ui#525 (comment) This CL implements those changes in behavior, which mostly deal with how popup=auto and popup=hint interact, and some small changes to how `defaultopen` works. Bug: 1307772 Change-Id: I4d280b60e7c341b4d0f97fe82e60134ff4a6e1fa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3742105 Auto-Submit: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1021327}
web-platform-tests · Jul 6, 2022 · 9713c9a · 9713c9a
1 parent 20c96e6
commit 9713c9a
Show file tree

Hide file tree

Showing 7 changed files with 256 additions and 108 deletions.
diff --git a/html/semantics/popups/popup-attribute-basic.tentative.html b/html/semantics/popups/popup-attribute-basic.tentative.html
@@ -20,6 +20,17 @@
   <div popup=invalid>Not a pop-up</div>
 </div>
 
+<div popup class=animated>Animated pop-up</div>
+<style>
+[popup].animated {
+  opacity: 0;
+  transition: opacity 10s;
+}
+[popup].animated:top-layer {
+  opacity: 1;
+}
+</style>
+
 <script>
   function popUpVisible(popUp, isPopUp) {
     const isVisible = isElementVisible(popUp);
@@ -75,9 +86,9 @@
   }
 
   test((t) => {
-    // YOU CAN set the attribute to anything.
-    // Setting IDL to something sets the content attribute to exactly that, always.
-    // GETTING the IDL only gets valid values.
+    // You can set the `popup` attribute to anything.
+    // Setting the `popUp` IDL to a string sets the content attribute to exactly that, always.
+    // Getting the `popUp` IDL value only retrieves valid values.
     const popUp = createPopUp(t);
     assert_equals(popUp.popUp,'auto');
     popUp.setAttribute('popup','hint');
@@ -198,4 +209,24 @@
       },`A showing popup=${type} does not match :modal`);
     }
   });
+
+  promise_test(async () => {
+    const popUp = document.querySelector('[popup].animated');
+    assert_true(!!popUp);
+    assert_false(isElementVisible(popUp));
+    popUp.showPopUp();
+    assert_true(popUp.matches(':top-layer'));
+    assert_true(getComputedStyle(popUp).opacity < 0.1,'Animations should start on show');
+    assert_throws_dom("InvalidStateError",() => popUp.showPopUp(),'Calling showPopUp on a popup that is in the process of animating show should throw InvalidStateError');
+    await finishAnimations(popUp);
+    assert_true(getComputedStyle(popUp).opacity > 0.9);
+    assert_true(isElementVisible(popUp));
+    popUp.hidePopUp();
+    assert_false(popUp.matches(':top-layer'));
+    assert_true(getComputedStyle(popUp).opacity > 0.9,'Animations should start on hide');
+    assert_throws_dom("InvalidStateError",() => popUp.hidePopUp(),'Calling hidePopUp on a popup that is in the process of animating hide should throw InvalidStateError');
+    popUp.showPopUp(); // But showPopUp should still be ok.
+    popUp.hidePopUp(); // Clean up
+    await finishAnimations(popUp);
+  },'Exceptions are thrown even when show/hide are animated')
 </script>
diff --git a/html/semantics/popups/popup-defaultopen-hints.tentative.html b/html/semantics/popups/popup-defaultopen-hints.tentative.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<link rel=author href="mailto:masonf@chromium.org">
+<link rel=help href="https://open-ui.org/components/popup.research.explainer">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/popup-utils.js"></script>
+
+<div popup=hint defaultopen>This is a popup=hint with defaultopen, which should NOT be open upon load</div>
+
+<script>
+  promise_test(async () => {
+    await waitForRender();
+    await waitForRender();
+    const popUp = document.querySelector('[popup]');
+    assert_false(popUp.matches(':top-layer'),'defaultopen does not apply to hint pop-ups');
+    assert_true(popUp.hasAttribute('defaultopen'),'attribute should still be present');
+    assert_true(popUp.defaultOpen,'defaultopen should be present/true, even if not opened');
+  }, "The defaultopen attribute should not apply to popup=hint");
+</script>
diff --git a/html/semantics/popups/popup-defaultopen.tentative.html b/html/semantics/popups/popup-defaultopen.tentative.html
@@ -8,7 +8,6 @@
 <div popup id=p1 defaultopen>This is a popup, which should be open upon load</div>
 <script></script> <!-- Possibly yield the parser, just to double-check -->
 <div popup id=p2 defaultopen>This is a second popup with defaultopen, which should NOT be open upon load</div>
-<div popup=hint id=p2b defaultopen>This is a hint popup with defaultopen, which should NOT be open upon load</div>
 <div popup id=p3>Also not visible</div>
 
 <div popup=manual id=p4 defaultopen>This is a manual popup with defaultopen, which should be open upon load</div>
@@ -25,7 +24,6 @@
         assert_true(p2.hasAttribute('defaultopen'),'defaultopen should be present/true, even if not opened');
         assert_true(p2.defaultOpen,'defaultopen should be present/true, even if not opened');
 
-        assert_false(p2b.matches(':top-layer'),'Only the first popup/hint with defaultopen should be open on load');
         assert_true(p4.matches(':top-layer'),'defaultopen should open all manual popups');
         assert_true(p5.matches(':top-layer'),'defaultopen should open all manual popups');
 

diff --git a/html/semantics/popups/popup-focus.tentative.html b/html/semantics/popups/popup-focus.tentative.html
@@ -132,10 +132,9 @@
     t.add_cleanup(() => priorFocus.remove());
     return priorFocus;
   }
-  async function finishAnimations(popUp) {
-    popUp.getAnimations({subtree: true}).forEach(animation => animation.finish());
-    await waitForRender();
-    assert_false(isElementVisible(popUp));
+  async function finishAnimationsAndVerifyHide(popUp) {
+    await finishAnimations(popUp);
+    assert_false(isElementVisible(popUp),'After animations are finished, the pop-up should be hidden');
   }
   function activateAndVerify(popUp) {
     const testName = popUp.getAttribute('data-test');
@@ -155,7 +154,7 @@
       assert_equals(document.activeElement, expectedFocusedElement, `${testName} activated by popUp.showPopUp()`);
       popUp.hidePopUp();
       assert_equals(document.activeElement, priorFocus, 'prior element should get focus on hide');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
 
       // Hit Escape:
       priorFocus.focus();
@@ -164,7 +163,7 @@
       assert_equals(document.activeElement, expectedFocusedElement, `${testName} activated by popUp.showPopUp()`);
       await sendEscape();
       assert_equals(document.activeElement, priorFocus, 'prior element should get focus after Escape');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
 
       // Change the pop-up type:
       priorFocus.focus();
@@ -174,7 +173,7 @@
       popUp.popUp = 'hint';
       assert_false(popUp.matches(':top-layer'), 'Changing the pop-up type should hide the pop-up');
       assert_equals(document.activeElement, priorFocus, 'prior element should get focus when the type is changed');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
       popUp.popUp = 'auto';
 
       // Remove from the document:
@@ -212,7 +211,7 @@
       button.click(); // Button is set to toggle the pop-up
       assert_false(popUp.matches(':top-layer'));
       assert_equals(document.activeElement, priorFocus, 'prior element should get focus on button-toggled hide');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
     }, "Popup focus test: " + testName);
 
     promise_test(async t => {
@@ -229,7 +228,7 @@
       await clickOn(button); // This will not light dismiss, but will "toggle" the popUp.
       assert_false(popUp.matches(':top-layer'));
       assert_equals(document.activeElement, priorFocus, 'Focus should return to prior focus');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
 
       // Same thing, but the button is contained within the pop-up
       button.removeAttribute('togglepopup');
@@ -244,7 +243,7 @@
       await clickOn(button);
       assert_false(popUp.matches(':top-layer'), 'clicking button should hide the pop-up');
       assert_equals(document.activeElement, priorFocus, 'Contained button should return focus to the previously focused element');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
 
       // Same thing, but the button is unrelated (no togglepopup)
       button = document.createElement('button');
@@ -255,7 +254,7 @@
       await clickOn(button); // This will light dismiss the pop-up, focus the prior focus, then focus this button.
       assert_false(popUp.matches(':top-layer'), 'clicking button should hide the pop-up (via light dismiss)');
       assert_equals(document.activeElement, button, 'Focus should go to unrelated button on light dismiss');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
     }, "Popup button click focus test: " + testName);
 
     promise_test(async t => {
@@ -276,7 +275,7 @@
       assert_equals(document.activeElement, newFocus, 'focus should not change when prior focus is removed');
       popUp.hidePopUp();
       assert_not_equals(document.activeElement, priorFocus, 'focused element has been removed');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
       document.body.appendChild(priorFocus); // Put it back
 
       // Move the prior focus inside the (already open) pop-up
@@ -292,7 +291,7 @@
       assert_true(isElementVisible(popUp),'Animations should keep the pop-up visible');
       assert_not_equals(getComputedStyle(popUp).display,'none','Animations should keep the pop-up visible');
       assert_equals(document.activeElement, priorFocus, 'focused element gets focused');
-      await finishAnimations(popUp);
+      await finishAnimationsAndVerifyHide(popUp);
       assert_equals(getComputedStyle(popUp).display,'none','Animations have ended, pop-up should be hidden');
       assert_not_equals(document.activeElement, priorFocus, 'focused element is display:none inside the pop-up');
       document.body.appendChild(priorFocus); // Put it back

diff --git a/html/semantics/popups/popup-light-dismiss.tentative.html b/html/semantics/popups/popup-light-dismiss.tentative.html
@@ -425,37 +425,37 @@
 </style>
 <script>
 promise_test(async () => {
-  const popup = document.querySelector('#p10');
+  const auto = document.querySelector('#p10');
   const hint = document.querySelector('#p11');
   const manual = document.querySelector('#p12');
   // All three can be open at once, if shown in this order:
-  popup.showPopUp();
+  auto.showPopUp();
   hint.showPopUp();
   manual.showPopUp();
-  assert_true(popup.matches(':top-layer'));
+  assert_true(auto.matches(':top-layer'));
   assert_true(hint.matches(':top-layer'));
   assert_true(manual.matches(':top-layer'));
-  // The hint was opened last, so clicking it shouldn't close anything:
+  // Clicking the hint will close the auto, but not the manual.
   await clickOn(hint);
-  assert_true(popup.matches(':top-layer'),'popup should stay open');
+  assert_false(auto.matches(':top-layer'),'auto should be hidden');
   assert_true(hint.matches(':top-layer'),'hint should stay open');
   assert_true(manual.matches(':top-layer'),'manual does not light dismiss');
-  // Clicking outside should close the hint and popup, but not the manual:
+  // Clicking outside should close the hint, but not the manual:
   await clickOn(outside);
-  assert_false(popup.matches(':top-layer'),'popup should close');
+  assert_false(auto.matches(':top-layer'));
   assert_false(hint.matches(':top-layer'),'hint should close');
   assert_true(manual.matches(':top-layer'),'manual does not light dismiss');
   manual.hidePopUp();
   assert_false(manual.matches(':top-layer'));
-  popup.showPopUp();
+  auto.showPopUp();
   hint.showPopUp();
-  assert_true(popup.matches(':top-layer'));
+  assert_true(auto.matches(':top-layer'));
   assert_true(hint.matches(':top-layer'));
-  // Clicking on the popup should close the hint:
-  await clickOn(popup);
-  assert_true(popup.matches(':top-layer'),'popup should stay open');
+  // Clicking on the auto should close the hint:
+  await clickOn(auto);
+  assert_true(auto.matches(':top-layer'),'auto should stay open');
   assert_false(hint.matches(':top-layer'),'hint should light dismiss');
-  popup.hidePopUp();
-  assert_false(popup.matches(':top-layer'));
+  auto.hidePopUp();
+  assert_false(auto.matches(':top-layer'));
 },'Light dismiss of mixed popup types');
 </script>