1773393 - (parent-actions) [meta] Dispatch input events asynchronously from the parent process

Status: NEW

(Remote Protocol :: Agent, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

So for example, sendSingleKey invokes synthesizeKey, it will not run through APZ code at all. That's NOT actually what normal browser does. It would be nice to use sendNativeBlah instead.

Comment 1

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

James could have a look how this might work once the current patches for touch and wheel support have been landed. Right now it's not clear what the trade-offs would be so running an experiment would be good.

Comment 2

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

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

James could have a look how this might work once the current patches for touch and wheel support have been landed. Right now it's not clear what the trade-offs would be so running an experiment would be good.

Setting needinfo for James given that both depending bugs have been fixed now.

Comment 3

[James Graham [:jgraham]]

I have the impression this is quite a lot of work.

https://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#653-681 suggests that we at least need to convert from viewport coordinates into screen coordinates. For touch in particular the WebDriver model assumes that we can just synthesize movement without knowing intent, but those APIs suggest that we need to know if something is intended to be a pinch guesture or a pan or similar. For wheel scrolling it appears that there's some platform-specific flags we need to set.

I think all of that is likely possible to overcome, but it might not be totally trivial. AFAICT this dispatch continues to happen in the child process, so on its own this isn't going to solve cross-process problems.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This should be added to our M7 milestone.

Couple of comments:

• Helps to unblock Bug 1799957 - test_driver's pointerMove with type "touch" does not cause scrolling and pointercancel
• Not all events need to be supported immediatelly. Priorities should be (#1) wheel events - unblocks Bug 1689546 (part of interop-2023-scrolling) and (#2) touch events - helpful for supporing scrollend and other APZ future features.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

I wonder whether dispatching widget events (still within Firefox) instead of native events (from the OS) can lower the amount of work mentioned in comment #3.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kagami [:saschanaz] from comment #5)

I wonder whether dispatching widget events (still within Firefox) instead of native events (from the OS) can lower the amount of work mentioned in comment #3.

From APZ's point of view, I believe that should be sufficient.

And I think we already have a codepath which does this: nsIWidget::DispatchInputEvent() (whereas e.g. nsIWidget::SynthesizeNativeTouchPoint will generate OS-level events).

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Great, I'd prefer the widget event way as I want to keep WPT working on headless mode.

Comment 8

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

Hm, when I check HeadlessWidget.cpp I can see that all the nsIWidget::SynthesizeNative* events like SynthesizeNativeMouseEvent actually call nsBaseWidget::DispatchInputEvent on their own. And APZ seems to get handled. So can we assume that we could actually use the available nsIWidget::SynthesizeNative* methods? If you don't know who else could we ask? Thanks.

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Hmm, maybe we can somehow force the use of HeadlessWidget in that case? But nsIWidget by itself are also implemented in platform specific way in https://searchfox.org/mozilla-central/rev/98397ff4eac3d32b815fbb33bff147297fb972d7/widget/windows/nsWindow.cpp#6713 for example.

Comment 10

[Botond Ballo [:botond]]

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

Hm, when I check HeadlessWidget.cpp I can see that all the nsIWidget::SynthesizeNative* events like SynthesizeNativeMouseEvent actually call nsBaseWidget::DispatchInputEvent on their own. And APZ seems to get handled. So can we assume that we could actually use the available nsIWidget::SynthesizeNative* methods?

Do WPT tests always run in headless mode? If so, then yes, it seems like using nsIWidget::SynthesizeNative* would have the intended effect of passing the event through APZ but not any native OS event queue.

However, if WPT tests can also run in non-headless mode, then using nsIWidget::SynthesizeNative* would mean the events going through the native OS event queue in non-headless mode. If our intention is to always skip the OS event queue and just go through APZ, nsIWidget::DispatchInputEvent() would be the better choice.

Comment 11

[Botond Ballo [:botond]]

if WPT tests can also run in non-headless mode, then [...] nsIWidget::DispatchInputEvent() would be the better choice.

Note that, for touch events, we have nsIDOMWindowUtils.sendTouchEvent() whose implementation already calls nsIWidget::DispatchInputEvent() if the pref test.events.async.enabled is set. We may be able to reuse this codepath (adjusting the condition if necessary).

Comment 12

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

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

Do WPT tests always run in headless mode? If so, then yes, it seems like using nsIWidget::SynthesizeNative* would have the intended effect of passing the event through APZ but not any native OS event queue.

No, wpt tests and out-of-tree WebDriver tests have to support both headless and headful mode.

However, if WPT tests can also run in non-headless mode, then using nsIWidget::SynthesizeNative* would mean the events going through the native OS event queue in non-headless mode. If our intention is to always skip the OS event queue and just go through APZ, nsIWidget::DispatchInputEvent() would be the better choice.

Ok, so that is then what we may have to use. Given your comment 11 it looks like that this will only work for touch events right now, and if we want to have that for other events like mouse, wheel, pen the relevant API's would have to be updated in DOMWindowUtils.cpp? Would that also apply to key events or are these handled differently?

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

I can see nsDOMWindowUtils::SendWheelEvent and nsDOMWindowUtils::SendMouseEvent also call DispatchInputEvent (see also the call diagram: https://searchfox.org/mozilla-central/query/default?q=calls-to%3A%27nsIWidget%3A%3ADispatchInputEvent%27%20depth%3A4)

But it simply has no functions for keyboard and pen.

Comment 14

[Botond Ballo [:botond]]

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

Given your comment 11 it looks like that this will only work for touch events right now, and if we want to have that for other events like mouse, wheel, pen the relevant API's would have to be updated in DOMWindowUtils.cpp?

Yep (with sendWheelEvent and sendMouseEvent already in place as Kagami mentioned).

Note that, as mentioned in comment 7, touch and wheel are the ones that have come up so far for APZ testing purposes, so even having just those for now would be a nice improvement.

Would that also apply to key events or are these handled differently?

APZ does have keyboard event support but it's more of a pure optimization (shaving off some latency by starting the scroll animation right away upon receiving the event in the compositor rather than having to do a round-trip to the content process first) than having any features tied to it, so I don't think it's a particular priority. But if we did want to route those through APZ, I think they would work the same way (adding an nsIDOMWindowUtils function that calls DispatchInputEvent).

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsIWidget::SynthesizeNative*() are designed for testing native event handlers in widget. It means that synthesizing native events may cause calling native API which may depend on the environment's settings. E.g., active keyboard layout, the system settings of various pointing device (e.g., drag start threshold), and may share the event queue of OS with the other applications. Therefore, they should be used only for testing the native event handlers.

Comment 16

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

Ok, so I tried to use the windowUtils.sendMouseEvent() method to synthesize an event on the widget level but there is nothing happening. Given by the documentation in WindowUtils.idl I have to set the aIsWidgetEventSynthesized flag to true, right? Only when it's false the click is send. No other code in the tree seems to actually use this flag yet and there doesn't seem to be some kind of MOZ_LOG that could be used to see some low level logs.

Is there something that I have to obey when setting aIsWidgetEventSynthesized to true? It doesn't matter if I synthesize the event from the content or parent process.

I've one more question as well. We usually allow to run multiple tests in parallel which means several instances of Firefox could be open. Focus is managed via enabling the focus manager, which works just fine and doesn't require the window under test to be the top-most window. Will we still be able to support this mode when we switch to widget level events?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

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

nsIWidget::SynthesizeNative*() are designed for testing native event handlers in widget.

Our swipe-to-navigation (which is triggered by pan gestures) code lives in widget. Also the android widget code has a special handling for touch events names touch resampling, it may have some impacts.

Botond is right, from the APZ standpoint nsIWidget::DispatchInputEvent is basically identical, but from the browser engine standpoint, it's different.

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

I've one more question as well. We usually allow to run multiple tests in parallel which means several instances of Firefox could be open. Focus is managed via enabling the focus manager, which works just fine and doesn't require the window under test to be the top-most window. Will we still be able to support this mode when we switch to widget level events?

I suppose some of tests have to run alone. I've seen mochitest failures where they had to move the mouse cursor on a certain place at the beginning of the test.

Comment 18

[Botond Ballo [:botond]]

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

Ok, so I tried to use the windowUtils.sendMouseEvent() method to synthesize an event on the widget level but there is nothing happening. Given by the documentation in WindowUtils.idl I have to set the aIsWidgetEventSynthesized flag to true, right?

Looking at where this flag ends up, it's here:

  // mReason indicates the reason why the event is fired:
  // - Representing mouse operation.
  // - Synthesized for emulating mousemove event when the content under the
  //   mouse cursor is scrolled.
  Reason mReason;

Based on this, it seems that aIsWidgetEventSynthesized=true is meant for a very particular use case ("emulating mousemove event when content under the mouse cursor is scrolled"), and for more typical mouse events aIsWidgetEventSynthesized=false should be used.

I've one more question as well. We usually allow to run multiple tests in parallel which means several instances of Firefox could be open. Focus is managed via enabling the focus manager, which works just fine and doesn't require the window under test to be the top-most window. Will we still be able to support this mode when we switch to widget level events?

I think that as long as the tab that's supposed to receive the event is the active tab within its window, this should work. The APZ machinery is per-window, and so while it does perform a hit-test, the hit-test doesn't need to select the targeted window, only the targeted content within that window.

Comment 19

[Botond Ballo [:botond]]

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

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

nsIWidget::SynthesizeNative*() are designed for testing native event handlers in widget.

Our swipe-to-navigation (which is triggered by pan gestures) code lives in widget. Also the android widget code has a special handling for touch events names touch resampling, it may have some impacts.

Botond is right, from the APZ standpoint nsIWidget::DispatchInputEvent is basically identical, but from the browser engine standpoint, it's different.

This is a good point, it means that swipe-to-navigate cannot be triggered from a WPT using the DispatchInputEvent approach.

Although swipe-to-navigate requires a PanGestureInput, and there already isn't a DOM event type that will map to that, unless we convert wheel events to PanGestureInput under some circumstances; I'm not sure how that would work.

Comment 20

[Botond Ballo [:botond]]

Just to wrap up the side discussion about swipe-to-nav:

• Triggering swipe-to-nav from a WPT is something that would be nice to have eventually, but it's not an urgent requirement, and in particular, not a blocker for Interop 2023.
• If we do want to allow triggering swipe-to-nav from a WPT in the future, we can probably build a solution that extends the DispatchInputEvent approach to do the additional processing that happens in our widget code, without requiring a native OS event dispatch.

Given the other advantages of avoiding synthesizing OS-native events, my suggestion is to proceed with the DispatchInputEvent approach.

Comment 21

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

Botond, after a while of a pause I would like to get back to this bug but given that most of that are unknowns to me it would be kinda helpful to know which test scenarios we actually could make working with the required changes. Do we already have some test cases (web platform tests maybe) that are currently failing because of our implementation not supporting native events? It would really help me to get a better understanding and a way to proof that changes are actually showing the expected behavior. Thanks

Comment 22

[Botond Ballo [:botond]]

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

Do we already have some test cases (web platform tests maybe) that are currently failing because of our implementation not supporting native events?

/css/css-overscroll-behavior/overscroll-behavior.html (without the patch in bug 1689546 to explicitly set test.events.async.enabled while running this) is one such test.

Will leave the needinfo on me while I follow up with some other examples.

Comment 23

[Botond Ballo [:botond]]

Dan, perhaps you could give some examples of scrollend web platform tests that would start exercising the relevant codepaths as a result of this change?

Comment 24

[Dan Robertson (:dlrobertson)]

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

Dan, perhaps you could give some examples of scrollend web platform tests that would start exercising the relevant codepaths as a result of this change?

AFAIK any test that uses touchScrollInTarget would be a test that is not currently functioning properly. The test case I'd focus on would be Tests that the target_div gets scrollend event when touch dragging (wpt.fyi).

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

[...] it would be kinda helpful to know which test scenarios we actually could make working with the required changes. Do we already have some test cases (web platform tests maybe) that are currently failing because of our implementation not supporting native events?

No guarantees I implemented scrollend properly and all things will work as expected :) I think many touchScrollIntarget tests could have a different result once native events are used, but the scrollend implementation certainly has bugs, so please reach out if something seems off.

Side note: After looking through the scrollend tests, there is significant cleanup that we should probably do to the tests. I'll start creating some bugs.

Comment 25

[Julian Descottes [:jdescottes]]

Needs investigation for sizing, might be for Henrik after PTO or if anybody else has time to handle it before that.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP: Bug 1773393 - Add an optional argument to send events via DispatchInputEvent regardless of `test.events.async.enabled`

Comment 27

[Hiroyuki Ikezoe (:hiro)]

If we've agreed on using DispatchInputEvent, an easy way to add an optional argument to the nsIDOMWindowUtils' functions which we've already used in web driver.

D183551 is the one. Some caveats on the change;

1. overscroll-behavior.html also uses click action, I ended up changing sendMouseEvent to make the test pass
2. nsIDOMWIndowUtils.sendMouseEvent has a tons of arguments, adding one more argument exceeds our XPIDL function argument number limit, we should rewrite some of the arguments with bit-wise value just like sendWheelEvent
3. the click action is invoked in a slightly different way here than other events, it should use MouseEventData, I guess that's the cause of bug 1841008

Anyway I am not going to work on this.

Comment 28

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

Thanks Hiro! I started to have a look at this bug now and your patch looks promising. Also that you all agreed on that widget level events seem to be totally enough. As such lets update the bugs summary to reflect that we don't want native events as of this state.

Running some wpt tests locally with the patch applied and some further additions made seems to work excellent at least for the scroll event wpt tests (except touch for now which I have to take a look at next). A lot of manifest files can be updated.

So I would prefer to land the platform changes on a different bug so that this one is purely about Marionette. So I'll go ahead and file such a bug soon once I'm done with the investigation for touch locally.

For Marionette itself I would propose that we add a capability that allows us to enable/disable widget level events. In case of problems users of WebDriver could then easily turn it off. We can remove that later one when we are confident that it works as expected in all the scenarios.

Comment 29

[Dan Robertson (:dlrobertson)]

Attachment: WIP: Bug 1773393 - Add wdspec test for touch scroll. r?whimboo

Comment 30

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

After noticing that Firefox currently crashes when using a scroll wheel event at widget level I'm going to make this bug a meta bug. There is quite a lot of work necessary and it's better to have individual bugs covering each event to be moved to the widget level, and that for changes on the platform side up to the Marionette / WebDriver layer.

There is already bug 1797215 for touch events so I'll go ahead and file two more bugs for wheel (scroll) and mouse.

Comment 31

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

Given that implementing async events for wheel seems to be the easiest, I also had a look at which tests actually use wheel scrolling and noticed that BrowserTestUtils.synthesizeMouse is basically the entry point for most tests.

When adding the new APIs I assume it will be enough to add some basic tests for EventUtils only, and then individual component owners could consider to also use async events at a later time?

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Yep, that sounds fine to me. For the case of overscroll-behavior.html we can change it not using mouse click events. The mouse click part is not an essential parts of the test. I guess the original intention to use the click events was just to simulate manual testing and then it was changed to an automated one.

Comment 33

[Botond Ballo [:botond]]

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

When adding the new APIs I assume it will be enough to add some basic tests for EventUtils only, and then individual component owners could consider to also use async events at a later time?

Certainly for mochitests (and other Gecko-specific tests), transitioning them to use async events does not need to be in scope for this bug.

For web platform tests, I'm not sure what level of control we have to transition them at a higher granularity. The first thing I tried (in bug 1689546) is to simply set the pref test.events.async.enabled for overscroll-behavior.html, but then I discovered that doesn't affect the WPT dashboard which runs tests with the browsers being in a default configuration :)

Comment 34

[Botond Ballo [:botond]]

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

For the case of overscroll-behavior.html we can change it not using mouse click events. The mouse click part is not an essential parts of the test. I guess the original intention to use the click events was just to simulate manual testing and then it was changed to an automated one.

I don't think we even need to change it. For the test to pass, it's only important that the events which cause scrolling go through APZ. So, fixing bug 1848957 (and not bug 1848958) should be sufficient for the test to pass, with no changes to the test itself needed.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

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

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

For the case of overscroll-behavior.html we can change it not using mouse click events. The mouse click part is not an essential parts of the test. I guess the original intention to use the click events was just to simulate manual testing and then it was changed to an automated one.

I don't think we even need to change it. For the test to pass, it's only important that the events which cause scrolling go through APZ. So, fixing bug 1848957 (and not bug 1848958) should be sufficient for the test to pass, with no changes to the test itself needed.

The overscroll-behavior.html sends a mouse click event on every test case runs, without setting test.events.async.enabled, the event goes through this branch and it will never reach to APZ, then, in turn, though I don't recall the details, any subsequent test cases didn't run properly.

Comment 36

[Botond Ballo [:botond]]

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

The overscroll-behavior.html sends a mouse click event on every test case runs, without setting test.events.async.enabled, the event goes through this branch and it will never reach to APZ, then, in turn, though I don't recall the details, any subsequent test cases didn't run properly.

That sounds like a bug we should investigate. A combination of wheel events which go through APZ and click events which do not should work in principle, unless we're looking for the click event to have some kind of effect in APZ (e.g. to cancel an ongoing animation), which doesn't seem to be the case here (it's just pressing a button).

Comment 38

[Thomas Wisniewski [:twisniewski]]

Based on bug 1689546 comment 19, this is what's keeping us from passing one of two remaining 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.

Comment 39

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

Yesterday, we enabled a new feature in both Marionette and WebDriver BiDi to process actions in the parent process and use IPC to dispatch individual events through the content process. This work took a bit longer because it was a huge refactor of our code base and we wanted to be sure that we do not regress performing of actions for our existing user base.

If issues arise, users can disable the feature by setting remote.events.async.enabled to false. This feature is now riding the release trains and will become available in Firefox 135, scheduled for February.

The next focus will be implementing widget events for the wheel input source. This will initially involve the scroll action, which should simplify the implementation process. We also plan to update and improve our Wdspec tests to validate additional events, such as scrollend, which the browser does not yet emit.