Pass modifier key info from TabParent::HandleSingleTap to TabChild

RESOLVED FIXED in Firefox 39

Status

()

Core
Event Handling
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mbrubeck, Assigned: botond)

Tracking

(Depends on: 1 bug)

28 Branch
mozilla39
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Depends on: 943546
(Assignee)

Comment 1

3 years ago
I'm touching this code in bug 1005815 and bug 1127066; I'm thinking I might as well implement this.
Assignee: nobody → botond
Depends on: 1127066
(Assignee)

Comment 2

3 years ago
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())
(Assignee)

Comment 3

3 years ago
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?
Flags: needinfo?(mbrubeck)
(Reporter)

Comment 4

3 years ago
(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.
Flags: needinfo?(mbrubeck)
(Assignee)

Comment 5

3 years ago
Created attachment 8563165 [details] [diff] [review]
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.
Attachment #8563165 - Flags: review?(mbrubeck)
Attachment #8563165 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 6

3 years ago
Created attachment 8563170 [details] [diff] [review]
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).
Attachment #8563170 - Flags: review?(mbrubeck)
Attachment #8563170 - Flags: review?(bugmail.mozilla)
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?
Attachment #8563165 - Flags: review?(bugmail.mozilla) → review+
Oops, ignore above comment.
Attachment #8563170 - Flags: review?(bugmail.mozilla) → review+
(Reporter)

Updated

3 years ago
Attachment #8563165 - Flags: review?(mbrubeck) → review+
(Reporter)

Comment 9

3 years ago
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. :/
Attachment #8563170 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8569520 [details] [diff] [review]
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.
Attachment #8563170 - Attachment is obsolete: true
Attachment #8569520 - Flags: review?(mbrubeck)
Attachment #8569520 - Flags: review?(bugmail.mozilla)
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.
Attachment #8569520 - Flags: review?(bugmail.mozilla) → review+
(Reporter)

Updated

3 years ago
Attachment #8569520 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e70a3fb50d37
https://hg.mozilla.org/mozilla-central/rev/caf0d391ceae
https://hg.mozilla.org/mozilla-central/rev/f9b76a07fcc6
https://hg.mozilla.org/mozilla-central/rev/e70a3fb50d37
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.