From e24e5570870c60ba45f6d043ce4ed3ca1f95e857 Mon Sep 17 00:00:00 2001
From: Mason Freed <masonf@chromium.org>
Date: Tue, 31 Jan 2023 11:35:39 -0800
Subject: [PATCH] Reland "Update popover post-toggle event naming and behavior"

This is a reland of commit 2e5ee120d7fc5df4b6c101e88d03a5f62a73d8b8

It seems that the event coalescing behavior changes cause a segfault
in the event dispatch code. This reland (compare to Patchset 1) just
includes the event renaming, and not the new behavior. I'll land
the new behavior in a separate CL to make it smaller if it gets
reverted again. This part should be safe.

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:
>   https://github.com/whatwg/html/pull/8717
>
> Bug: 1307772
> Change-Id: Iabc5a9093d7cef3bbd6e54e488d8e571c51ea568
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4195120
> Auto-Submit: Mason Freed <masonf@chromium.org>
> Commit-Queue: Joey Arhar <jarhar@chromium.org>
> Reviewed-by: Joey Arhar <jarhar@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1098728}

Bug: 1307772
Change-Id: Iede18bbf05516cec4ee881f1afc663c45380c82e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4205710
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1099397}
---
 .../popovers/idlharness.tentative.html        |  12 +-
 .../popovers/popover-events.tentative.html    | 120 ++++++++++---
 .../toggleevent-interface.tentative.html      | 164 +++++++++---------
 interfaces/popover.tentative.idl              |  10 +-
 4 files changed, 186 insertions(+), 120 deletions(-)

diff --git a/html/semantics/popovers/idlharness.tentative.html b/html/semantics/popovers/idlharness.tentative.html
index d1a258bb4ad3ac..1cab891261cfd4 100644
--- a/html/semantics/popovers/idlharness.tentative.html
+++ b/html/semantics/popovers/idlharness.tentative.html
@@ -41,12 +41,12 @@
         'document.getElementById("b3")',
       ],
       BeforeToggleEvent: [
-        '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"})',
+        '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"})',
       ],
     });
   }
diff --git a/html/semantics/popovers/popover-events.tentative.html b/html/semantics/popovers/popover-events.tentative.html
index b96a0f5a53f489..b299424570024c 100644
--- a/html/semantics/popovers/popover-events.tentative.html
+++ b/html/semantics/popovers/popover-events.tentative.html
@@ -10,6 +10,13 @@
 <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 => {
@@ -22,52 +29,48 @@
       function listener(e) {
         if (e.type === "beforetoggle") {
           if (e.newState === "open") {
-            assert_equals(e.currentState,"closed",'The "beforetoggle" event should be fired before the popover is open');
+            ++showCount;
+            assert_equals(e.oldState,"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.currentState,"open",'The "beforetoggle" event should be fired before the popover is closed')
+            assert_equals(e.oldState,"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,"aftertoggle",'Popover events should be "beforetoggle" and "aftertoggle"')
+          assert_equals(e.type,"toggle",'Popover events should be "beforetoggle" and "toggle"')
           if (e.newState === "open") {
-            assert_equals(e.currentState,"open",'Aftertoggle should be fired after the popover is open');
+            ++afterShowCount;
             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(); // "aftertoggle" should not be cancelable.
+          e.preventDefault(); // "toggle" should not be cancelable.
         }
       };
       switch (method) {
         case "listener":
-          const controller = new AbortController();
-          const signal = controller.signal;
-          t.add_cleanup(() => controller.abort());
+          const {signal} = getPopoverAndSignal(t);
           // These events bubble.
           document.addEventListener('beforetoggle', listener, {signal});
-          document.addEventListener('aftertoggle', listener, {signal});
+          document.addEventListener('toggle', listener, {signal});
           break;
         case "attribute":
           assert_false(popover.hasAttribute('onbeforetoggle'));
           t.add_cleanup(() => popover.removeAttribute('onbeforetoggle'));
           popover.onbeforetoggle = listener;
-          assert_false(popover.hasAttribute('onaftertoggle'));
-          t.add_cleanup(() => popover.removeAttribute('onaftertoggle'));
-          popover.onaftertoggle = listener;
+          assert_false(popover.hasAttribute('ontoggle'));
+          t.add_cleanup(() => popover.removeAttribute('ontoggle'));
+          popover.ontoggle = listener;
           break;
         default: assert_unreached();
       }
@@ -82,7 +85,7 @@
       assert_equals(0,afterShowCount);
       assert_equals(0,afterHideCount);
       await waitForRender();
-      assert_equals(1,afterShowCount,'aftertoggle show is fired asynchronously');
+      assert_equals(1,afterShowCount,'toggle show is fired asynchronously');
       assert_equals(0,afterHideCount);
       assert_true(popover.matches(':open'));
       popover.hidePopover();
@@ -93,7 +96,7 @@
       assert_equals(0,afterHideCount);
       await waitForRender();
       assert_equals(1,afterShowCount);
-      assert_equals(1,afterHideCount,'aftertoggle hide is fired asynchronously');
+      assert_equals(1,afterHideCount,'toggle hide is fired asynchronously');
       // No additional events
       await waitForRender();
       await waitForRender();
@@ -106,10 +109,7 @@
   }
 
   promise_test(async t => {
-    const popover = document.querySelector('[popover]');
-    const controller = new AbortController();
-    const signal = controller.signal;
-    t.add_cleanup(() => controller.abort());
+    const {popover,signal} = getPopoverAndSignal(t);
     let cancel = true;
     popover.addEventListener('beforetoggle',(e) => {
       if (e.newState !== "open")
@@ -128,10 +128,7 @@
   }, 'The "beforetoggle" event is cancelable for the "opening" transition');
 
   promise_test(async t => {
-    const popover = document.querySelector('[popover]');
-    const controller = new AbortController();
-    const signal = controller.signal;
-    t.add_cleanup(() => {controller.abort();});
+    const {popover,signal} = getPopoverAndSignal(t);
     popover.addEventListener('beforetoggle',(e) => {
       assert_not_equals(e.newState,"closed",'The "beforetoggle" event was fired for the closing transition');
     }, {signal});
@@ -144,5 +141,74 @@
     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>
diff --git a/html/semantics/popovers/toggleevent-interface.tentative.html b/html/semantics/popovers/toggleevent-interface.tentative.html
index 4d437b0c0ad523..45709458b00c46 100644
--- a/html/semantics/popovers/toggleevent-interface.tentative.html
+++ b/html/semantics/popovers/toggleevent-interface.tentative.html
@@ -7,200 +7,200 @@
 
 <script>
 test(function() {
-  var event = new PopoverToggleEvent("");
-  assert_true(event instanceof window.PopoverToggleEvent);
-}, "the event is an instance of PopoverToggleEvent");
+  var event = new ToggleEvent("");
+  assert_true(event instanceof window.ToggleEvent);
+}, "the event is an instance of ToggleEvent");
 
 test(function() {
-  var event = new PopoverToggleEvent("");
+  var event = new ToggleEvent("");
   assert_true(event instanceof window.Event);
 }, "the event inherts from Event");
 
 test(function() {
   assert_throws_js(TypeError, function() {
-    new PopoverToggleEvent();
+    new ToggleEvent();
   }, 'First argument (type) is required, so was expecting a TypeError.');
 }, 'Missing type argument');
 
 test(function() {
-  var event = new PopoverToggleEvent("test");
+  var event = new ToggleEvent("test");
   assert_equals(event.type, "test");
 }, "type argument is string");
 
 test(function() {
-  var event = new PopoverToggleEvent(null);
+  var event = new ToggleEvent(null);
   assert_equals(event.type, "null");
 }, "type argument is null");
 
 test(function() {
-  var event = new PopoverToggleEvent(undefined);
+  var event = new ToggleEvent(undefined);
   assert_equals(event.type, "undefined");
 }, "event type set to undefined");
 
 test(function() {
-  var event = new PopoverToggleEvent("test");
-  assert_equals(event.currentState, "");
-}, "currentState has default value of empty string");
+  var event = new ToggleEvent("test");
+  assert_equals(event.oldState, "");
+}, "oldState has default value of empty string");
 
 test(function() {
-  var event = new PopoverToggleEvent("test");
-  assert_readonly(event, "currentState", "readonly attribute value");
-}, "currentState is readonly");
+  var event = new ToggleEvent("test");
+  assert_readonly(event, "oldState", "readonly attribute value");
+}, "oldState is readonly");
 
 test(function() {
-  var event = new PopoverToggleEvent("test");
+  var event = new ToggleEvent("test");
   assert_equals(event.newState, "");
 }, "newState has default value of empty string");
 
 test(function() {
-  var event = new PopoverToggleEvent("test");
+  var event = new ToggleEvent("test");
   assert_readonly(event, "newState", "readonly attribute value");
 }, "newState is readonly");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", null);
-  assert_equals(event.currentState, "");
+  var event = new ToggleEvent("test", null);
+  assert_equals(event.oldState, "");
   assert_equals(event.newState, "");
-}, "PopoverToggleEventInit argument is null");
+}, "ToggleEventInit argument is null");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", undefined);
-  assert_equals(event.currentState, "");
+  var event = new ToggleEvent("test", undefined);
+  assert_equals(event.oldState, "");
   assert_equals(event.newState, "");
-}, "PopoverToggleEventInit argument is undefined");
+}, "ToggleEventInit argument is undefined");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {});
-  assert_equals(event.currentState, "");
+  var event = new ToggleEvent("test", {});
+  assert_equals(event.oldState, "");
   assert_equals(event.newState, "");
-}, "PopoverToggleEventInit argument is empty dictionary");
+}, "ToggleEventInit argument is empty dictionary");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: "sample"});
-  assert_equals(event.currentState, "sample");
-}, "currentState set to 'sample'");
+  var event = new ToggleEvent("test", {oldState: "sample"});
+  assert_equals(event.oldState, "sample");
+}, "oldState set to 'sample'");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: undefined});
-  assert_equals(event.currentState, "");
-}, "currentState set to undefined");
+  var event = new ToggleEvent("test", {oldState: undefined});
+  assert_equals(event.oldState, "");
+}, "oldState set to undefined");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: null});
-  assert_equals(event.currentState, "null");
-}, "currentState set to null");
+  var event = new ToggleEvent("test", {oldState: null});
+  assert_equals(event.oldState, "null");
+}, "oldState set to null");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: false});
-  assert_equals(event.currentState, "false");
-}, "currentState set to false");
+  var event = new ToggleEvent("test", {oldState: false});
+  assert_equals(event.oldState, "false");
+}, "oldState set to false");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: true});
-  assert_equals(event.currentState, "true");
-}, "currentState set to true");
+  var event = new ToggleEvent("test", {oldState: true});
+  assert_equals(event.oldState, "true");
+}, "oldState set to true");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: 0.5});
-  assert_equals(event.currentState, "0.5");
-}, "currentState set to a number");
+  var event = new ToggleEvent("test", {oldState: 0.5});
+  assert_equals(event.oldState, "0.5");
+}, "oldState set to a number");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: []});
-  assert_equals(event.currentState, "");
-}, "currentState set to []");
+  var event = new ToggleEvent("test", {oldState: []});
+  assert_equals(event.oldState, "");
+}, "oldState set to []");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: [1, 2, 3]});
-  assert_equals(event.currentState, "1,2,3");
-}, "currentState set to [1, 2, 3]");
+  var event = new ToggleEvent("test", {oldState: [1, 2, 3]});
+  assert_equals(event.oldState, "1,2,3");
+}, "oldState set to [1, 2, 3]");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {currentState: {sample: 0.5}});
-  assert_equals(event.currentState, "[object Object]");
-}, "currentState set to an object");
+  var event = new ToggleEvent("test", {oldState: {sample: 0.5}});
+  assert_equals(event.oldState, "[object Object]");
+}, "oldState set to an object");
 
 test(function() {
-  var event = new PopoverToggleEvent("test",
-      {currentState: {valueOf: function () { return 'sample'; }}});
-  assert_equals(event.currentState, "[object Object]");
-}, "currentState set to an object with a valueOf function");
+  var event = new ToggleEvent("test",
+      {oldState: {valueOf: function () { return 'sample'; }}});
+  assert_equals(event.oldState, "[object Object]");
+}, "oldState set to an object with a valueOf function");
 
 test(function() {
-  var eventInit = {currentState: "sample",newState: "sample2"};
-  var event = new PopoverToggleEvent("test", eventInit);
-  assert_equals(event.currentState, "sample");
+  var eventInit = {oldState: "sample",newState: "sample2"};
+  var event = new ToggleEvent("test", eventInit);
+  assert_equals(event.oldState, "sample");
   assert_equals(event.newState, "sample2");
-}, "PopoverToggleEventInit properties set value");
+}, "ToggleEventInit properties set value");
 
 test(function() {
-  var eventInit = {currentState: "open",newState: "closed"};
-  var event = new PopoverToggleEvent("beforetoggle", eventInit);
-  assert_equals(event.currentState, "open");
+  var eventInit = {oldState: "open",newState: "closed"};
+  var event = new ToggleEvent("beforetoggle", eventInit);
+  assert_equals(event.oldState, "open");
   assert_equals(event.newState, "closed");
-}, "PopoverToggleEventInit properties set value 2");
+}, "ToggleEventInit properties set value 2");
 
 test(function() {
-  var eventInit = {currentState: "closed",newState: "open"};
-  var event = new PopoverToggleEvent("aftertoggle", eventInit);
-  assert_equals(event.currentState, "closed");
+  var eventInit = {oldState: "closed",newState: "open"};
+  var event = new ToggleEvent("toggle", eventInit);
+  assert_equals(event.oldState, "closed");
   assert_equals(event.newState, "open");
-}, "PopoverToggleEventInit properties set value 3");
+}, "ToggleEventInit properties set value 3");
 
 test(function() {
-  var eventInit = {currentState: "open",newState: "open"};
-  var event = new PopoverToggleEvent("beforetoggle", eventInit);
-  assert_equals(event.currentState, "open");
+  var eventInit = {oldState: "open",newState: "open"};
+  var event = new ToggleEvent("beforetoggle", eventInit);
+  assert_equals(event.oldState, "open");
   assert_equals(event.newState, "open");
-}, "PopoverToggleEventInit properties set value 4");
+}, "ToggleEventInit properties set value 4");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: "sample"});
+  var event = new ToggleEvent("test", {newState: "sample"});
   assert_equals(event.newState, "sample");
 }, "newState set to 'sample'");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: undefined});
+  var event = new ToggleEvent("test", {newState: undefined});
   assert_equals(event.newState, "");
 }, "newState set to undefined");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: null});
+  var event = new ToggleEvent("test", {newState: null});
   assert_equals(event.newState, "null");
 }, "newState set to null");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: false});
+  var event = new ToggleEvent("test", {newState: false});
   assert_equals(event.newState, "false");
 }, "newState set to false");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: true});
+  var event = new ToggleEvent("test", {newState: true});
   assert_equals(event.newState, "true");
 }, "newState set to true");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: 0.5});
+  var event = new ToggleEvent("test", {newState: 0.5});
   assert_equals(event.newState, "0.5");
 }, "newState set to a number");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: []});
+  var event = new ToggleEvent("test", {newState: []});
   assert_equals(event.newState, "");
 }, "newState set to []");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: [1, 2, 3]});
+  var event = new ToggleEvent("test", {newState: [1, 2, 3]});
   assert_equals(event.newState, "1,2,3");
 }, "newState set to [1, 2, 3]");
 
 test(function() {
-  var event = new PopoverToggleEvent("test", {newState: {sample: 0.5}});
+  var event = new ToggleEvent("test", {newState: {sample: 0.5}});
   assert_equals(event.newState, "[object Object]");
 }, "newState set to an object");
 
 test(function() {
-  var event = new PopoverToggleEvent("test",
+  var event = new ToggleEvent("test",
       {newState: {valueOf: function () { return 'sample'; }}});
   assert_equals(event.newState, "[object Object]");
 }, "newState set to an object with a valueOf function");
diff --git a/interfaces/popover.tentative.idl b/interfaces/popover.tentative.idl
index bf23c7641d94e8..4e8c85f7dacb14 100644
--- a/interfaces/popover.tentative.idl
+++ b/interfaces/popover.tentative.idl
@@ -13,13 +13,13 @@ interface mixin PopoverTargetElement {
 HTMLInputElement includes PopoverTargetElement;
 HTMLButtonElement includes PopoverTargetElement;
 
-interface PopoverToggleEvent : Event {
-    constructor(DOMString type, optional PopoverToggleEventInit eventInitDict = {});
-    readonly attribute DOMString currentState;
+interface ToggleEvent : Event {
+    constructor(DOMString type, optional ToggleEventInit eventInitDict = {});
+    readonly attribute DOMString oldState;
     readonly attribute DOMString newState;
 };
 
-dictionary PopoverToggleEventInit : EventInit {
-    DOMString currentState = "";
+dictionary ToggleEventInit : EventInit {
+    DOMString oldState = "";
     DOMString newState = "";
 };