Combine the popoverhide/popovershow events into one beforetoggle event

See [1] and [2] for context. This eliminates the "popoverhide" and "popovershow" events, and replaces them with the "beforetoggle" event, for both of these transitions. The event is a BeforeToggleEvent, which has attributes `currentState` and `newState`, both of which are either "open" or "closed" (and opposite of each other). [1] openui/open-ui#607 (comment) [2] whatwg/html#8221 (comment) Bug: 1307772 Change-Id: I404861a10579365b56e6e8eae7c29f88a5aac166 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4035832 Reviewed-by: David Baron <[email protected]> Commit-Queue: Mason Freed <[email protected]> Auto-Submit: Mason Freed <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1080695}
web-platform-tests · Dec 8, 2022 · 90aec8c · 90aec8c
1 parent 466c3b3
commit 90aec8c
Show file tree

Hide file tree

Showing 8 changed files with 283 additions and 49 deletions.
diff --git a/html/semantics/popovers/popover-animation-corner-cases.tentative.html b/html/semantics/popovers/popover-animation-corner-cases.tentative.html
@@ -100,7 +100,9 @@
   assert_true(isElementVisible(popover));
   assert_equals(popover.getAnimations({subtree: true}).length,0);
   let animation;
-  popover.addEventListener('popoverhide', () => {
+  popover.addEventListener('beforetoggle', (e) => {
+    if (e.newState !== "closed")
+      return;
     animation = popover.animate([{opacity: 1},{opacity: 0}],1000000);
   });
   assert_equals(popover.getAnimations({subtree: true}).length,0,'There should be no animations yet');
@@ -111,7 +113,7 @@
   animation.finish();
   await waitForRender();
   assert_false(isElementVisible(popover),'Once the animation ends, the popover is hidden');
-},'It should be possible to use the "popoverhide" event handler to animate the hide');
+},'It should be possible to use the "beforetoggle" event handler to animate the hide');
 
 
 promise_test(async (t) => {
@@ -121,8 +123,10 @@
   popover.showPopover();
   assert_true(isElementVisible(popover));
   assert_equals(popover.getAnimations({subtree: true}).length,0);
-  let animation
-  popover.addEventListener('popoverhide', () => {
+  let animation;
+  popover.addEventListener('beforetoggle', (e) => {
+    if (e.newState !== "closed")
+      return;
     animation = popover.animate([{opacity: 1},{opacity: 0}],1000000);
   });
   assert_equals(popover.getAnimations({subtree: true}).length,0,'There should be no animations yet');
@@ -133,19 +137,17 @@
   animation.finish();
   await waitForRender();
   assert_false(isElementVisible(popover),'Once the animation ends, the popover is hidden');
-},'It should be possible to use the "popoverhide" event handler to animate the hide, even when the hide is due to dialog.showModal');
+},'It should be possible to use the "beforetoggle" event handler to animate the hide, even when the hide is due to dialog.showModal');
 
 promise_test(async (t) => {
   const {popover, descendent} = createPopover(t,'');
   popover.showPopover();
   assert_true(isElementVisible(popover));
-  popover.addEventListener('popoverhide', (e) => {
-    e.preventDefault();
-  });
+  popover.addEventListener('beforetoggle', (e) => e.preventDefault());
   popover.hidePopover();
   await waitForRender();
   assert_false(isElementVisible(popover),'Even if hide event is cancelled, the popover still closes');
-},'hide event cannot be cancelled');
+},'toggle event cannot be cancelled');
 
 promise_test(async (t) => {
   const {popover, descendent} = createPopover(t,'animation');
@@ -173,7 +175,9 @@
   popover.showPopover();
   assert_true(isElementVisible(popover));
   assert_equals(popover.getAnimations({subtree: true}).length,0);
-  popover.addEventListener('popoverhide', () => {
+  popover.addEventListener('beforetoggle', (e) => {
+    if (e.newState !== "closed")
+      return;
     popover.animate([{opacity: 1},{opacity: 0}],1000000);
   });
   assert_equals(popover.getAnimations({subtree: true}).length,0,'There should be no animations yet');
@@ -197,7 +201,9 @@
 promise_test(async (t) => {
   const {popover, descendent} = createPopover(t,'');
   popover.showPopover();
-  popover.addEventListener('popoverhide', () => {
+  popover.addEventListener('beforetoggle', (e) => {
+    if (e.newState !== "closed")
+      return;
     popover.animate([{opacity: 1},{opacity: 0}],1000000);
   });
   popover.hidePopover();

diff --git a/html/semantics/popovers/popover-attribute-basic.tentative.html b/html/semantics/popovers/popover-attribute-basic.tentative.html
@@ -275,15 +275,16 @@
     other_popover.showPopover();
     const popover = createPopover(t);
     popover.setAttribute('popover','auto');
-    other_popover.addEventListener('popoverhide',() => {
+    other_popover.addEventListener('toggle', (e) => {
+      if (e.state !== "closing") return;
       popover.setAttribute('popover','manual');
     },{once: true});
     assert_true(other_popover.matches(':open'));
     assert_false(popover.matches(':open'));
     popover.showPopover();
     assert_false(other_popover.matches(':open'),'unrelated popover is hidden');
     assert_false(popover.matches(':open'),'popover is not shown if its type changed during show');
-  },`Changing the popover type in a "popoverhide" event handler should not cause problems (during showPopover())`);
+  },`Changing the popover type in a "toggle" event handler should not cause problems (during showPopover())`);
 
   test((t) => {
     const popover = createPopover(t);
@@ -294,11 +295,13 @@
     popover.showPopover();
     other_popover.showPopover();
     let nested_popover_hidden=false;
-    other_popover.addEventListener('popoverhide',() => {
+    other_popover.addEventListener('toggle', (e) => {
+      if (e.state !== "closing") return;
       nested_popover_hidden = true;
       popover.setAttribute('popover','manual');
     },{once: true});
-    popover.addEventListener('popoverhide',() => {
+    popover.addEventListener('toggle', (e) => {
+      if (e.state !== "closing") return;
       assert_true(nested_popover_hidden,'The nested popover should be hidden first');
     },{once: true});
     assert_true(popover.matches(':open'));
@@ -307,7 +310,7 @@
     assert_false(other_popover.matches(':open'),'unrelated popover is hidden');
     assert_false(popover.matches(':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 "popoverhide" event handler should not cause problems (during hidePopover())`);
+  },`Changing the popover type in a "toggle" event handler should not cause problems (during hidePopover())`);
 
   function interpretedType(typeString,method) {
     if (validTypes.includes(typeString))
@@ -344,7 +347,8 @@
             popover.showPopover();
             assert_true(popover.matches(':open'));
             let gotEvent = false;
-            popover.addEventListener('popoverhide',() => {
+            popover.addEventListener('toggle', (e) => {
+              if (e.state !== "closing") return;
               gotEvent = true;
               setPopoverValue(popover,inEventType,method);
             },{once:true});
@@ -380,7 +384,7 @@
                 }
               }
             }
-          },`Changing a popover from ${type} to ${newType} (via ${method}), and then ${inEventType} during 'popoverhide' works`);
+          },`Changing a popover from ${type} to ${newType} (via ${method}), and then ${inEventType} during 'toggle' works`);
         });
       });
     });

diff --git a/...opovers/popover-show-event.tentative.html → ...beforetoggle-opening-event.tentative.html b/...opovers/popover-show-event.tentative.html → ...beforetoggle-opening-event.tentative.html
@@ -14,7 +14,9 @@
   requestAnimationFrame(() => {++frameCount;});
   const popover = document.querySelector('[popover]');
   const testText = 'Show Event Occurred';
-  popover.addEventListener('popovershow',() => {
+  popover.addEventListener('beforetoggle',(e) => {
+    if (e.newState !== "open")
+      return;
     popover.textContent = testText;
   })
   popover.offsetHeight;
@@ -25,5 +27,5 @@
   assert_equals(popover.textContent,testText);
   assert_equals(frameCount,0,'nothing should be rendered before the popover is updated');
   popover.hidePopover(); // Cleanup
-},'Ensure the `show` event can be used to populate content before the popover renders');
+},'Ensure the `beforetoggle` event can be used to populate content before the popover renders');
 </script>
diff --git a/html/semantics/popovers/popover-events.tentative.html b/html/semantics/popovers/popover-events.tentative.html
@@ -17,31 +17,32 @@
       assert_false(popover.matches(':open'));
       let showCount = 0;
       let hideCount = 0;
-      function showListener(e) {
-        assert_true(e.target.matches(':closed'),'The popover should be in the :closed state when the popovershow event fires.');
-        assert_false(e.target.matches(':open'),'The popover should *not* be in the :open state when the popovershow event fires.');
-        ++showCount;
-      };
-      function hideListener(e) {
-        assert_true(e.target.matches(':open'),'The popover should be in the :open state when the popoverhide event fires.');
-        assert_false(e.target.matches(':closed'),'The popover should *not* be in the :closed state when the popoverhide event fires.');
-        ++hideCount;
+      function listener(e) {
+        if (e.newState === "open") {
+          assert_equals(e.currentState,"closed",'Popover toggleevent states should be "open" and "closed"')
+          assert_true(e.target.matches(':closed'),'The popover should be in the :closed state when the opening event fires.');
+          assert_false(e.target.matches(':open'),'The popover should *not* be in the :open state when the opening event fires.');
+          ++showCount;
+        } else {
+          assert_equals(e.currentState,"open",'Popover toggleevent states should be "open" and "closed"')
+          assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"')
+          assert_true(e.target.matches(':open'),'The popover should be in the :open state when the hiding event fires.');
+          assert_false(e.target.matches(':closed'),'The popover should *not* be in the :closed state when the hiding event fires.');
+          ++hideCount;
+        }
       };
       switch (method) {
         case "listener":
           const controller = new AbortController();
           const signal = controller.signal;
           t.add_cleanup(() => controller.abort());
-          document.addEventListener('popovershow',showListener, {signal});
-          document.addEventListener('popoverhide',hideListener, {signal});
+          // The 'beforetoggle' event bubbles.
+          document.addEventListener('beforetoggle', listener, {signal});
           break;
         case "attribute":
-          assert_false(popover.hasAttribute('onpopovershow'));
-          assert_false(popover.hasAttribute('onpopoverhide'));
-          t.add_cleanup(() => popover.removeAttribute('onpopovershow'));
-          t.add_cleanup(() => popover.removeAttribute('onpopoverhide'));
-          popover.onpopovershow = showListener;
-          popover.onpopoverhide = hideListener;
+          assert_false(popover.hasAttribute('onbeforetoggle'));
+          t.add_cleanup(() => popover.removeAttribute('onbeforetoggle'));
+          popover.onbeforetoggle = listener;
           break;
         default: assert_unreached();
       }
@@ -62,7 +63,7 @@
       assert_false(popover.matches(':open'));
       assert_equals(1,showCount);
       assert_equals(1,hideCount);
-    }, `Popovershow and popoverhide events (${method}) get properly dispatched for popovers`);
+    }, `Toggle event (${method}) get properly dispatched for popovers`);
   }
 
   promise_test(async t => {
@@ -71,18 +72,20 @@
     const signal = controller.signal;
     t.add_cleanup(() => controller.abort());
     let cancel = true;
-    popover.addEventListener('popovershow',(e) => {
+    popover.addEventListener('beforetoggle',(e) => {
+      if (e.newState !== "open")
+        return;
       if (cancel)
         e.preventDefault();
     }, {signal});
     assert_false(popover.matches(':open'));
     popover.showPopover();
-    assert_false(popover.matches(':open'),'The "popovershow" event should be cancelable');
+    assert_false(popover.matches(':open'),'The "beforetoggle" event should be cancelable for the "opening" transition');
     cancel = false;
     popover.showPopover();
     assert_true(popover.matches(':open'));
     popover.hidePopover();
     assert_false(popover.matches(':open'));
-  }, 'Popovershow event is cancelable');
+  }, 'Toggle event is cancelable for the "opening" transition');
 };
 </script>
diff --git a/html/semantics/popovers/popover-invoking-attribute.tentative.html b/html/semantics/popovers/popover-invoking-attribute.tentative.html
@@ -175,8 +175,12 @@
 const button = document.querySelector('button');
 let showCount = 0;
 let hideCount = 0;
-popover.addEventListener('popovershow',() => ++showCount);
-popover.addEventListener('popoverhide',() => ++hideCount);
+popover.addEventListener('beforetoggle',(e) => {
+  if (e.newState === "open")
+      ++showCount;
+    else
+      ++hideCount;
+  });
 
 async function assertState(expectOpen,expectShow,expectHide) {
   assert_equals(popover.matches(':open'),expectOpen,'Popover open state is incorrect');

diff --git a/html/semantics/popovers/popover-light-dismiss-on-scroll.tentative.html b/html/semantics/popovers/popover-light-dismiss-on-scroll.tentative.html
@@ -44,7 +44,9 @@
   }
   async_test(t => {
     for(let popover of popovers) {
-      popover.addEventListener('popoverhide',e => {
+      popover.addEventListener('beforetoggle',e => {
+        if (e.newState !== "closed")
+          return;
         assert_unreached('Scrolling should not light-dismiss a popover');
       });
     }

diff --git a/html/semantics/popovers/popover-light-dismiss.tentative.html b/html/semantics/popovers/popover-light-dismiss.tentative.html
@@ -47,14 +47,18 @@
   const afterp1 = document.querySelector('#after_p1');
 
   let popover1HideCount = 0;
-  popover1.addEventListener('popoverhide',(e) => {
+  popover1.addEventListener('beforetoggle',(e) => {
+    if (e.newState !== "closed")
+      return;
     ++popover1HideCount;
-    e.preventDefault(); // 'popoverhide' should not be cancellable.
+    e.preventDefault(); // 'beforetoggle' should not be cancellable.
   });
   let popover2HideCount = 0;
-  popover2.addEventListener('popoverhide',(e) => {
+  popover2.addEventListener('beforetoggle',(e) => {
+    if (e.newState !== "closed")
+      return;
     ++popover2HideCount;
-    e.preventDefault(); // 'popoverhide' should not be cancellable.
+    e.preventDefault(); // 'beforetoggle' should not be cancellable.
   });
   promise_test(async () => {
     assert_false(popover1.matches(':open'));
@@ -482,7 +486,9 @@
   p13.showPopover();
   p14.showPopover();
   p15.showPopover();
-  p15.addEventListener('popoverhide',() => {
+  p15.addEventListener('beforetoggle', (e) => {
+    if (e.newState !== "closed")
+      return;
     p14.hidePopover();
   },{once:true});
   assert_true(p13.matches(':open') && p14.matches(':open') && p15.matches(':open'),'all three should be open');