Add test for wheel event groups #37445

dlrobertson · 2022-12-10T00:35:02Z

Add tests for groups of wheel events.

dlrobertson · 2022-12-10T01:09:04Z

Test	Blink	WebKit	Gecko
Wheel events are grouped and are not sent to a child element	✅	✅	❌
Wheel events are not bound to a scroll frame	✅	✅	✅
Wheel event groups beyond the origin bound have the same target	✅	❌	❌

As stated in w3c/uievents#338 the WebKit failure in my local tests is due to a webdriver exception, but follows the expected behavior with manual tests.

masayuki-nakano · 2022-12-12T01:58:54Z

dom/events/scrolling/wheel-event-groups.html

+      .scroll(40, 2, 0, 20, {origin: firstSpacer})
+      .scroll(40, 2, 0, 100, {origin: firstSpacer})
+      .pause(1)
+      .scroll(40, 2, 0, 50, {origin: firstSpacer})


In my understanding, firstSpacer is now scrolled out after the previous .scroll call. Does this call emulate user input in any browsers?

Yeah, this emulates what happens for several small wheel scrolls that start on firstSpacer that happen in quick succession. I'll add a test that doesn't add an origin for additional coverage.

Added a test that does what this test did without an element as an origin. It intermittently fails on WebKit, but the behavior is always the same for a manual test.

masayuki-nakano · 2022-12-12T02:05:18Z

dom/events/scrolling/wheel-event-groups.html

+    assert_equals(second_spacer_wheel_event, false, "Wheel event should not be fired to the second spacer");
+    assert_equals(inner_wheel_event, false, "Wheel event should not be fired to inner div");


At least under current UI Events spec, one of these should be true if the last .scroll call emulates a wheel operation on secondSpacer or innerDiv because both of them are in same scrollable element.

This is also my understanding of the current spec. This is what I was trying to describe in w3c/uievents#338, but I'm not sure if I described it well. For both manual and emulated tests, the current spec behavior does not seem to happen on WebKit or Blink.

Note that a event sent to secondSpacer when the scroll moves beyond the bounds of firstSpacer would cause user scrolls to be interrupted in the case that secondSpacer has a event listener that uses preventDefault. We get a steady stream of bug reports for this in the Gecko APZ component. So maybe it makes the most sense to fire the wheel event at the parent that contains both elements instead of firing it at the initial target and relying on the event bubbling up.

Sorry for the delay to reply, but I've still not reached the conclusion yet.

Could you rewrite these tests to collect Event.target of wheel events with window.addEventListener into an array and compare each array item with what you expect? Then, what's expected here becomes easier to understand. I.e.,

let wheelEventTargets = []; window.addEventListener("wheel", event => { wheelEventTargets.push(event.target); }, {passive: true}); ... function runTest() { wheelEventTargets = []; ... assert_equal( document.getElementById("..."), wheelEventTargets[0], "..." ); assert_equal( document.getElementById("..."), wheelEventTargets[1], "..." ); }

(I guess and assume that the number of wheel events is not different between browsers in what this test does.)

Sorry for the delay to reply, but I've still not reached the conclusion yet.

No problem! This is a bit of a complex topic that (I think) doesn't currently follow the spec.

Could you rewrite these tests to collect Event.target of wheel events with window.addEventListener into an array and compare each array item with what you expect?

Thanks for the feedback! Yeah, that makes sense... I think I can rework the test to work like this. Some of the way the tests are architected is an artifact of me trying to figure out how non-firefox browsers handle wheel event group targetting.

dlrobertson

Thanks for the review! Hopefully this version makes it easier to reason about... if it doesn't, let me know and I can think about altering things to make it more readable.

I can also create a example for manual testing if it would help.

dlrobertson · 2023-01-13T21:28:13Z

dom/events/scrolling/wheel-event-groups.html

+      testInfo.scrollingElement.scrollTo({top: 0, left: 0, behavior: "instant"});
+
+      wheelEventTargetsEqual(testInfo.targetElement,
+                             "All wheel events in the group have the same element");


Could you rewrite these tests to collect Event.target of wheel events with window.addEventListener into an array and compare each array item with what you expect?

Went about doing this in a slightly different manner. All the wheel event targets for a given test should be in the same wheel event group, so they all should be the same. As a result, we don't need to check wheelEventTargets[0] == expectedTarget, we can just iterate through all the wheel event targets we collected in the testcase and ensure they're the expected target.

dlrobertson · 2023-01-13T21:28:54Z

dom/events/scrolling/wheel-event-groups.html

+    targetElement: secondInnerSpacer,
+    title: 'Wheel event groups beyond the origin bound have the same target',
+  },
+];


Used a array of test info instead of separate test definitions to make it easier to reason about the test cases.

dlrobertson

I also wanted to add a test for a moved element, but that turns out to be a bit more complicated and the tests get super flaky, so I'm not really sure what the correct behavior is.

dlrobertson · 2023-01-18T02:21:52Z

dom/events/scrolling/wheel-event-groups.html

+                           "All wheel events in the group have the same target");
+
+    document.removeEventListener("wheel", onEventRemoveInitialTarget, {passive: true});
+  }, "Remove the initial wheel event target.");


Added a simple test that removes the initial target on the first wheel event. Note that I assumed chromes behavior for this test because it was easier to test. I'm unsure if this is the right behavior.

dlrobertson · 2023-01-19T18:28:50Z

I also wanted to add a test for a moved element, but that turns out to be a bit more complicated and the tests get super flaky, so I'm not really sure what the correct behavior is.

Reorganized the tests and rewrote the removal/move initial target tests to greatly improve their reliability. All tests pass very reliably for chrome and firefox (with the bug 1168182 patch) for me on the main box I'm using to work on this. I'll try to run the tests on WebKit and maybe other OSes over the next few days.

dlrobertson · 2023-01-27T16:49:49Z

Added dom/events/scrolling/wheel-event-groups-target-elements.html which ensures the text is not the event target for the wheel event group. Blink handles this correctly, but I'm currently unable to test on WebKit. This is the last failing test (that I know of 😆) with D163484 for Gecko.

dlrobertson · 2023-02-06T16:30:13Z

CC: @smaug----

smaug---- · 2023-02-07T14:12:31Z

dom/events/scrolling/wheel-event-groups-target-move.html

+
+    // The first wheel event should be targetted at the moved element.
+    assert_equals(wheelEventTargets.shift(), initialTarget,
+                  "Initial wheel event is has the moved element as the target");


dlrobertson

Thanks for the review! Added new test that exercises the display: none case.

dlrobertson · 2023-02-08T13:23:49Z

dom/events/scrolling/wheel-event-groups-target-move.html

+
+    // The first wheel event should be targetted at the moved element.
+    assert_equals(wheelEventTargets.shift(), initialTarget,
+                  "Initial wheel event is has the moved element as the target");


dlrobertson · 2023-02-08T13:51:13Z

dom/events/scrolling/wheel-event-groups-target-removal.html

+    wheelEventTargets.forEach((wheelEventTarget) => {
+      assert_not_equals(wheelEventTarget, initialTarget,
+                        "All following wheel events are not targetting the removed element");
+    });


Note that browser engines do different things in the event target mutation test cases. E.g. Gecko and WebKit will essentially fallback to the non wheel event groups event target after the original event target is mutated in some way, while Blink will only retarget once. I greatly weakened this assertion, so that all engines should pass. We might want to circle back at some point once there is some sort of consensus.

masayuki-nakano · 2023-02-19T01:16:45Z

dom/events/scrolling/wheel-event-groups-target-resize.html

+        e.target.style.height = '10px';
+      }
+    }
+    window.addEventListener("wheel", resizeInitialElement, {passive: true});


I wonder, passive value could change the behavior due to optimization in some browsers. Did you check the false case?

Could be nicer to check both passive: true and passive: false cases in variants.

masayuki-nakano · 2023-02-19T01:19:11Z

dom/events/scrolling/wheel-event-groups-basic.html

+    scrollY: 2,
+    scrollingElement: document.scrollingElement,
+    targetElement: firstRootSpacer,
+    title: 'Wheel events are grouped and are not targetted at following elements.',


FYI: using "should" or "shouldn't" make the expectation clearer for everybody.

masayuki-nakano · 2023-02-19T01:20:44Z

dom/events/scrolling/wheel-event-groups-basic.html

+      // Wait some extra time to ensure nothing from the prior test impacts this one.
+      // At a minimum this should be greater than 500ms to ensure that the wheel events
+      // in this test are in a new wheel event group
+      await new Promise(resolve => step_timeout(resolve, 1000));


Is this really required in some browsers? It's odd to work a wheel transaction across browsing contexts.

Yeah, I was also confused by this. Spent some time investigating this and the issue was partially solved by using variants. It turns out these tests also need to wait for the compositor to be ready waitForCompositorReady().

masayuki-nakano · 2023-02-19T01:25:57Z

dom/events/scrolling/wheel-event-groups-basic.html

+      wheelEventTargets.forEach((wheelEventTarget) => {
+        assert_equals(wheelEventTarget, testInfo.targetElement,
+                      "All wheel events in the group have the same target");
+      });


It's unclear which index's result is wrong. Please update the assertion comment to let developers know the failure point.

masayuki-nakano · 2023-02-19T01:27:58Z

dom/events/scrolling/wheel-event-groups-basic.html

+
+      await waitForCompositorCommit();
+
+      wheelEventTargets = [];


Why don't you define the variable at this line?

masayuki-nakano · 2023-02-19T01:35:15Z

dom/events/scrolling/wheel-event-groups-target-elements.html

+function runTest() {
+  promise_test(async (t) => {


I wonder, if you do:

promise_test(async t => { await new Promise(resolve => addEventListener("load", resolve, {once:true}); ...

Then, you can reduce one indent level of the test body.

masayuki-nakano · 2023-02-19T01:36:13Z

dom/events/scrolling/wheel-event-groups-target-elements.html

+    wheelEventTargets.forEach((wheelEventTarget) => {
+      assert_equals(wheelEventTarget, firstRootSpacer,
+                    "All following wheel events in the group have the same target");
+    });


Ditto, please update the assertion message to let developers know which index failed.

masayuki-nakano · 2023-02-19T01:36:55Z

dom/events/scrolling/wheel-event-groups-target-move.html

+    wheelEventTargets.forEach((wheelEventTarget) => {
+      assert_not_equals(wheelEventTarget, initialTarget,
+                        "All following wheel events are not targetting the moved element");
+    });


Ditto please change the assertion message for each.

masayuki-nakano · 2023-02-19T01:37:16Z

dom/events/scrolling/wheel-event-groups-target-removal.html

+      assert_not_equals(wheelEventTarget, initialTarget,
+                        "All following wheel events are not targetting the removed element");


masayuki-nakano · 2023-02-19T01:38:21Z

dom/events/scrolling/wheel-event-groups-target-resize.html

+      assert_equals(wheelEventTarget, initialTarget,
+                    "All wheel events should target the initial element");


Ditto update the assertion message for each.

masayuki-nakano · 2023-02-19T01:43:05Z

dom/events/scrolling/wheel-event-groups-target-resize.html

+    await waitForAnimationEnd(() => { return document.scrollingElement.scrollTop; });
+    await waitForCompositorCommit();
+
+    wheelEventTargets.forEach((wheelEventTarget) => {


I wonder, scroll top should be checked whether it's still 0 or not.

Probably worthwhile

smaug----

We might want to put all the filed under tentative/ just because there is no spec for this. But other than that looks good

dlrobertson · 2023-09-22T13:25:44Z

We might want to put all the filed under tentative/ just because there is no spec for this. But other than that looks good

Renamed all tests to <test-name>.tentative.html. Good suggestion

dom/events/scrolling/wheel-event-transactions-basic.tentative.html

dom/events/scrolling/wheel-event-transactions-multiple-action-chains.tentative.html

masayuki-nakano · 2023-09-24T03:21:08Z

dom/events/scrolling/wheel-event-transactions-basic.tentative.html

+<head>
+<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">


Please always add <meta charset="utf-8"> to skip maybe expensive auto-detection of the browsers.

And shouldn't this test have <meta name="timeout" content="long"> for avoiding unexpected timeout during the test?

masayuki-nakano · 2023-09-24T03:21:22Z

dom/events/scrolling/wheel-event-transactions-multiple-action-chains.tentative.html

+<head>
+<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">


masayuki-nakano · 2023-09-24T03:21:41Z

dom/events/scrolling/wheel-event-transactions-target-display-change.tentative.html

+<head>
+<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">


masayuki-nakano · 2023-09-24T03:22:07Z

dom/events/scrolling/wheel-event-transactions-target-removal.tentative.html

+<head>
+<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">


masayuki-nakano · 2023-09-24T03:22:15Z

dom/events/scrolling/wheel-event-transactions-target-resize.tentative.html

+<head>
+<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">


dom/events/scrolling/wheel-event-transactions-target-display-change.tentative.html

dom/events/scrolling/wheel-event-transactions-target-removal.tentative.html

whimboo · 2023-11-28T07:49:25Z

Hi @dlrobertson. Do you plan to update this PR? It might be nice to have those tests for the upcoming work for widget level events in Marionette. Thanks.

dlrobertson · 2023-12-04T17:27:28Z

@whimboo Updated and resolved all comments. Thanks for the ping!

zcorpan · 2023-12-05T13:00:00Z

Re tentative, now there is a spec since w3c/uievents#338 was merged.

dlrobertson · 2023-12-06T15:47:09Z

Re tentative, now there is a spec since w3c/uievents#338 was merged.

👍 Thanks for the note about this. Forgot that I'd need to update this.

Also it looks like chrome no longer passes the tests reliably. I'll have to dig into the tests to figure out what changed.

dlrobertson · 2023-12-06T15:48:36Z

@zcorpan Actually to what extent should I rename things? For example, AFAIK it isn't explicitly specified what should happen when the initial target of the wheel transaction is removed, but I have a test for that in this PR.

zcorpan · 2023-12-06T16:26:20Z

Keep tentative for any tests where there's no specified correct behavior, and file a spec issue. Bonus points for pointing to the spec issue in the test (with <link rel=help>).

whimboo · 2024-04-09T08:23:13Z

Hi @dlrobertson. Do you have an update for this PR?

whimboo · 2024-06-04T07:14:30Z

@masayuki-nakano could you please take another look. It looks like that after the last push it was missed to request review again. Thanks.

masayuki-nakano · 2024-06-19T07:09:04Z

dom/events/scrolling/wheel-event-transactions-target-removal.html

+  await waitForCompositorCommit();
+
+  initialTarget.addEventListener("wheel", () => {
+    assert_true(false, "wheel event should never be fired after the target is removed")


I don't understand this test. Looks like that you add this event listener after all events are fired. Don't you need to do this in the first wheel event listener, removeInitialElement?

Good catch! Updated to set this in the first wheel event listener.

masayuki-nakano

Okay, looks fine although I've not checked actual changes strictly because it seems that it's impossible to get the diff from previous version :-(

Well, I'd be happy if you'd add <meta charset="utf-8"> (to skip running encode detection) to all tests for quicker run in CI, but up to you.

dlrobertson · 2024-07-18T21:41:06Z

Awesome! I'll update it asap

Add tests for groups of wheel events.

dlrobertson · 2024-07-19T12:27:50Z

Updated!

whimboo · 2024-08-08T19:29:20Z

@dlrobertson I assume that this PR doesn't need another review, so what is the merge blocked on?

dlrobertson · 2024-08-11T16:32:20Z

Merged! Just wasn't sure if I was supposed to merge it, or if I was supposed to wait for a reviewer to merge it. Thanks for the ping!

wpt-pr-bot added the dom label Dec 10, 2022

wpt-pr-bot assigned jdm Dec 10, 2022

wpt-pr-bot requested review from annevk, jdm and zqzhang December 10, 2022 00:35

dlrobertson mentioned this pull request Dec 10, 2022

Wheel event groups are not defined w3c/uievents#338

Closed

masayuki-nakano reviewed Dec 12, 2022

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch from 2625a56 to 7876bd1 Compare December 15, 2022 21:46

dlrobertson force-pushed the wheel-event-groups branch from 7876bd1 to dbf4b63 Compare January 13, 2023 21:22

dlrobertson commented Jan 13, 2023

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch 2 times, most recently from 9bca6f9 to ce7b93f Compare January 18, 2023 02:20

dlrobertson commented Jan 18, 2023

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch from ce7b93f to 2dd2f45 Compare January 19, 2023 18:24

dlrobertson force-pushed the wheel-event-groups branch from 2dd2f45 to 9884b2b Compare January 19, 2023 20:22

dlrobertson force-pushed the wheel-event-groups branch from 9884b2b to 9e28eb2 Compare January 27, 2023 16:45

dlrobertson force-pushed the wheel-event-groups branch 2 times, most recently from aad9b4d to 18e23c3 Compare January 27, 2023 18:39

smaug---- reviewed Feb 7, 2023

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch 2 times, most recently from 3271944 to 2effcbd Compare February 8, 2023 13:47

dlrobertson commented Feb 8, 2023

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch from 2effcbd to 69a7b66 Compare February 17, 2023 18:33

masayuki-nakano requested changes Feb 19, 2023

View reviewed changes

masayuki-nakano reviewed Feb 19, 2023

View reviewed changes

smaug---- approved these changes Sep 20, 2023

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch from 598c501 to eb993ee Compare September 21, 2023 12:42

masayuki-nakano reviewed Sep 24, 2023

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch 3 times, most recently from 2691a25 to b4d5721 Compare December 4, 2023 17:25

dlrobertson force-pushed the wheel-event-groups branch from b4d5721 to 93c1d44 Compare May 13, 2024 12:21

whimboo requested a review from masayuki-nakano June 4, 2024 07:13

masayuki-nakano requested changes Jun 19, 2024

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch from 93c1d44 to 7c500ba Compare June 24, 2024 21:41

whimboo requested a review from masayuki-nakano July 3, 2024 06:54

masayuki-nakano approved these changes Jul 17, 2024

View reviewed changes

Add test for wheel event groups

8b9432a

Add tests for groups of wheel events.

dlrobertson force-pushed the wheel-event-groups branch from 7c500ba to 8b9432a Compare July 19, 2024 12:27

dlrobertson merged commit ee15c2a into web-platform-tests:master Aug 11, 2024
19 checks passed

moz-wptsync-bot removed the mozilla:gecko-blocked label Aug 12, 2024

dlrobertson deleted the wheel-event-groups branch August 13, 2024 07:08

		assert_equals(second_spacer_wheel_event, false, "Wheel event should not be fired to the second spacer");
		assert_equals(inner_wheel_event, false, "Wheel event should not be fired to inner div");

		assert_not_equals(wheelEventTarget, initialTarget,
		"All following wheel events are not targetting the removed element");

		assert_equals(wheelEventTarget, initialTarget,
		"All wheel events should target the initial element");

		<head>
		<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">

Add test for wheel event groups #37445

Add test for wheel event groups #37445

Conversation

dlrobertson commented Dec 10, 2022

dlrobertson commented Dec 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson commented Jan 19, 2023

dlrobertson commented Jan 27, 2023 • edited Loading

dlrobertson commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaug---- left a comment

Choose a reason for hiding this comment

dlrobertson commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Nov 28, 2023

dlrobertson commented Dec 4, 2023

zcorpan commented Dec 5, 2023

dlrobertson commented Dec 6, 2023

dlrobertson commented Dec 6, 2023

zcorpan commented Dec 6, 2023

whimboo commented Apr 9, 2024

whimboo commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayuki-nakano left a comment

Choose a reason for hiding this comment

dlrobertson commented Jul 18, 2024

dlrobertson commented Jul 19, 2024

whimboo commented Aug 8, 2024

dlrobertson commented Aug 11, 2024

dlrobertson commented Jan 27, 2023 •

edited

Loading