Skip to content

Commit c19821c

Browse files
philloooomarcoscaceres
authored andcommitted
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 <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: 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 <phillis@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: Phillis Tang <phillis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1098927}
1 parent 764dd5b commit c19821c

File tree

4 files changed

+120
-186
lines changed

4 files changed

+120
-186
lines changed

html/semantics/popovers/idlharness.tentative.html

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,12 @@
4141
'document.getElementById("b3")',
4242
],
4343
BeforeToggleEvent: [
44-
'new ToggleEvent("beforetoggle")',
45-
'new ToggleEvent("beforetoggle", {oldState: "open"})',
46-
'new ToggleEvent("beforetoggle", {oldState: "open",newState: "open"})',
47-
'new ToggleEvent("toggle")',
48-
'new ToggleEvent("toggle", {oldState: "open"})',
49-
'new ToggleEvent("toggle", {oldState: "open",newState: "open"})',
44+
'new PopoverToggleEvent("beforetoggle")',
45+
'new PopoverToggleEvent("beforetoggle", {currentState: "open"})',
46+
'new PopoverToggleEvent("beforetoggle", {currentState: "open",newState: "open"})',
47+
'new PopoverToggleEvent("aftertoggle")',
48+
'new PopoverToggleEvent("aftertoggle", {currentState: "open"})',
49+
'new PopoverToggleEvent("aftertoggle", {currentState: "open",newState: "open"})',
5050
],
5151
});
5252
}

html/semantics/popovers/popover-events.tentative.html

Lines changed: 27 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,6 @@
1010
<div popover>Popover</div>
1111

1212
<script>
13-
function getPopoverAndSignal(t) {
14-
const popover = document.querySelector('[popover]');
15-
const controller = new AbortController();
16-
const signal = controller.signal;
17-
t.add_cleanup(() => controller.abort());
18-
return {popover, signal};
19-
}
2013
window.onload = () => {
2114
for(const method of ["listener","attribute"]) {
2215
promise_test(async t => {
@@ -29,48 +22,52 @@
2922
function listener(e) {
3023
if (e.type === "beforetoggle") {
3124
if (e.newState === "open") {
32-
++showCount;
33-
assert_equals(e.oldState,"closed",'The "beforetoggle" event should be fired before the popover is open');
25+
assert_equals(e.currentState,"closed",'The "beforetoggle" event should be fired before the popover is open');
3426
assert_true(e.target.matches(':closed'),'The popover should be in the :closed state when the opening event fires.');
3527
assert_false(e.target.matches(':open'),'The popover should *not* be in the :open state when the opening event fires.');
28+
++showCount;
3629
} else {
37-
++hideCount;
3830
assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"');
39-
assert_equals(e.oldState,"open",'The "beforetoggle" event should be fired before the popover is closed')
31+
assert_equals(e.currentState,"open",'The "beforetoggle" event should be fired before the popover is closed')
4032
assert_true(e.target.matches(':open'),'The popover should be in the :open state when the hiding event fires.');
4133
assert_false(e.target.matches(':closed'),'The popover should *not* be in the :closed state when the hiding event fires.');
34+
++hideCount;
4235
}
4336
} else {
44-
assert_equals(e.type,"toggle",'Popover events should be "beforetoggle" and "toggle"')
37+
assert_equals(e.type,"aftertoggle",'Popover events should be "beforetoggle" and "aftertoggle"')
4538
if (e.newState === "open") {
46-
++afterShowCount;
39+
assert_equals(e.currentState,"open",'Aftertoggle should be fired after the popover is open');
4740
if (document.body.contains(e.target)) {
4841
assert_true(e.target.matches(':open'),'The popover should be in the :open state when the after opening event fires.');
4942
assert_false(e.target.matches(':closed'),'The popover should *not* be in the :closed state when the after opening event fires.');
5043
}
44+
++afterShowCount;
5145
} else {
52-
++afterHideCount;
5346
assert_equals(e.newState,"closed",'Popover toggleevent states should be "open" and "closed"');
47+
assert_equals(e.currentState,"closed",'Aftertoggle should be fired after the popover is closed');
5448
assert_true(e.target.matches(':closed'),'The popover should be in the :closed state when the after hiding event fires.');
5549
assert_false(e.target.matches(':open'),'The popover should *not* be in the :open state when the after hiding event fires.');
50+
++afterHideCount;
5651
}
57-
e.preventDefault(); // "toggle" should not be cancelable.
52+
e.preventDefault(); // "aftertoggle" should not be cancelable.
5853
}
5954
};
6055
switch (method) {
6156
case "listener":
62-
const {signal} = getPopoverAndSignal(t);
57+
const controller = new AbortController();
58+
const signal = controller.signal;
59+
t.add_cleanup(() => controller.abort());
6360
// These events bubble.
6461
document.addEventListener('beforetoggle', listener, {signal});
65-
document.addEventListener('toggle', listener, {signal});
62+
document.addEventListener('aftertoggle', listener, {signal});
6663
break;
6764
case "attribute":
6865
assert_false(popover.hasAttribute('onbeforetoggle'));
6966
t.add_cleanup(() => popover.removeAttribute('onbeforetoggle'));
7067
popover.onbeforetoggle = listener;
71-
assert_false(popover.hasAttribute('ontoggle'));
72-
t.add_cleanup(() => popover.removeAttribute('ontoggle'));
73-
popover.ontoggle = listener;
68+
assert_false(popover.hasAttribute('onaftertoggle'));
69+
t.add_cleanup(() => popover.removeAttribute('onaftertoggle'));
70+
popover.onaftertoggle = listener;
7471
break;
7572
default: assert_unreached();
7673
}
@@ -85,7 +82,7 @@
8582
assert_equals(0,afterShowCount);
8683
assert_equals(0,afterHideCount);
8784
await waitForRender();
88-
assert_equals(1,afterShowCount,'toggle show is fired asynchronously');
85+
assert_equals(1,afterShowCount,'aftertoggle show is fired asynchronously');
8986
assert_equals(0,afterHideCount);
9087
assert_true(popover.matches(':open'));
9188
popover.hidePopover();
@@ -96,7 +93,7 @@
9693
assert_equals(0,afterHideCount);
9794
await waitForRender();
9895
assert_equals(1,afterShowCount);
99-
assert_equals(1,afterHideCount,'toggle hide is fired asynchronously');
96+
assert_equals(1,afterHideCount,'aftertoggle hide is fired asynchronously');
10097
// No additional events
10198
await waitForRender();
10299
await waitForRender();
@@ -109,7 +106,10 @@
109106
}
110107

111108
promise_test(async t => {
112-
const {popover,signal} = getPopoverAndSignal(t);
109+
const popover = document.querySelector('[popover]');
110+
const controller = new AbortController();
111+
const signal = controller.signal;
112+
t.add_cleanup(() => controller.abort());
113113
let cancel = true;
114114
popover.addEventListener('beforetoggle',(e) => {
115115
if (e.newState !== "open")
@@ -128,7 +128,10 @@
128128
}, 'The "beforetoggle" event is cancelable for the "opening" transition');
129129

130130
promise_test(async t => {
131-
const {popover,signal} = getPopoverAndSignal(t);
131+
const popover = document.querySelector('[popover]');
132+
const controller = new AbortController();
133+
const signal = controller.signal;
134+
t.add_cleanup(() => {controller.abort();});
132135
popover.addEventListener('beforetoggle',(e) => {
133136
assert_not_equals(e.newState,"closed",'The "beforetoggle" event was fired for the closing transition');
134137
}, {signal});
@@ -141,74 +144,5 @@
141144
await waitForRender(); // Check for async events also
142145
assert_false(popover.matches(':open'));
143146
}, 'The "beforetoggle" event is not fired for element removal');
144-
145-
promise_test(async t => {
146-
const {popover,signal} = getPopoverAndSignal(t);
147-
let events;
148-
function resetEvents() {
149-
events = {
150-
singleShow: false,
151-
singleHide: false,
152-
coalescedShow: false,
153-
coalescedHide: false,
154-
};
155-
}
156-
function setEvent(type) {
157-
assert_equals(events[type],false,'event repeated');
158-
events[type] = true;
159-
}
160-
function assertOnly(type,msg) {
161-
Object.keys(events).forEach(val => {
162-
assert_equals(events[val],val===type,`${msg} (${val})`);
163-
});
164-
}
165-
popover.addEventListener('toggle',(e) => {
166-
switch (e.newState) {
167-
case "open":
168-
switch (e.oldState) {
169-
case "open": setEvent('coalescedShow'); break;
170-
case "closed": setEvent('singleShow'); break;
171-
default: assert_unreached();
172-
}
173-
break;
174-
case "closed":
175-
switch (e.oldState) {
176-
case "closed": setEvent('coalescedHide'); break;
177-
case "open": setEvent('singleHide'); break;
178-
default: assert_unreached();
179-
}
180-
break;
181-
default: assert_unreached();
182-
}
183-
}, {signal});
184-
185-
resetEvents();
186-
assertOnly('none');
187-
assert_false(popover.matches(':open'));
188-
popover.showPopover();
189-
await waitForRender();
190-
assert_true(popover.matches(':open'));
191-
assertOnly('singleShow','Single event should have been fired, which is a "show"');
192-
193-
resetEvents();
194-
popover.hidePopover();
195-
popover.showPopover(); // Immediate re-show
196-
await waitForRender();
197-
assert_true(popover.matches(':open'));
198-
assertOnly('coalescedShow','Single coalesced event should have been fired, which is a "show"');
199-
200-
resetEvents();
201-
popover.hidePopover();
202-
await waitForRender();
203-
assertOnly('singleHide','Single event should have been fired, which is a "hide"');
204-
assert_false(popover.matches(':open'));
205-
206-
resetEvents();
207-
popover.showPopover();
208-
popover.hidePopover(); // Immediate re-hide
209-
await waitForRender();
210-
assertOnly('coalescedHide','Single coalesced event should have been fired, which is a "hide"');
211-
assert_false(popover.matches(':open'));
212-
}, 'The "toggle" event is coalesced');
213147
};
214148
</script>

0 commit comments

Comments
 (0)