1146243 - Allow synthetic events to be dispatched asynchronously

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

bug 1140293 aims to fix tests that are broken by synthesizeWheel becoming asynchronous with APZ enabled. It turns out that's not quite good enough, because APZ also needs to know about keyboard and mouse events. APZ is in the parent process, but those events can dispatch through PuppetWidget in a child process.

Fixing synthesizeWheel tests was laborious enough. To expedite getting APZ enabled, I'd like to add a flag that lets us choose whether or not test events are asynchronous. A new pref seems a lot easier than propagating an actual flag through everywhere.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Patch has new pref that tests can set and fixes up the few places that need to care about it.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

(removed debug stmts)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

FYI: nsIDOMWindowUtils::SendKeyEvent() will be dropped soon. So, you don't need to support it. Actually, EventUtils.js already uses nsITextInputProcessor for dispatching keyboard events (and composition events).

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8581491 [details] [diff] [review]
patch

Review of attachment 8581491 [details] [diff] [review]:
-----------------------------------------------------------------

See comment below. But before you do that I'd like to see an example of how you plan to use this pref. Is it going to be set on individual tests? If you're having to modify individual tests to use this, it might be easier to not do this work at all and just use DOMWindowUtils.sendNativeXXX to generate the events, using my patch on bug 1146349. That will inject it directly into the widget (on windows anyway, but I can try to write linux support too if you want) and from there it will go through the flow that real user input takes, including going through DispatchInputEvent. I feel like that's a better long-term fix but without knowing exactly which tests are affected by this I don't know if it will solve your current problem. Thoughts?

::: dom/ipc/PBrowser.ipdl
@@ +493,5 @@
>      async SetDimensions(uint32_t aFlags, int32_t aX, int32_t aY, int32_t aCx, int32_t aCy);
>  
>      prio(high) sync SynthesizedMouseWheelEvent(WidgetWheelEvent event);
> +    prio(high) sync SynthesizedMouseEvent(WidgetMouseEvent event);
> +    prio(high) sync SynthesizedKeyboardEvent(WidgetKeyboardEvent event);

So pretty much everywhere else in the code we have functions with "synthesize" in the name, they take raw event data (like action, coordinates, deltas, etc). I wanted to write a patch to rename SynthesizedMouseWheelEvent to something like DispatchMouseWheelEvent, but since you're touching this code anyway, could you rename these functions to DispatchMouseWheelEvent, DispatchMouseEvent, and DispatchKeyboardEvent? It's also odd to have this called from a "Dispatch" function in PuppetWidget and then go back out to a "Dispatch" function in TabParent.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

"MouseWheelEvent" is legacy event name which was implemented by IE and WebKit. Please call just "WheelEvent" for the D3E's wheel event.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8581491 [details] [diff] [review]
> patch
> 
> Review of attachment 8581491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See comment below. But before you do that I'd like to see an example of how
> you plan to use this pref. Is it going to be set on individual tests? 

Yes, it will just be part of setUserPref/pushPrefEnv calls in tests that use synthesizeWheel alongside other input events.

> If you're having to modify individual tests to use this, it might be easier to
> not do this work at all and just use DOMWindowUtils.sendNativeXXX to
> generate the events, using my patch on bug 1146349. That will inject it
> directly into the widget (on windows anyway, but I can try to write linux
> support too if you want) and from there it will go through the flow that
> real user input takes, including going through DispatchInputEvent. I feel
> like that's a better long-term fix but without knowing exactly which tests
> are affected by this I don't know if it will solve your current problem.
> Thoughts?

Yeah, though the tests still have to pass on OS X/Linux. Going the native route seems fine if that patch will land shortly and we can have a fallback implementation that does fake-o event construction. Then we could transition to native support elsewhere whenever we want.

> So pretty much everywhere else in the code we have functions with
> "synthesize" in the name, they take raw event data (like action,
> coordinates, deltas, etc). I wanted to write a patch to rename
> SynthesizedMouseWheelEvent to something like DispatchMouseWheelEvent, but
> since you're touching this code anyway, could you rename these functions to
> DispatchMouseWheelEvent, DispatchMouseEvent, and DispatchKeyboardEvent? It's
> also odd to have this called from a "Dispatch" function in PuppetWidget and
> then go back out to a "Dispatch" function in TabParent.

Sure.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> FYI: nsIDOMWindowUtils::SendKeyEvent() will be dropped soon. So, you don't
> need to support it. Actually, EventUtils.js already uses
> nsITextInputProcessor for dispatching keyboard events (and composition
> events).

Ok, good to know. I'll make the MouseWheel change.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

bug 1146349 doesn't address TextInputProcessor, FWIW. Seems like a fair bit of surgery would be needed there to get it synthesizing through the widget, but I don't know if that's even desired, or what the interaction between TIP/TextEventDispatcher and widgets are.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: test fix example

Example - this gets test_wheeltransaction.xul passing.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8581858 [details] [diff] [review]
v2

Review of attachment 8581858 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPrefs.h
@@ +342,5 @@
>  
>    DECL_GFX_PREF(Live, "nglayout.debug.widget_update_flashing", WidgetUpdateFlashing, bool, false);
> +
> +  DECL_GFX_PREF(Live, "test.events.async.enabled",             TestEventsAsyncEnabled, bool, false);
> +  DECL_GFX_PREF(Live, "test.mousescroll",                      MouseScrollTestingEnabled, bool, false);

:)

::: widget/TextEventDispatcher.cpp
@@ +128,5 @@
> +  nsresult rv = NS_OK;
> +  if (aEvent.AsInputEvent() &&
> +      (!aEvent.mFlags.mIsSynthesizedForTests || gfxPrefs::TestEventsAsyncEnabled()))
> +  {
> +    aStatus = widget->DispatchInputEvent(aEvent.AsInputEvent());

I'm not familiar with this TextEventDispatcher stuff, so I'm not sure I'm qualified to review this. In particular I don't know why you have the mIsSynthesizedForTests check here. I assume that this code is running in the child process for the TestEventsAsyncEnabled() check to be relevant (it shouldn't be needed if this is running only in the parent-process), but I might be missing something.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
>
> I'm not familiar with this TextEventDispatcher stuff, so I'm not sure I'm
> qualified to review this. In particular I don't know why you have the
> mIsSynthesizedForTests check here. I assume that this code is running in the
> child process for the TestEventsAsyncEnabled() check to be relevant (it
> shouldn't be needed if this is running only in the parent-process), but I
> might be missing something.

My thinking was, "the only place we shouldn't be routing events through APZ, is for old tests that depend on synchronous scrolling". And - yeah, the check will only matter for code running in the child process, since the parent will be dispatching events synchronously (at least until we handle VK_DOWN/UP in apz or something).

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/884d44a41ac7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/884d44a41ac7