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

dlrobertson · 2023-05-17T14:44:52Z

This bug has been fixed two months ago. I've re-triggered the tests so hopefully they should all pass now.

Thanks for re-triggering the tests! Just re-ran these tests locally on chrome beta and m-c (with and without the bug 1821733 patchset). Tests pass reliably for me, but I'd be interested if others see the same results.

@dlrobertson is there anything else this PR would need?

I think the tests accurately describe browsers current behavior and are stable in my local testing. @masayuki-nakano is there anything else you think this PR needs?

dlrobertson · 2023-05-17T17:30:50Z

Added dom/events/scrolling/wheel-event-groups-multiple-action-chains.html. It is largely focused on testing that the webdriver functions like we'd hope. Tested on both chrome and mozilla-central (fails on current m-c and pasess with bug 1821733 patches). If this test should exist somewhere else, I'm happy to move it... it has enough in common with the other wheel-event-group-* tests that I placed it with the rest of them.

whimboo · 2023-05-23T10:24:56Z

@dlrobertson thanks for adding the test! It would be good if you could retrigger the CI checks (at least those for Firefox) once the fix from bug 1821733 made it into the next Nightly build.

dlrobertson · 2023-05-23T13:00:50Z

@dlrobertson thanks for adding the test! It would be good if you could retrigger the CI checks (at least those for Firefox) once the fix from bug 1821733 made it into the next Nightly build.

@whimboo good point. Re-triggered the CI checks and the check wpt-firefox-nightly-results seems to pass now.

whimboo · 2023-05-23T13:44:51Z

Great. So I think that @masayuki-nakano could have another look now.

dlrobertson · 2023-06-20T14:55:32Z

@smaug---- or @masayuki-nakano PTAL, I'd like to get more testing coverage for this, since this behavior is now the default in firefox (riding the trains with bug 1823700)). If this doesn't look like it is ready to be merged soon-ish, I can pull this into the mozilla specific wpts for now.

smaug---- · 2023-06-27T09:27:21Z

I think I'd still prefer to call this thing something else than "event group". It is more like a wheel transaction or some such.

And sorry being late here. Getting the review through bugzilla would be faster in my case, even for public wpts
(not only because the reviewing tools we use are quite a bit nicer than github).

But I'll take a look here.

dlrobertson · 2023-06-27T13:15:11Z

And sorry being late here. Getting the review through bugzilla would be faster in my case, even for public wpts
(not only because the reviewing tools we use are quite a bit nicer than github).

No problem, thanks for the feedback!

smaug----

The tests are a bit hard to review since there is really no comment what is the overall thing they are trying to test. Could you add some comments?

smaug---- · 2023-06-27T14:51:29Z

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

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


This talk about grouping is a bit unusual. In pointer events spec events may be captured to a one particular target. Could this all use similar terminology, something like wheel event capturing?

And then elsewhere there is also the concept of transaction used for this thing.

And there is also pointer lock. Is this whole thing some sort of implicit wheel event lock?

smaug---- · 2023-06-27T14:56:49Z

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

+
+let variants = [
+  {
+    key: 'target-basic',


What does key refer to here?

ahaa, it is for some helper function.

smaug---- · 2023-06-27T16:30:25Z

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

+
+<script>
+
+let variants = [


I wish there was some generic comment on top of the script explaining what is being tested. This is not very easy test to read when one needs to find all the relevant helper methods in other script files etc.

whimboo · 2023-08-21T10:06:40Z

@smaug---- as it looks like this PR is waiting for another round of review. Could you please have a look?

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.

Add tests for groups of wheel events.

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?

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

dlrobertson force-pushed the wheel-event-groups branch from 8e67e3a to b8e8af9 Compare May 17, 2023 17:25

dlrobertson force-pushed the wheel-event-groups branch from b8e8af9 to b5d49fa Compare May 23, 2023 12:33

dlrobertson requested a review from masayuki-nakano May 23, 2023 12:35

whimboo requested a review from smaug---- June 27, 2023 05:26

smaug---- requested changes Jun 27, 2023

View reviewed changes

dlrobertson force-pushed the wheel-event-groups branch from b5d49fa to 598c501 Compare July 20, 2023 21:47

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 2 times, most recently from 8388187 to 2691a25 Compare December 4, 2023 17:22

Add test for wheel event groups

b4d5721

Add tests for groups of wheel events.

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

		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

Are you sure you want to change the base?

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

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

dlrobertson commented May 17, 2023

dlrobertson commented May 17, 2023

whimboo commented May 23, 2023

dlrobertson commented May 23, 2023

whimboo commented May 23, 2023

dlrobertson commented Jun 20, 2023

smaug---- commented Jun 27, 2023 • edited

dlrobertson commented Jun 27, 2023

smaug---- 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

whimboo commented Aug 21, 2023

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

dlrobertson commented Jan 27, 2023 •

edited

smaug---- commented Jun 27, 2023 •

edited