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

Status: RESOLVED WORKSFORME

(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.