1971979 - deltaX and deltaY for widget wheel scroll events have to take the scaling of the layout viewport into account

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Hiroyuki Ikezoe (:hiro) from bug 1970851 comment #2)

What the test does is;

    await new test_driver.Actions().scroll(50, 50, 0, 50).send();
    // Allow the possibility the scroll is not fully synchronous
    await t.step_wait(() => scroller.scrollTop === 50);

to trigger a wheel action, the deltaY is 50, I suppose the unit is CSS. Given that the test file doesn't have any meta viewport tag, then it's scaled by a value less than 1.0. On my environment it's scaled by 0.8163265585899353.

We convert the position of the wheel action into the browser coordinates, but we don't convert the deltas.

The below change fixed this failure at least on my environments.

  diff --git a/remote/shared/webdriver/Actions.sys.mjs b/remote/shared/webdriver/Actions.sys.mjs
  index bc16807fbe0a..223afecec164 100644
  --- a/remote/shared/webdriver/Actions.sys.mjs
  +++ b/remote/shared/webdriver/Actions.sys.mjs
  @@ -1827,21 +1827,25 @@ class WheelScrollAction extends WheelAction {
  -
       await assertInViewPort(scrollCoordinates, context);
  -
  -    lazy.logger.trace(
  -      `Dispatch ${this.constructor.name} with id: ${this.id} ` +
  -        `pageX: ${scrollCoordinates[0]} pageY: ${scrollCoordinates[1]} ` +
  -        `deltaX: ${this.deltaX} deltaY: ${this.deltaY} ` +
  -        `async: ${actions.useAsyncWheelEvents}`
  -    );
  -
       // Only convert coordinates if those are for a content process
       if (context.isContent && actions.useAsyncWheelEvents) {
         scrollCoordinates = await toBrowserWindowCoordinates(
           scrollCoordinates,
           context
         );
  +      [this.deltaX, this.deltaY] = await toBrowserWindowCoordinates(
  +        [this.deltaX, this.deltaY],
  +        context
  +      );
       }
  -
  +    lazy.logger.trace(
  +      `Dispatch ${this.constructor.name} with id: ${this.id} ` +
  +        `pageX: ${scrollCoordinates[0]} pageY: ${scrollCoordinates[1]} ` +
  +        `deltaX: ${this.deltaX} deltaY: ${this.deltaY} ` +
  +        `async: ${actions.useAsyncWheelEvents}`
  +    );
  +
       const startX = 0;
       const startY = 0;
       // This is an action-local state that holds the amount of scroll completed

As such we have to make sure that the delta values are correctly converted so that we scroll the correct amount of pixels in both of the directions.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1971979 - [remote] Calculate deltaX/deltaY for wheel scrolling based on layout viewport scaling.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1971979 - [wdspec] Add wheel scroll tests for scaled layout viewport.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So I tried the proposed solution from Hiro but it doesn't actually work for me with the attached patches and the wdspec test. I was sure it was working 2 weeks ago when it was proposed. So maybe something changed? I surely removed the meta viewport entry from the iframe-chains.html test, but using eg a scale of 0.5 now produces a delta of 100 when using 50.

Hiro, are you aware of any change in that area?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Commented on phabricator.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh no! The main reason why it was not working is that I'm running ./mach wpt --no-install, but I did a ./mach install before. Sadly I missed that the latter command actually installs org.mozilla.geckoview-example but NOT the testrunner that wpt is using. As such running the tests never actually used my modified code from within the Remote Agent and surely failed. :(

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

To repeat here what I mentioned on Phabricator: Applying D254705 seem to cause a 10s delay after Firefox exited within the wptrunner. I'm not sure what's causing it. So moving that revision to changes planned for now.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

What's also suspicious is that for the following test where we are trying to scroll a non-scrollable element, the deltaY value of the wheel event is way off to what we actually requested:

https://searchfox.org/mozilla-central/rev/ba8d4b59f46a820fc3c2f0a55c638e4f8b29bfda/testing/web-platform/tests/webdriver/tests/classic/perform_actions/wheel.py#37-48

Here the output that I see locally with the patches attached:

/webdriver/tests/classic/perform_actions/wheel.py
  FAIL test_scroll_not_scrollable - assert 95 == 10
session = <Session 570c8370-93e4-4719-b01f-850b3d68bed1>, test_actions_scroll_page = None
wheel_chain = <webdriver.client.ActionSequence object at 0x1056c4510>

    def test_scroll_not_scrollable(session, test_actions_scroll_page, wheel_chain):
        target = session.find.css("#not-scrollable", all=False)

        wheel_chain.scroll(0, 0, 5, 10, origin=target).perform()

        events = get_events(session)
        assert len(events) == 1
        assert events[0]["type"] == "wheel"
        assert events[0]["deltaX"] == 5
>       assert events[0]["deltaY"] == 10
E       assert 95 == 10

events     = [{'altKey': False,
  'button': 0,
  'buttons': 0,
  'ctrlKey': False,
  'deltaMode': 0,
  'deltaX': 5,
  'deltaY': 95,
  'deltaZ': 0,
  'metaKey': False,
  'pageX': 58,
  'pageY': 93,
  'shiftKey': False,
  'target': 'not-scrollable-content',
  'type': 'wheel'}]

This is because we end up with dispatching the following wheel scroll event:

0:05.24 pid:56040 1750792736296 RemoteAgent TRACE Dispatch WheelScrollAction with id: wheel_id pageX: 58 pageY: 178 deltaX: 5 deltaY: 95 async: true

I assume that this is actually the toolbar height that gets factored in here so that LayoutUtils.rectToTopLevelWidgetRect() is not something that we should use here. Maybe we need some new method in WindowUtils (and LayoutUtils) that could compute us relative coordinates? Or would it actually be enough to only check the top-level browsing context for the layout scaling?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Yeah, I think you are right. We probably need a new function dedicated the delta conversions.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

Yeah, I think you are right. We probably need a new function dedicated the delta conversions.

I filed bug 1973884 for that request. Thanks for checking.

Given that it's not blocking us from enabling async wheel scroll events for wpt and that this implementation is blocked on a platform bug lets move it into the P3 bucket of our next milestone.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I'd inform Henrik before his off that I realized we can use nsIDOMWindowUtils.toTopLevelWidgetRect to convert the deltas if we specify the deltas as aWidth and aHeight.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I run the test as mentioned above but I can no longer reproduce this problem. Using async wheel events the delta values are now calculated correctly:

 0:07.41 pid:37726 1769184554377	Marionette	DEBUG	0 -> [0,9,"WebDriver:PerformActions",{"actions":[{"actions":[{"deltaX":5,"deltaY":10,"origin":{"element-6066-11e4-a52e-4f735466cecf":"fea6def2-a4ac-42fc-92b9-3317d210a827"},"type":"scroll","x":0,"y":0}],"id":"wheel_id","type":"wheel"}]}]
 0:07.41 pid:37726 1769184554380	RemoteAgent	TRACE	Dispatching tick 1/1
 0:07.42 pid:37726 1769184554383	RemoteAgent	TRACE	Dispatch WheelScrollAction with id: wheel_id pageX: 58 pageY: 93 deltaX: 5 deltaY: 10 async: true
 0:07.42 pid:37726 1769184554384	RemoteAgent	TRACE	moveOverTime start: 0,0 target: 5,10 duration: 0
 0:07.42 pid:37726 1769184554384	RemoteAgent	TRACE	WheelScrollAction.performOneWheelScrollStep [5,10]
 0:07.42 pid:37726 1769184554385	RemoteAgent	TRACE	WebDriverProcessData actor created for PID 37731
 0:07.42 pid:37726 1769184554386	Marionette	TRACE	[2] MarionetteCommands actor created for window id 4
 0:07.43 pid:37726 1769184554395	Marionette	DEBUG	0 <- [1,9,null,{"value":null}]
 0:07.43 pid:37726 1769184554396	webdriver::server	DEBUG	<- 200 OK {"value":null}
 0:07.43 pid:37726 1769184554396	webdriver::server	DEBUG	-> POST /session/d0eb1e67-981e-4314-a897-3b34f5f65ee7/execute/sync {"script": "return allEvents.events;", "args": []}
 0:07.43 pid:37726 1769184554396	Marionette	DEBUG	0 -> [0,10,"WebDriver:ExecuteScript",{"args":[],"script":"return allEvents.events;"}]
 0:07.43 pid:37726 1769184554399	Marionette	DEBUG	0 <- [1,10,null,{"value":[{"type":"wheel","button":0,"buttons":0,"pageX":58,"pageY":93,"deltaX":5,"deltaY":10,"deltaZ":0,"deltaMode":0,"target":"not-scrollable-content","altKey":false,"ctrlKey":false,"metaKey":false,"shiftKey":false}]}]

Not sure what exactly fixed that so marking it as WFM.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I was actually wrong. This is a bug that is still needed. It's about the scaling of the layout viewport. I'll update the summary to make it clearer.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7895ec344330
https://hg.mozilla.org/integration/autoland/rev/68bf6aeda0d7
[remote] Calculate deltaX/deltaY for wheel scrolling based on layout viewport scaling. r=jdescottes,hiro
https://github.com/mozilla-firefox/firefox/commit/f37269fbbd51
https://hg.mozilla.org/integration/autoland/rev/e1e2525706a4
[wdspec] Add wheel scroll tests for scaled layout viewport. r=hiro,jdescottes

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/60581 for changes under testing/web-platform/tests

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/72c165452fd9
https://hg.mozilla.org/integration/autoland/rev/e1a40ef100c8
Revert "Bug 1971979 - [wdspec] Add wheel scroll tests for scaled layout viewport. r=hiro,jdescottes" for causing wpt failures @ smooth-scrollIntoView-with-unrelated-gesture-scroll.html

Comment 17

[amarc]

Backed out for causing wpt failures

• Backout link
• Push with failures
• Failure Log

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is actually caused by /css/cssom-view/smooth-scrollIntoView-with-unrelated-gesture-scroll.html | scrollIntoView is not interrupted by unrelated gesture scroll now permanently passing on Android. It requires a wpt metadata update.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/72690ab2074e
https://hg.mozilla.org/integration/autoland/rev/fa216bf115bb
[remote] Calculate deltaX/deltaY for wheel scrolling based on layout viewport scaling. r=jdescottes,hiro
https://github.com/mozilla-firefox/firefox/commit/193de5f97d73
https://hg.mozilla.org/integration/autoland/rev/ee2282b2c43a
[wdspec] Add wheel scroll tests for scaled layout viewport. r=hiro,jdescottes

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b76267e042d3
https://hg.mozilla.org/integration/autoland/rev/fc7e23db1d16
Revert "Bug 1971979 - [wdspec] Add wheel scroll tests for scaled layout viewport. r=hiro,jdescottes" for causing wpt failures in wheel-multiple-sources-deadlock.html
https://github.com/mozilla-firefox/firefox/commit/0ad95494adf7
https://hg.mozilla.org/integration/autoland/rev/20ff4968226c
Revert "Bug 1971979 - [remote] Calculate deltaX/deltaY for wheel scrolling based on layout viewport scaling. r=jdescottes,hiro" for causing wpt failures in wheel-multiple-sources-deadlock.html

Comment 22

[Cristian Tuns]

Backed out for causing wpt failures

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | /uievents/order-of-events/mouse-events/wheel-multiple-sources-deadlock.html | Simultaneous wheel events in different directions should not deadlock. - assert_approx_equals: Vertical delta from wheel1 must be processed. expected 50 +/- 0.1 but got 40.81632995605469

Also possibly this: https://treeherder.mozilla.org/logviewer?job_id=572884965&repo=autoland&task=D9i93_oCRD6CrznKw8vpjg.0

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hiro, I will have a look at those two failing tests tomorrow, but maybe you already have an idea what might be wrong.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

CCing Alex since the other failure in comment 22 is unrelated-gesture-scroll-during-snap.html.

Comment 25

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg-edge.mozilla.org/mozilla-central/rev/20ff4968226c

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Now I tried toreproduce wheel-multiple-sources-deadlock.html failure on my local Android emulator with a debug build, I can't produce. Now I realized all failures on opt builds. So I suspect there's a race condition when WebDriver dispatches the two scroll actions the document has't yet been scaled down, but when the actions reached to APZ, the document has been already scaled. That's my hypothesis. Adding a meta viewport tag to the test would be a workaround.

For the other test failure, unrelated-gesture-scroll-during-snap.html, it's unexpected pass so we don't need to worry here, we can just change the annotation. unrelated-gesture-scroll-during-snap.html.ini this test annotation looks odd to me, that's another story.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

I can't reproduce even on a local opt build with --disable-fission --repeat=50.

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)

Now I tried toreproduce wheel-multiple-sources-deadlock.html failure on my local Android emulator with a debug build, I can't produce. Now I realized all failures on opt builds. So I suspect there's a race condition when WebDriver dispatches the two scroll actions the document has't yet been scaled down, but when the actions reached to APZ, the document has been already scaled. That's my hypothesis. Adding a meta viewport tag to the test would be a workaround.

I don't have to necessarily add a meta viewport tag here. It works as well when awaiting the load event before starting the test. Then the test permanently passes for me as well. Note that by default the wptrunner only waits for the DOMContentLoaded event (and awaiting this keeps the test failing). I wonder if this is a bug in Firefox or expected. On the other side I wonder as well how many other tests might fail due to a not-yet applied viewport scaling.

For the other test failure, unrelated-gesture-scroll-during-snap.html, it's unexpected pass so we don't need to worry here, we can just change the annotation. unrelated-gesture-scroll-during-snap.html.ini this test annotation looks odd to me, that's another story.

Ah, I didn't look closely at that one yet. Yes, that's easy to fix.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1971979 - [wpt] Fix meta data for unrelated-gesture-scroll-during-snap.html.ini.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/21c1c7790a39
https://hg.mozilla.org/integration/autoland/rev/efb6b997e080
[wpt] Fix meta data for unrelated-gesture-scroll-during-snap.html.ini. r=hiro DONTBUILD

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7ecc0240d24e
https://hg.mozilla.org/mozilla-central/rev/0a0c39d17232
[remote] Calculate deltaX/deltaY for wheel scrolling based on layout viewport scaling. r=jdescottes,hiro
https://github.com/mozilla-firefox/firefox/commit/ddc5f8ca05da
https://hg.mozilla.org/mozilla-central/rev/42525a223ba2
[wdspec] Add wheel scroll tests for scaled layout viewport. r=hiro,jdescottes

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0a0c39d17232
https://hg.mozilla.org/mozilla-central/rev/42525a223ba2
https://hg.mozilla.org/mozilla-central/rev/efb6b997e080

Comment 34

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot