Closed Bug 1146243 Opened 5 years ago Closed 5 years ago

Allow synthetic events to be dispatched asynchronously

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Patch has new pref that tests can set and fixes up the few places that need to care about it.
Attachment #8581490 - Flags: review?(bugmail.mozilla)
Attached patch patch (obsolete) — Splinter Review
(removed debug stmts)
Attachment #8581490 - Attachment is obsolete: true
Attachment #8581490 - Flags: review?(bugmail.mozilla)
Attachment #8581491 - Flags: review?(bugmail.mozilla)
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 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.
Attachment #8581491 - Flags: review?(bugmail.mozilla)
"MouseWheelEvent" is legacy event name which was implemented by IE and WebKit. Please call just "WheelEvent" for the D3E's wheel event.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
Attached patch v2Splinter Review
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.
Attachment #8581491 - Attachment is obsolete: true
Attachment #8581858 - Flags: review?(bugmail.mozilla)
Attached patch test fix exampleSplinter Review
Example - this gets test_wheeltransaction.xul passing.
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.
Attachment #8581858 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
https://hg.mozilla.org/mozilla-central/rev/884d44a41ac7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.