Closed Bug 1848957 Opened 3 years ago Closed 1 year ago

Send async "wheel" events at the widget level instead of synthesized DOM events

Categories

(Remote Protocol :: Agent, task, P2)

Product:
Remote Protocol
Component:
Agent
Type:
task
Points:
5

Tracking

(firefox140 fixed)

Status:
RESOLVED FIXED
Milestone:
140 Branch
Tracking Flags:
Tracking Status
firefox140 --- fixed

People

(Reporter: whimboo, Assigned: whimboo, NeedInfo)

References

(Blocks 9 open bugs)

Details

(Whiteboard: [webdriver:m16], [wptsync upstream])

Attachments

(3 external files, 4 obsolete files)

See more details on bug 1773393. We need widget level events to be able to handle scrollend and overscroll events.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1849227
Depends on: 1849229

As it turned out we always crash when we are sending any kind of widget level event via EventUtils and DOMWindowUtils. The reason is that we only allow those events in Automation:

https://searchfox.org/mozilla-central/rev/d7a8eadc28298c31381119cbf25c8ba14b8712b3/dom/ipc/BrowserParent.cpp#1658,1675,1692,1709

Sadly both Marionette and Remote Agent are not considered when checking Cu.isInAutomation and as such we force a crash.

That means that I have to fix bug 1834306 first before I'm able to continue on this bug. For now I've uploaded WIP patches, which reflect the current state of work.

Depends on: 1834306

By using synthesizeWheelAtPoint() from EventUtils.js I can see an interesting behavior. After each call of this method a scrollend event is fired, which basically will not work when in WebDriver we split up a wheel action into ticks (smaller movements per 17ms) to slow down the action. Workaround for now is to use a duration of 0 for the wheel event so perform it in one go.

Botond, is that expected by that API? If yes, what would I have to use to indicate that the wheel event isn't done yet?

Flags: needinfo?(botond)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #5)

By using synthesizeWheelAtPoint() from EventUtils.js I can see an interesting behavior. After each call of this method a scrollend event is fired, which basically will not work when in WebDriver we split up a wheel action into ticks (smaller movements per 17ms) to slow down the action. Workaround for now is to use a duration of 0 for the wheel event so perform it in one go.

Botond, is that expected by that API?

I believe so. The API is synthesizing a wheel event with instant scrolling mode, so the behaviour will match what happens when a user with "Use smooth scrolling" disabled in preferences uses a mouse with a scroll wheel that scrolls in discrete increments, and scrolls the wheel one increment. In such a case, the production behaviour is also that we get a scrollend after each wheel event.

If yes, what would I have to use to indicate that the wheel event isn't done yet?

One thing that occurs to me is that we could give the API an option to synthesize a wheel event with smooth scrolling mode; the behaviour will then match what happens when a user scrolls one increment with "Use smooth scrolling" enabled in preferences, i.e. a scroll animation that takes several frames to complete will be launched, and a single scrollend will be fired at the end of the animation.

Note that in this case, the scroll animation will be under the control of the platform, i.e. you'd only call the API once, with the full distance you'd like to scroll, and the platform will divide that up into segments to cover each frame.

Would this meet Marionette's needs?

Flags: needinfo?(botond)

Thanks for the explanation Botond! That totally makes sense. Regarding your question I don't know if we can fully hand over the segmentation to the platform given that it might / would break our WebDriver Actions behavior especially when multiple input sources are assigned. The documentation around Tick explains that.

Also I'm not sure how other browsers behave here. Especially it would be interesting to know which events Chrome and Safari actually fire when a wheel action takes multiple ticks. I can check that tomorrow and report back.

Btw Dan also pointed me to https://github.com/w3c/csswg-drafts/issues/8396 which is a bit annoying for our testing given that both scroll and scrollend events are fired at the exact same time which adds a race when trying to check the order of events. Only when scrolling within an iframe the order is correct (scrollend after scroll).

Depends on: 1849972

I've checked WebDriver tests for Chrome and it seems they as well synthesize events these days given that only the wheel event is emitted when performing a wheel action, and both scroll and scrollend are missing.

I believe it should be fine to hand smooth scrolling over to the platform; the exact details of the events inside a tick are implementation defined. What is supposed to be true is that the overall scroll duration matches the tick duration, so we'd need some way to specify that (I guess nearest frame would be enough in practice).

Depends on: 1850166
No longer depends on: 1834306

To workaround the Cu.isInAutomation problem we could add a new function like AllowContentToDispatchWidgetEvents to the IPC related code, which checks the running state of Marionette and Remote Agent, or even better to not send these events from an untrusted content process and instead change Marionette to emit events directly from the parent process. I would definitely consider the latter option first, and will get it investigated tomorrow.

I had a look into performing actions within the parent process. Overall I had some difficulty to convert the relevant positions for mouse and scroll events to screen coordinates. But with LayoutUtils.sys.mjs this was actually not that hard. At least it works now for DPR settings of 1, but still fails on HiRes displays.

While I was happy that everything works I missed to also check for the key actions, and adding tests for those last I was faced with an error from EventUtils.sys.mjs:

[Exception... "Failure arg 0 [nsITextInputProcessor.beginInputTransactionForTests]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://remote/content/external/EventUtils.js :: _getTIP :: line 1955" data: no]
stacktrace:

I assume that beginInputTransactionForTests() is not meant to work from Chrome scope? In that case we probably would have to send key events still via the content process and need a special check to circumvent the Cu.isInAutomation check. If we cannot change that, or not short term I will have to keep using content process dispatching for now by using extra checks in the IPC code.

Masayuki, could you please share some further information given that you might know most from that area. Thanks.

Flags: needinfo?(masayuki)

I'm not familiar with privilege management around there. Cannot elevate the privilege when you call functions of EventUtils.js?

Flags: needinfo?(masayuki) → needinfo?(hskupin)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

I'm not familiar with privilege management around there. Cannot elevate the privilege when you call functions of EventUtils.js?

I was сompletely wrong with the above comment. Sorry for that. The MOZ_RELEASE_ASSERT actually checks that Chrome privileges exist and that is what we have. Also it would not raise an NS_ERROR_FAILURE with Failure arg 0. That means the problem is in BeginInputTransactionInternal and here the related arg checks.

I had a look with the debugger again what gets passed into the call to beginInputTransactionForTests and I noticed that it is not a window but a custom object. This actually is the reason of a refactoring in our Marionette code. Updating the code to correctly pass in the ChromeWindow makes it all work! Sorry again for the false alarm.

All that are great news and I'm going now to file a new bug as dependency so that we can send all kind of events via the parent process guarded behind a preference. Initially it will be turned off by default given the complexity of the changes and possible regressions in tests. But widget events would override it then - which are off by default as well for the time being.

Flags: needinfo?(hskupin)

Actually what I completely missed is the fact that sending events from the parent process only works when the browser to interact with is actually the foreground tab. When Marionette / Remote Agents has to interact with a background tab events no longer work given that they are still send to the foreground tab.

Masayuki or Hiro, do you know if there is or could be a way to work with EventUtils in the parent process and forward the event not only to the currently visible tab but a specific (top-level) browsing context? If that's not possible we won't be able to use parent process emitting of events.

Flags: needinfo?(masayuki)
Flags: needinfo?(hikezoe.birchill)

As far as I know of, as of now, there's no way. But of course we could implement it in BrowsingContext. Maybe we could also extend existing nsIDOMWindowUtils functions (used in EventUtils.js) to make it work for the given BrowsingContext.

That being said, I am afraid I haven't quite followed the way to send events from the parent process. Given that a JS script tries to send a native event in a cross process iframe in a web platform test, how does it work with the parent process? You meant sending events via the parent process? If it means via, that's pretty normal, that's the way how events are handled in APZ in our mochitest framework usually.

Flags: needinfo?(hikezoe.birchill)

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

As far as I know of, as of now, there's no way. But of course we could implement it in BrowsingContext. Maybe we could also extend existing nsIDOMWindowUtils functions (used in EventUtils.js) to make it work for the given BrowsingContext.

What would be a potential timeline for that? If it has low priority and we might have to wait for such an implementation a couple of weeks, I cannot rely on it and for now would try to get widget events working from the content process.

That being said, I am afraid I haven't quite followed the way to send events from the parent process. Given that a JS script tries to send a native event in a cross process iframe in a web platform test, how does it work with the parent process? You meant sending events via the parent process? If it means via, that's pretty normal, that's the way how events are handled in APZ in our mochitest framework usually.

I didn't test (cross-process) iframes yet. So in Marionette right now we are sending the event directly via EventUtils from the JSWindowActor that is alive in the related content process and browsing context. So there is no issue. Parent here would mean that EventUtils is called from the parent process (a parent actor) and the ChromeWindow is passed with the appropriate transformed coordinates for mouse / wheel events.

Flags: needinfo?(hikezoe.birchill)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #16)

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

As far as I know of, as of now, there's no way. But of course we could implement it in BrowsingContext. Maybe we could also extend existing nsIDOMWindowUtils functions (used in EventUtils.js) to make it work for the given BrowsingContext.

What would be a potential timeline for that? If it has low priority and we might have to wait for such an implementation a couple of weeks, I cannot rely on it and for now would try to get widget events working from the content process.

The work would be straight forward but would require a bunch of code changes, could be done within two weeks I guess, but we haven't prioritized such work in this H2 at least. :/

That being said, I am afraid I haven't quite followed the way to send events from the parent process. Given that a JS script tries to send a native event in a cross process iframe in a web platform test, how does it work with the parent process? You meant sending events via the parent process? If it means via, that's pretty normal, that's the way how events are handled in APZ in our mochitest framework usually.

I didn't test (cross-process) iframes yet. So in Marionette right now we are sending the event directly via EventUtils from the JSWindowActor that is alive in the related content process and browsing context. So there is no issue. Parent here would mean that EventUtils is called from the parent process (a parent actor) and the ChromeWindow is passed with the appropriate transformed coordinates for mouse / wheel events.

Okay, now I understand it. The parent actor in the parent process is the actor corresponding to the content process regardless whether it's in out of process or not, so it should work, but need to be careful about coordinates conversions.

One question, with the way to send events from the parent process, do we still run this if (StaticPrefs::test_events_async_enabled()) { branch as if we set test.events.async.enabled to true? If not (IIUC why you started it, the answer is no?), things will be more involved than I thought. And honestly I would not recommend it.

Flags: needinfo?(hikezoe.birchill)

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

The work would be straight forward but would require a bunch of code changes, could be done within two weeks I guess, but we haven't prioritized such work in this H2 at least. :/

So given that it would actually be possible let me file a bug for that particular feature addition. Then we can see if it's doable or not within the time frame.

Okay, now I understand it. The parent actor in the parent process is the actor corresponding to the content process regardless whether it's in out of process or not, so it should work, but need to be careful about coordinates conversions.

Yes, I got that conversation working locally for DPR settings of 1 but still have issues with HiRes displayes, but that should be (hopefully not hard) solvable as well. I will definitely check how it works with elements in frames.

One question, with the way to send events from the parent process, do we still run this if (StaticPrefs::test_events_async_enabled()) { branch as if we set test.events.async.enabled to true? If not (IIUC why you started it, the answer is no?), things will be more involved than I thought. And honestly I would not recommend it.

Yes we should once the patches on this bug will land. Moving everything to the parent process is just the first step to workaround the problem with Cu.isInAutomation (see bug 1834306). If parent process event submission is not something we can do in the short term I have to add a workaround for the BrowserParent IPC check to also include Marionette and Remote Agent. Right now it just crashes Firefox because widget events from an untrusted content process aren't allowed. Does that answer your question?

Flags: needinfo?(masayuki) → needinfo?(hikezoe.birchill)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #18)

One question, with the way to send events from the parent process, do we still run this if (StaticPrefs::test_events_async_enabled()) { branch as if we set test.events.async.enabled to true? If not (IIUC why you started it, the answer is no?), things will be more involved than I thought. And honestly I would not recommend it.

Yes we should once the patches on this bug will land. Moving everything to the parent process is just the first step to workaround the problem with Cu.isInAutomation (see bug 1834306). If parent process event submission is not something we can do in the short term I have to add a workaround for the BrowserParent IPC check to also include Marionette and Remote Agent. Right now it just crashes Firefox because widget events from an untrusted content process aren't allowed. Does that answer your question?

I am confused. With events from the parent process , if we call widget->DispatchInputEvent, it will hit this IsInAutomation, isn't it? Is that really what we wan to do?

Even if we take the else branch, we call widget->DispatchEvent, and the DispatchEvent is a function used in production, thus we can't have IsInAutomation assertion, but I guess we should have the assertion?

Flags: needinfo?(hikezoe.birchill)

Text input needs to work with globally focused element. It's targeted to focused remote, and IME state needs to live with focus because IME related code in the parent process (and also native IME APIs) does not assume that multiple editable element can be active at same time. So, synthesizing keyboard (text) inputs in inactive tab does not make sense if the author needs to test the browser behavior.

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

I am confused. With events from the parent process , if we call widget->DispatchInputEvent, it will hit this IsInAutomation, isn't it? Is that really what we wan to do?

Based on Nika's reply on the other bug we should only reach those lines in BrowserParent.cpp when the request comes from an untrusted content process. Emitting the event from the parent process instead should not end-up in this method.

To verify that I did not set the disable all security preference, which leads to the expected crash when the event is sent from the JSWindowActor child. Keeping that but sending the event with Marionette from the parent process does indeed not crash Firefox and I can see a wheel, scroll and scrollend event reported on the test page.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #20)

Text input needs to work with globally focused element. It's targeted to focused remote, and IME state needs to live with focus because IME related code in the parent process (and also native IME APIs) does not assume that multiple editable element can be active at same time. So, synthesizing keyboard (text) inputs in inactive tab does not make sense if the author needs to test the browser behavior.

Does it also apply when the focusmanager.testmode is set to true? That's what we do in Marionette to allow tests to even run when the window is in the background.

Flags: needinfo?(masayuki)
Flags: needinfo?(hikezoe.birchill)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #21)

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

I am confused. With events from the parent process , if we call widget->DispatchInputEvent, it will hit this IsInAutomation, isn't it? Is that really what we wan to do?

Based on Nika's reply on the other bug we should only reach those lines in BrowserParent.cpp when the request comes from an untrusted content process. Emitting the event from the parent process instead should not end-up in this method.

To verify that I did not set the disable all security preference, which leads to the expected crash when the event is sent from the JSWindowActor child. Keeping that but sending the event with Marionette from the parent process does indeed not crash Firefox and I can see a wheel, scroll and scrollend event reported on the test page.

Great to hear that. Then things should be easier than I thought.

What we need to do for making the EventUtils function (I am supposing it's synthesizeWheel) work from the parent process with the given BrowsingContext is;

  1. Add a new argument to nsIDOMWindowUtils.sendWheelEvent to represent the BrowsingContext (I am not sure what the appropriate value to represent it though)
  2. In C++ implementation of sendWheelEvent, get the corresponding BrowserParent from the BrowsingContext represent value
  3. Call BrowserParent::RecvDispatchWheelEvent directly

I think it should just work.

Flags: needinfo?(hikezoe.birchill)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #21)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #20)

Text input needs to work with globally focused element. It's targeted to focused remote, and IME state needs to live with focus because IME related code in the parent process (and also native IME APIs) does not assume that multiple editable element can be active at same time. So, synthesizing keyboard (text) inputs in inactive tab does not make sense if the author needs to test the browser behavior.

Does it also apply when the focusmanager.testmode is set to true? That's what we do in Marionette to allow tests to even run when the window is in the background.

It depends on how each OS implements IME API. When you change some state of focused editor, e.g., focusing, blurring, changing caret position or changing content, remote process sends the data to the parent process, then, parent process will interact with OS because modern IMEs need the content data. At this moment, if the tab is active at least in our with setting focusmanager.testmode, our widget (probably) can interact with OS. However, OS may do nothing (i.e., not request content query, etc). Such difference might cause actual behavior difference.

If no editable element has focus, this is not applied.

Flags: needinfo?(masayuki)
Blocks: 1797215
Blocks: 1848958

Thanks for all the feedback. As it looks like I will have to create / run some more tests to ensure that switching the sending of events from the content process to the parent process will not cause a functional regression for Firefox and WebDriver. I hope that I get to that today.

Here some findings:

  • Emitting key actions from the parent would clearly require a target window other than the ChromeWindow because right now they would trigger chrome scope shortcuts like opening the bookmarks sidebar when Meta+B is pressed. I assume that with the solution from Hiro this would not happen?

  • As per spec the input state for each input source has to be kept separate for each browsing context. Moving to the parent process would mean we need to move these input states to the parent process as well, and keep it separate for each open browsing context. Same applies to the dblclick state flag, which is per browsing context as well.

  • We do not have any Perform Actions tests which are actually run within an iframe. As such verifying that no regressions are present for those scenarios is not possible. We definitely have to create those tests.

  • For WebDriver classic we do not have any Perform Actions tests which run in a background window or background tab - this is mostly a Mozilla only feature because it's not in the specification. For WebDriver BiDi it potentially should work given that a tab where an action is performed can be in the background.

Things that I haven't investigated or fixed yet:

  • There are problems with checking if the click point is in the visible viewport or not. I didn't investigate that yet.
  • Other device pixel resolutions than 1 still don't work yet - some calculation is off here and would need to be fixed.

Overall this is a lot of work to do, and I'm currently considering to change my mind and temporarily add the checks for Marionette and Remote Agent to the IPC code. But this will only work if we do not see any performance regressions. I should investigate this first before we make the final decision how to get this implemented.

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #14)

Actually what I completely missed is the fact that sending events from the parent process only works when the browser to interact with is actually the foreground tab. When Marionette / Remote Agents has to interact with a background tab events no longer work given that they are still send to the foreground tab.

Masayuki or Hiro, do you know if there is or could be a way to work with EventUtils in the parent process and forward the event not only to the currently visible tab but a specific (top-level) browsing context? If that's not possible we won't be able to use parent process emitting of events.

I think trying to send events through APZ to a page in a background tab might cause other problems.

When APZ processes an event, it performs a hit-test using rendering-related state, such as the WebRender scene and other things derived from the display list. I think those pieces of rendering state are not created for background tabs.

Depends on: 1851511
Attachment #9349592 - Attachment description: WIP: \Bug 1848957 - [dom] Add support for widget-level wheel (scroll) events to WindowUtils. → WIP: Bug 1848957 - [dom] Add support for widget-level wheel (scroll) events to WindowUtils.
Attachment #9349591 - Attachment description: WIP: Bug 1848957 - [wpt] Update /dom/events/scrolling/ metadata to get tests passing locally on MacOS. → Bug 1848957 - [wpt] Update /dom/events/scrolling/ metadata to get tests passing locally on MacOS.
Attachment #9349592 - Attachment description: WIP: Bug 1848957 - [dom] Add support for widget-level wheel (scroll) events to WindowUtils. → Bug 1848957 - [dom] Add support for widget-level wheel (scroll) events to WindowUtils.
Attachment #9349593 - Attachment description: WIP: Bug 1848957 - [remote] Add support for wheel (scroll) widget level events. → Bug 1848957 - [remote] Add support for wheel (scroll) widget level events.

To give a quick update... After the recent investigations I had a look in how we could send the event from the content process by making an exception for the Remote Agent so that it not crashes the browser. I filed bug 1851511 and provided a patch, but recent discussions show that we probably do not want to do that because it would harm security. A final decision hasn't been made yet but as it looks like we should actually follow the proposal for dispatching the events in the parent process. Given that we want to do that anyway in the future I've already filed a meta bug 1851812 but won't add it as dependency yet.

I have some comments and questions:

  1. When we would be able to dispatch an event from the parent process to a specific browsing context, I assume we won't have to do all the coordinate calculations because in such a case it would be all based on the browsing context's window? Means no screen coordinates would be necessary?

  2. Regarding the problem with keyboard events, we may want to leave those for now by being dispatched as synthesized events in the content process. It looks like those aren't needed for the interop2023 goal. But please correct me if I'm wrong.

  3. The above should allow us to run the relevant scroll and touch wpt tests not only in our own CI with the async events preference for tests enabled, but also within the upstream web-platform-tests checks. This is most likely also what we want to show our progress, and there the before mentioned preference doesn't work.

  4. For the Gecko platform work I probably need help, so it might be good to have a meeting scheduled to discuss the individual work items. But lets wait for the content process decision first.

Hiro and Botond do you have any further feedback?

Flags: needinfo?(hikezoe.birchill)
Flags: needinfo?(botond)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #27)

I have some comments and questions:

  1. When we would be able to dispatch an event from the parent process to a specific browsing context, I assume we won't have to do all the coordinate calculations because in such a case it would be all based on the browsing context's window? Means no screen coordinates would be necessary?

It depends on how we send the event and what the original event position coordinate system is. For example, if it ends up calling BrowserParent::RecvDispatchWheelEvent, the event coord system needs to be in the corresponding BrowserChild's coords, say, given that it's in an oop iframe, the coord system needs to be the iframe one.

  1. Regarding the problem with keyboard events, we may want to leave those for now by being dispatched as synthesized events in the content process. It looks like those aren't needed for the interop2023 goal. But please correct me if I'm wrong.

Right, sending native keyboard events is not in the interop2023 scope.

Flags: needinfo?(hikezoe.birchill)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #27)

Hiro and Botond do you have any further feedback?

My only outstanding concern is the one mentioned in comment 26, about widget events only being able to target the foreground tab due to the hit-test using rendering data structures that are only retained for the foreground tab.

I'm unclear what the expectations are about what needs to work in background tabs (for example, are WPTs ever run in background tabs?), so I'm not sure if this is actually a problem, just want to make sure you're aware of it.

Flags: needinfo?(botond) → needinfo?(hskupin)

(In reply to Botond Ballo [:botond] from comment #29)

My only outstanding concern is the one mentioned in comment 26, about widget events only being able to target the foreground tab due to the hit-test using rendering data structures that are only retained for the foreground tab.

If required we would keep the keyboard event dispatching in the content process for now. Once I wrote more tests and compared between different browsers I'll have a better idea what the overall status is.

I'm unclear what the expectations are about what needs to work in background tabs (for example, are WPTs ever run in background tabs?), so I'm not sure if this is actually a problem, just want to make sure you're aware of it.

This doesn't apply to wpt tests but WebDriver testing and automation in general. This is all outside of our CI and can be done by anyone.

I've pushed a try build for having the test.events.async.enabled preference set in the default profile for wpt tests. Lets see if that causes some fallout in other tests. If not we can get this landed and wait for the upstream sync so that on GitHub the test is going to pass as well.

https://treeherder.mozilla.org/jobs?repo=try&revision=670bb19b90d3720f8c07d10d951bbc6394799a2f

Flags: needinfo?(hskupin)

Given that we have to dispatch events from the parent process (work for that will happen on the meta bug 1851812) we no longer need to block on bug 1849972 (MOZ_DISABLE_NONLOCAL_CONNECTIONS). Individual dependencies will be filed soon.

No longer depends on: 1849972, 1851511
Blocks: 1851812
Depends on: 1853120
No longer blocks: 1851812
Depends on: 1851812
Summary: Consider sending widget input events for scroll (wheel) instead of synthesized DOM events → Send async "wheel" events at the widget level instead of synthesized DOM events

Please note that with bug 1852243 we have a temporary workaround available that will work fine for layout folks to work towards the interop 2023 goals. As such we do not have to work on this feature in M8 anymore.

Status: ASSIGNED → NEW
Priority: P1 → P2
Whiteboard: [webdriver:m8] → [webdriver:backlog]
Assignee: hskupin → nobody
No longer depends on: 1851812
Blocks: 1866500
No longer blocks: 1866500

Based on bug 1689546 comment 20, this may help us pass one of our two remaining failing WPTs for interop2023's scrolling focus area (css/css-overscroll-behavior/overscroll-behavior.html), and since that bug is closed I felt it might be good to not lose sight of that by mentioning it here.

Blocks: interop-2023-scrolling

Note that for calculating the position relative to the top-level widget (browser), you can use the new LayoutUtils.rectToToplevelWidgetRect() helper method. This method allows you to get the target position based on local coordinates, even from within frames. Ensure that the device pixel ratio settings are applied before calling this method.

Depends on: 1904859
Whiteboard: [webdriver:backlog] → [webdriver:m12]
Depends on: 1852529

https://github.com/web-platform-tests/wpt/pull/37445 added some more wheel scroll event tests that could be useful when implementing this bug. These tests can be found under /dom/events/scrolling/wheel-event-transactions*.

See Also: → https://github.com/web-platform-tests/wpt/pull/37445
Whiteboard: [webdriver:m12] → [webdriver:m13]
Whiteboard: [webdriver:m13] → [webdriver:m14]
Blocks: 1904859
No longer depends on: 1904859
Whiteboard: [webdriver:m14] → [webdriver:m15]

A good number of wpt tests for scrolling - that we have to take into account for testing this new feature - can be found at:
https://github.com/web-platform-tests/wpt/tree/master/dom/events/scrolling

See Also: https://github.com/web-platform-tests/wpt/pull/37445 → https://github.com/web-platform-tests/wpt/tree/master/dom/events/scrolling
Blocks: 1945692
Whiteboard: [webdriver:m15] → [webdriver:m16]

I tried to have updated webdriver tests first but without having it properly implemented in Firefox and as well Chrome failing (due to crashes) on most of them I cannot easily verify. As such lets flip the order and get the Marionette and platform bits implemented first.

Assignee: nobody → hskupin
Blocks: 1852529
Status: NEW → ASSIGNED
No longer depends on: 1852529
Component: Marionette → Agent
Depends on: 1957955

For the DOM/platform related work I've filed bug 1957955. Means I'm going to split-up my patches as attached here.

Attachment #9349592 - Attachment description: Bug 1848957 - [dom] Add support for widget-level wheel (scroll) events to WindowUtils. → WIP: Bug 1848957 - [dom] Add support for widget-level wheel (scroll) events to WindowUtils.

Comment on attachment 9349592 [details]
WIP: Bug 1848957 - [dom] Add support for widget-level wheel (scroll) events to WindowUtils.

Revision D186548 was moved to bug 1957955. Setting attachment 9349592 [details] to obsolete.

Attachment #9349592 - Attachment is obsolete: true
Attachment #9349593 - Attachment description: Bug 1848957 - [remote] Add support for wheel (scroll) widget level events. → WIP: Bug 1848957 - [remote] Add support for wheel scroll widget events.

When trying to run a Marionette test with the patch on D186549 attached I can see a crash in the IPC code - sadly minidump-stackwalk is not able to symbolicate it. To reproduce you only need to run the following Marionette test:

# coding=UTF-8

from urllib.parse import quote

from marionette_driver import By
from marionette_harness import (
    MarionetteTestCase,
    WindowManagerMixin,
)

BOILERPLATES = {
    "html": "<!doctype html>\n<meta charset={charset}>\n{src}",
    "xhtml": """<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
  <head>
    <title>XHTML might be the future</title>
  </head>

  <body>
    {src}
  </body>
</html>""",
    "xml": """<?xml version="1.0" encoding="{charset}"?>\n{src}""",
}

MIME_TYPES = {
    "html": "text/html",
    "xhtml": "application/xhtml+xml",
    "xml": "text/xml",
}


def inline(doc, doctype="html", charset="UTF-8"):
    content = BOILERPLATES[doctype].format(src=doc, charset=charset)

    return f"data:{MIME_TYPES[doctype]},{quote(content)}"


class TestMinimizedTestCase(WindowManagerMixin, MarionetteTestCase):
    def setUp(self):
        super(TestMinimizedTestCase, self).setUp()

    def tearDown(self):
        if (hasattr(self, "close_all_tabs")):
            self.close_all_tabs()

        super(TestMinimizedTestCase, self).tearDown()

    def test_parent_process_events(self):
        # self.marionette.set_pref("layout.css.devPixelsPerPx", "1")
        self.marionette.set_pref("remote.events.async.wheel.enabled", True)

        self.marionette.navigate(
            inline("""
                <div id="iframe-container" style="height: 200px">
                    <iframe srcdoc="<div>foo" style="height: 201px; border: none"></iframe>
                </div>
                <div style="height: 200vh;"></div>
                """))
        target = self.marionette.find_element(By.CSS_SELECTOR, "iframe")

        wheel_chain = self.marionette.actions.sequence("wheel", "wheel_id")
        wheel_chain \
            .scroll(0, 0, 0, 40, origin=target, duration=0) \
            .scroll(0, 0, 0, 20, origin=target, duration=0)
        wheel_chain.perform()

Oh, this was actually a code path which is basically invalid. While performing the action sequence in the parent process we still synthesize the events by default through the content process. So dispatchEvent needs to be updated to actually run the EventUtils code in the parent process.

Depends on: 1920959

Now I've updated the code so that EventUtils dispatches the widget event through the chrome window and by using browser window based coordinates the wheel scrolling action works perfectly. The code still needs to be cleaned-up and WebDriver tests written for it.

I was actually curious how certain wpt tests will now behave. So I've checked overscroll-behavior-root.html because it has the preference test.events.async.enabled set to true. The test passes fine as well when changing the preference to the upcoming remote.events.async.wheel.enabled=true. But it also passes with the value set to false. This made me wonder and I reverted the preference change and tried with test.events.async.enabled=false and it passes as well. I assume that should not be the case and the test doesn't really reflect the reality?

Dan could you please take a look?

Flags: needinfo?(drobertson)
Attachment #9349591 - Attachment description: Bug 1848957 - [wpt] Update /dom/events/scrolling/ metadata to get tests passing locally on MacOS. → WIP: Bug 1848957 - [wpt] Enable widget events for wheel scrolling tests.

Indeed the test gets passed without test.events.async.enabled=true on a bare mozilla-central; https://hg-edge.mozilla.org/mozilla-central/rev/8f14ccced260b4aec913a27a1af8ef0339631924

With adding two rAFs after scroll action just like;

    await new Promise(resolve => requestAnimationFrame(resolve));  
    await new Promise(resolve => requestAnimationFrame(resolve));

With this change, the test fails properly without test.events.async.enabled=true .

I am pretty sure that the test originally had failed without the pref. So I wonder WebDriver's action handling has changed, e.g. not waiting for the state the given action was processed or some such?

I am not sure whether the current WebDriver's behavior is correct or not. I think it's fine to change the test itself.

Flags: needinfo?(drobertson)

Henrik, if you believe the current WebDriver behavior is correct, please file a bug for changing overscroll-behavior-root.html. Thanks!

Flags: needinfo?(hskupin)

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

Henrik, if you believe the current WebDriver behavior is correct, please file a bug for changing overscroll-behavior-root.html. Thanks!

I'll take a look on Monday and will check also with older releases (before I started all the action related refactoring work). If nothing changed on our side I'll file the bug to get the test properly updated.

Otherwise I can see intermittent test failures for wheel scrolling on elements of a Shadow DOM locally on MacOS only. On try the tests work as it looks like (but I'll retrigger the specific Wdspec test job soon).

These tests are failing:

 0:36.83 INFO | `/webdriver/tests/classic/perform_actions/wheel.py` | `test_scroll_shadow_tree[outer-open]`   | **FAIL: 1/10, PASS: 9/10** | `assert 0 == 1` |
 0:36.83 INFO | `/webdriver/tests/classic/perform_actions/wheel.py` | `test_scroll_shadow_tree[outer-closed]` | **FAIL: 4/10, PASS: 6/10** | `assert 0 == 1` |
 0:36.83 INFO | `/webdriver/tests/classic/perform_actions/wheel.py` | `test_scroll_shadow_tree[inner-open]`   | **FAIL: 4/10, PASS: 6/10** | `assert 0 == 1` |
 0:36.83 INFO | `/webdriver/tests/classic/perform_actions/wheel.py` | `test_scroll_shadow_tree[inner-closed]` | **FAIL: 3/10, PASS: 7/10** | `assert 0 == 1` |

Given that we do not enable widget events for wheel scroll yet, I will most likely get them fixed via a follow-up bug.

See Also: → 1962560
Attachment #9349593 - Attachment description: WIP: Bug 1848957 - [remote] Add support for wheel scroll widget events. → WIP: Bug 1848957 - [remote] Add support for wheel scroll widget events to Marionette and WebDriver BiDi.
Attachment #9349593 - Attachment description: WIP: Bug 1848957 - [remote] Add support for wheel scroll widget events to Marionette and WebDriver BiDi. → Bug 1848957 - [remote] Add support for wheel scroll widget events to Marionette and WebDriver BiDi.
Attachment #9349591 - Attachment description: WIP: Bug 1848957 - [wpt] Enable widget events for wheel scrolling tests. → Bug 1848957 - [wpt] Enable widget events for wheel scrolling tests.
No longer blocks: 1850607
Attachment #9484300 - Attachment is obsolete: true
Attachment #9484299 - Attachment is obsolete: true

Comment on attachment 9349591 [details]
Bug 1848957 - [wpt] Enable widget events for wheel scrolling tests.

Revision D186547 was moved to bug 1963567. Setting attachment 9349591 [details] to obsolete.

Attachment #9349591 - Attachment is obsolete: true
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f39697266d29 [wpt] Fix handling of result in wpt's mach command. r=jgraham https://hg.mozilla.org/integration/autoland/rev/8e3c549c6335 [remote] Re-enable async-event-dispatching job in CI. r=jmaher,webdriver-reviewers,jgraham https://hg.mozilla.org/integration/autoland/rev/c1166d3df835 [remote] Add support for wheel scroll widget events to Marionette and WebDriver BiDi. r=webdriver-reviewers,jdescottes
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/52329 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m16] → [webdriver:m16], [wptsync upstream]

https://hg.mozilla.org/mozilla-central/rev/f39697266d29
https://hg.mozilla.org/mozilla-central/rev/8e3c549c6335
https://hg.mozilla.org/mozilla-central/rev/c1166d3df835

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox140: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 140 Branch
Upstream PR merged by moz-wptsync-bot
Blocks: 1894654
Blocks: 2086540
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: