943537 - Pass modifier key info from TabParent::HandleSingleTap to TabChild

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Matt Brubeck (:mbrubeck)]

Bug 942689 added support for modifier keys (e.g. shift, ctrl) in "tap" gestures sent by AsyncPanZoomController.  To use these in Firefox OS, we'll need to pass them from TabParent::HandleSingleTap to TabChild, and then use them in the mouse events dispatched by TabChild::RecvHandleSingleTap.

Comment 1

[Botond Ballo [:botond]]

I'm touching this code in bug 1005815 and bug 1127066; I'm thinking I might as well implement this.

Comment 2

[Botond Ballo [:botond]]

For reference, here's Matt's description of what should be done with the modifiers:

21:00	mbrubeck	botond: IIRC, it's for when TabChild generates mousemove/mousedown/mouseup/click events
21:00	mbrubeck	botond: These events should have their ctrlKey, altKey, etc. properties set (also getModifierState())

Comment 3

[Botond Ballo [:botond]]

It looks pretty straightforward to implement this for single-tap events.

I notice bug 942689 also added the modifiers for long-taps and double-taps. Should anything be done for those?

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Botond Ballo [:botond] from comment #3)
> It looks pretty straightforward to implement this for single-tap events.
> 
> I notice bug 942689 also added the modifiers for long-taps and double-taps.
> Should anything be done for those?

If they can result in simulated mouse or click events (e.g. right-click? double-click?) then yes, though it's probably not critical.

If there are no simulated mouse actions, then there's probably no need.

Comment 5

[Botond Ballo [:botond]]

Attachment: Part 1 - Have GeckoContentController accept modifiers in 'widget' format instead of 'DOM' foramt

The rationale here is that the modifiers originate in widget format, and the GeckoCC implementations consume them in widget format (by setting them on  events dispatched with nsIWidget::DispatchEvent()), so it seems kind of pointless to convert them to DOM format in between.

Comment 6

[Botond Ballo [:botond]]

Attachment: Part 2 - Handle modifiers in RemoteContentController and ChromeProcessController

Matt, a couple of specific things I'd like your feedback on:

  - When handling a long tap, we first dispatch a 'contextmenu'
    event, and then if that wasn't handled, an NS_MOUSE_MOZLONGTAP
    event. I added the modifiers to the NS_MOUSE_MOZLONGTAP; should
    I also add them to the contextmenu event? (If so, I would have
    to convert them to DOM format, because the API for dispatching
    that particular event happens to take them in DOM format (only
    to convert them back to widget format in its implementation...),
    but that's doable if necessary).

  - Is there a way I can test these patches? I don't have a Firefox
    OS phone with a physical keyboard (and even if I did, my 
    understanding is that bug 943546 would also have to be fixed on
    the gonk side first).

Comment 7

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

Comment on attachment 8563165 [details] [diff] [review]
Part 1 - Have GeckoContentController accept modifiers in 'widget' format instead of 'DOM' foramt

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

Presumably PBrowser.ipdl doesn't need any changes?

Comment 8

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

Oops, ignore above comment.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8563170 [details] [diff] [review]
Part 2 - Handle modifiers in RemoteContentController and ChromeProcessController

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

(In reply to Botond Ballo [:botond] from comment #6)
>   - When handling a long tap, we first dispatch a 'contextmenu'
>     event, and then if that wasn't handled, an NS_MOUSE_MOZLONGTAP
>     event. I added the modifiers to the NS_MOUSE_MOZLONGTAP; should
>     I also add them to the contextmenu event?

Desktop browsers do set the modifier attributes for 'contextmenu' events, so Firefox OS probably should for compatibility.  I have no opinion on how important that work is, though.

>   - Is there a way I can test these patches?

I can't think of any. :/

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 2 - Handle modifiers in RemoteContentController and ChromeProcessController

> (In reply to Botond Ballo [:botond] from comment #6)
> >   - When handling a long tap, we first dispatch a 'contextmenu'
> >     event, and then if that wasn't handled, an NS_MOUSE_MOZLONGTAP
> >     event. I added the modifiers to the NS_MOUSE_MOZLONGTAP; should
> >     I also add them to the contextmenu event?
> 
> Desktop browsers do set the modifier attributes for 'contextmenu' events, so
> Firefox OS probably should for compatibility.  I have no opinion on how
> important that work is, though.

I resurrected WidgetModifiersToDOMModifiers in APZEventState.cpp and used it to propagate modifiers to the 'contextmenu' event.

Comment 11

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9091f14ec4ef

Comment 12

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

Comment on attachment 8569520 [details] [diff] [review]
Part 2 - Handle modifiers in RemoteContentController and ChromeProcessController

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

::: gfx/layers/apz/util/APZEventState.cpp
@@ +206,5 @@
>  
> +  // Converting the modifiers to DOM format for the DispatchMouseEvent call
> +  // is the most useless thing ever because nsDOMWindowUtils::SendMouseEvent
> +  // just converts them back to widget format, but that API has many callers,
> +  // including in JS code, so it's not trivial to change.

We can probably avoid doing this if we use APZCCH::DispatchSynthesizedMouseEvent instead of APZCCH::DispatchMouseEvent. In the end they all funnel down to widget->DispatchEvent so we should be able to merge the two with appropriate tweaking of flags and params. I'm fine with doing that in a follow-up though.

Comment 13

[Botond Ballo [:botond]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/caf0d391ceae
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b76a07fcc6

Comment 14

[Botond Ballo [:botond]]

Needed a follow-up to fix bustage introduced while rebasing the patches across bug 1133492:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e70a3fb50d37

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/caf0d391ceae
https://hg.mozilla.org/mozilla-central/rev/f9b76a07fcc6
https://hg.mozilla.org/mozilla-central/rev/e70a3fb50d37