Revert "Update popover post-toggle event naming and behavior"

This reverts commit 2e5ee120d7fc5df4b6c101e88d03a5f62a73d8b8. Reason for revert: cause test failures on Linux Tests (dbg) Example failures: https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests%20(dbg)(1)/111061/ https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests%20(dbg)(1)/111062/ Original change's description: > Update popover post-toggle event naming and behavior > > This CL updates the post-toggle event in the following ways: > 1. Rename the 'aftertoggle' event to 'toggle'. > 2. Rename PopoverToggleEvent to ToggleEvent. > 3. Rename the currentState attribute to oldState. > 4. Add event coalescing behavior. If two transitions occur before the > first 'toggle' event has been fired, cancel the first event and > queue a replacement that has oldState === newState. > > These changes were driven by the corresponding changes to the spec PR: > whatwg/html#8717 > > Bug: 1307772 > Change-Id: Iabc5a9093d7cef3bbd6e54e488d8e571c51ea568 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4195120 > Auto-Submit: Mason Freed <[email protected]> > Commit-Queue: Joey Arhar <[email protected]> > Reviewed-by: Joey Arhar <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1098728} Bug: 1307772 Change-Id: I6e466ecffe4726b4ec69ca14704b24842e45f5d6 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4205308 Owners-Override: Phillis Tang <[email protected]> Bot-Commit: Rubber Stamper <[email protected]> Commit-Queue: Phillis Tang <[email protected]> Cr-Commit-Position: refs/heads/main@{#1098927}
web-platform-tests · Jan 30, 2023 · 938aa51 · 938aa51
1 parent d52f654
commit 938aa51
Show file tree

Hide file tree

Showing 4 changed files with 120 additions and 186 deletions.
diff --git a/html/semantics/popovers/idlharness.tentative.html b/html/semantics/popovers/idlharness.tentative.html
@@ -41,12 +41,12 @@
         'document.getElementById("b3")',
       ],
       BeforeToggleEvent: [
-        'new ToggleEvent("beforetoggle")',
-        'new ToggleEvent("beforetoggle", {oldState: "open"})',
-        'new ToggleEvent("beforetoggle", {oldState: "open",newState: "open"})',
-        'new ToggleEvent("toggle")',
-        'new ToggleEvent("toggle", {oldState: "open"})',
-        'new ToggleEvent("toggle", {oldState: "open",newState: "open"})',
+        'new PopoverToggleEvent("beforetoggle")',
+        'new PopoverToggleEvent("beforetoggle", {currentState: "open"})',
+        'new PopoverToggleEvent("beforetoggle", {currentState: "open",newState: "open"})',
+        'new PopoverToggleEvent("aftertoggle")',
+        'new PopoverToggleEvent("aftertoggle", {currentState: "open"})',
+        'new PopoverToggleEvent("aftertoggle", {currentState: "open",newState: "open"})',
       ],
     });
   }

diff --git a/html/semantics/popovers/popover-events.tentative.html b/html/semantics/popovers/popover-events.tentative.html
@@ -10,13 +10,6 @@
 <div popover>Popover</div>
 
 <script>
-function getPopoverAndSignal(t) {
-  const popover = document.querySelector('[popover]');
-  const controller = new AbortController();
-  const signal = controller.signal;
-  t.add_cleanup(() => controller.abort());
-  return {popover, signal};
-}
 window.onload = () => {
   for(const method of ["listener","attribute"]) {
     promise_test(async t => {
@@ -29,48 +22,52 @@
       function listener(e) {
         if (e.type === "beforetoggle") {
           if (e.newState === "open") {
-            ++showCount;
-            assert_equals(e.oldState,"closed",'The "beforetoggle" event should be fired before the popover is open');
+            assert_equals(e.currentState,"closed",'The "beforetoggle" event should be fired before the popover is open');
             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 {
-            ++hideCount;
             assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"');
-            assert_equals(e.oldState,"open",'The "beforetoggle" event should be fired before the popover is closed')
+            assert_equals(e.currentState,"open",'The "beforetoggle" event should be fired before the popover is 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;
           }
         } else {
-          assert_equals(e.type,"toggle",'Popover events should be "beforetoggle" and "toggle"')
+          assert_equals(e.type,"aftertoggle",'Popover events should be "beforetoggle" and "aftertoggle"')
           if (e.newState === "open") {
-            ++afterShowCount;
+            assert_equals(e.currentState,"open",'Aftertoggle should be fired after the popover is open');
             if (document.body.contains(e.target)) {
               assert_true(e.target.matches(':open'),'The popover should be in the :open state when the after opening event fires.');
               assert_false(e.target.matches(':closed'),'The popover should *not* be in the :closed state when the after opening event fires.');
             }
+            ++afterShowCount;
           } else {
-            ++afterHideCount;
             assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"');
+            assert_equals(e.currentState,"closed",'Aftertoggle should be fired after the popover is closed');
             assert_true(e.target.matches(':closed'),'The popover should be in the :closed state when the after hiding event fires.');
             assert_false(e.target.matches(':open'),'The popover should *not* be in the :open state when the after hiding event fires.');
+            ++afterHideCount;
           }
-          e.preventDefault(); // "toggle" should not be cancelable.
+          e.preventDefault(); // "aftertoggle" should not be cancelable.
         }
       };
       switch (method) {
         case "listener":
-          const {signal} = getPopoverAndSignal(t);
+          const controller = new AbortController();
+          const signal = controller.signal;
+          t.add_cleanup(() => controller.abort());
           // These events bubble.
           document.addEventListener('beforetoggle', listener, {signal});
-          document.addEventListener('toggle', listener, {signal});
+          document.addEventListener('aftertoggle', listener, {signal});
           break;
         case "attribute":
           assert_false(popover.hasAttribute('onbeforetoggle'));
           t.add_cleanup(() => popover.removeAttribute('onbeforetoggle'));
           popover.onbeforetoggle = listener;
-          assert_false(popover.hasAttribute('ontoggle'));
-          t.add_cleanup(() => popover.removeAttribute('ontoggle'));
-          popover.ontoggle = listener;
+          assert_false(popover.hasAttribute('onaftertoggle'));
+          t.add_cleanup(() => popover.removeAttribute('onaftertoggle'));
+          popover.onaftertoggle = listener;
           break;
         default: assert_unreached();
       }
@@ -85,7 +82,7 @@
       assert_equals(0,afterShowCount);
       assert_equals(0,afterHideCount);
       await waitForRender();
-      assert_equals(1,afterShowCount,'toggle show is fired asynchronously');
+      assert_equals(1,afterShowCount,'aftertoggle show is fired asynchronously');
       assert_equals(0,afterHideCount);
       assert_true(popover.matches(':open'));
       popover.hidePopover();
@@ -96,7 +93,7 @@
       assert_equals(0,afterHideCount);
       await waitForRender();
       assert_equals(1,afterShowCount);
-      assert_equals(1,afterHideCount,'toggle hide is fired asynchronously');
+      assert_equals(1,afterHideCount,'aftertoggle hide is fired asynchronously');
       // No additional events
       await waitForRender();
       await waitForRender();
@@ -109,7 +106,10 @@
   }
 
   promise_test(async t => {
-    const {popover,signal} = getPopoverAndSignal(t);
+    const popover = document.querySelector('[popover]');
+    const controller = new AbortController();
+    const signal = controller.signal;
+    t.add_cleanup(() => controller.abort());
     let cancel = true;
     popover.addEventListener('beforetoggle',(e) => {
       if (e.newState !== "open")
@@ -128,7 +128,10 @@
   }, 'The "beforetoggle" event is cancelable for the "opening" transition');
 
   promise_test(async t => {
-    const {popover,signal} = getPopoverAndSignal(t);
+    const popover = document.querySelector('[popover]');
+    const controller = new AbortController();
+    const signal = controller.signal;
+    t.add_cleanup(() => {controller.abort();});
     popover.addEventListener('beforetoggle',(e) => {
       assert_not_equals(e.newState,"closed",'The "beforetoggle" event was fired for the closing transition');
     }, {signal});
@@ -141,74 +144,5 @@
     await waitForRender(); // Check for async events also
     assert_false(popover.matches(':open'));
   }, 'The "beforetoggle" event is not fired for element removal');
-
-  promise_test(async t => {
-    const {popover,signal} = getPopoverAndSignal(t);
-    let events;
-    function resetEvents() {
-      events = {
-        singleShow: false,
-        singleHide: false,
-        coalescedShow: false,
-        coalescedHide: false,
-      };
-    }
-    function setEvent(type) {
-      assert_equals(events[type],false,'event repeated');
-      events[type] = true;
-    }
-    function assertOnly(type,msg) {
-      Object.keys(events).forEach(val => {
-        assert_equals(events[val],val===type,`${msg} (${val})`);
-      });
-    }
-    popover.addEventListener('toggle',(e) => {
-      switch (e.newState) {
-        case "open":
-          switch (e.oldState) {
-            case "open": setEvent('coalescedShow'); break;
-            case "closed": setEvent('singleShow'); break;
-            default: assert_unreached();
-          }
-          break;
-        case "closed":
-          switch (e.oldState) {
-            case "closed": setEvent('coalescedHide'); break;
-            case "open": setEvent('singleHide'); break;
-            default: assert_unreached();
-          }
-          break;
-        default: assert_unreached();
-      }
-    }, {signal});
-
-    resetEvents();
-    assertOnly('none');
-    assert_false(popover.matches(':open'));
-    popover.showPopover();
-    await waitForRender();
-    assert_true(popover.matches(':open'));
-    assertOnly('singleShow','Single event should have been fired, which is a "show"');
-
-    resetEvents();
-    popover.hidePopover();
-    popover.showPopover(); // Immediate re-show
-    await waitForRender();
-    assert_true(popover.matches(':open'));
-    assertOnly('coalescedShow','Single coalesced event should have been fired, which is a "show"');
-
-    resetEvents();
-    popover.hidePopover();
-    await waitForRender();
-    assertOnly('singleHide','Single event should have been fired, which is a "hide"');
-    assert_false(popover.matches(':open'));
-
-    resetEvents();
-    popover.showPopover();
-    popover.hidePopover(); // Immediate re-hide
-    await waitForRender();
-    assertOnly('coalescedHide','Single coalesced event should have been fired, which is a "hide"');
-    assert_false(popover.matches(':open'));
-  }, 'The "toggle" event is coalesced');
 };
 </script>