1191137 - Mulet does not pass key events for volume up and down through to apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[David Flanagan [:djf]]

Mulet (or the devtools responsivemode code that supports it) dispatches keyboard events to the gaia system app when the user clicks on the simulated buttons by calling SystemAppProxy.dispatchKeyboardEvent() to send keydown and keyup events.

But gecko and gaia have changed (see bug 989198, for example) so that keyboard events (for the volume keys, at least) can get passed through to individual apps.  This is done with the new mozbrowser[before|after]key[up|down] events of type BeforeAfterKeyboardEvent.

On a device an app (like the Gallery app) can receive keydown and key up events for VolumeDown and VolumeUp.  But the same app running under Mulet cannot see these events.

This is a real problem for the RedSquare project where we need to use Mulet to simulate a flip phone which has many hardware keys.

I've tried modifying SystemAppProxy to send the before and after events, but that does not seem to do it.

I suspect that we need to modify browser/devtools/responsivemode.jsm so that it dispatches its key events in some lower-level way. I don't understand the code in bug 989198, but suspect that we need to send key events via layout/base/nsPresShell.cpp or something like that.

I don't know if this should be categorized as a Mulet bug or as a dev tools bug, but I urgently need a fix for red square.

Needinfo Alexandre because he knows Mulet.  And needinfo Olli because he was the review for bug 989198.

Comment 1

[David Flanagan [:djf]]

Nominating to block v2.2r because until we get hardware, we're going to have to simulate flip phones with Mulet and we need the simulated hardware buttons to send keyboard events to apps.

Comment 2

[David Flanagan [:djf]]

Note that although I've flagged this bug for 2.2r, I'm filing this bug report against master.  I haven't tested yet with a 2.2 version of Mulet.

Comment 3

[:gerard-majax]

If it's devtools I'm not the best one.

Comment 4

[Alexandre Poirot [:ochameau]]

The related code lives here:
  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/responsivedesign/responsivedesign.jsm#479

I don't get bug 989198 as well. Especially its test:
  http://mxr.mozilla.org/mozilla-central/source/dom/events/test/test_dom_before_after_keyboard_event.html?force=1#90
I don't see any special event being fired, just a simple "a" keypress.

Comment 5

[Alexandre Poirot [:ochameau]]

I'm wondering... is it really related to bug 989198?!
That would mean these buttons are broken since last october?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I have zero idea how Mulet works, so could you point to some code how it dispatches events?
Or is it the same as that responsivedesign.jsm. That seems to use SystemAppProxy.jsm which just
dispatches plain normal DOM events.
One should use the APIs in nsIDOMWindowUtils or in case of key/IME events possibly nsITextInputProcessor to dispatch events closer to what happens when user interacts with the UA.

Comment 7

[David Flanagan [:djf]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> The related code lives here:
>  
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/
> responsivedesign/responsivedesign.jsm#479
> 

Yeah, that's the code that Mulet depends on: clicks on the simulated buttons just turn into keydown/keyup events delivered directly to the system app. They don't go through the full gecko event dispatch mechanism, so don't get expanded to include before/after events like actual hardware buttons do.

> I don't get bug 989198 as well. Especially its test:
>  
> http://mxr.mozilla.org/mozilla-central/source/dom/events/test/
> test_dom_before_after_keyboard_event.html?force=1#90
> I don't see any special event being fired, just a simple "a" keypress.

There is code in nsPresShell.cpp that expands keyup and keydown events to include the before and after events. That code handles things like not dispatching the keydown event if the mozbrowserbeforekeydown is cancelled, for example.  This is all behind a pref so that it is b2g only. See https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6916

Thanks for pointing out that test code, though. Maybe that synthesizeKey() function that the tests are using can also be used by Mulet/responsivemode.jsm

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> I'm wondering... is it really related to bug 989198?!
> That would mean these buttons are broken since last october?

I'm not sure when it all got preffed on, but I can believe this because none of our gaia apps respond to hardware buttons, right? The feature was added to support the smart TV project, I think, and Mulet wouldn't have been very useful to that project.  There's been talk of allowing a volume key to be used as a camera shutter button, but I don't think we've added any code to do that.  And even if we had, camera wouldn't work in Mulet so it wouldn't have been tested there.

(In reply to Olli Pettay [:smaug] from comment #6)
> I have zero idea how Mulet works, so could you point to some code how it
> dispatches events?
> Or is it the same as that responsivedesign.jsm. That seems to use
> SystemAppProxy.jsm which just
> dispatches plain normal DOM events.

Yes, that's the code, it just send keydown/keyup directly to the system app with nothing to dispatch those events on to the currently focused app within the system app.

> One should use the APIs in nsIDOMWindowUtils or in case of key/IME events
> possibly nsITextInputProcessor to dispatch events closer to what happens
> when user interacts with the UA.

I'll take a look at those, though I know so little about Gecko that I'm not sure I'll be able to do much with them myself. I see a lot of tests using EventUtils.synthesizeKey(). Do you suppose that is something we could use in responsivemode.jsm? (I haven't figured out where it is defined yet, unfortunately.)

Comment 8

[David Flanagan [:djf]]

Actually, I found the EventUtils.synthesizeKey() code and it uses nsITextInputProcessor, so maybe there is code there that I can copy into responsivemode.jsm

Comment 9

[David Flanagan [:djf]]

So I've found that if I use nsITextInputProcessor, I can dispatch a keydown and keyup events and the before and after events will be added, and the event will be visible to the app.  But, it only works once. After that my key events aren't being dispatched.

Comment 10

[David Flanagan [:djf]]

I just can't make this work. nsITextInputProcessor.keydown() and keyup() generate the right set of keyevents once. Then they stop working. If I switch to a new app, then I can get another keydown/keyup sequence to go through, and then they stop working again.

I've also tried using nsIDOMWindowUtils.  The dispatchDOMEventViaPresShell() method sounds perfect, but it requires an event target (like the body element) and seems to dispatch the events to that target rather than to the window where the gaia system app expects them.

nsIDOMWindowUtils also has a sendKeyEvent() method which appears to only generate old-style key events with keycode and charcode but not the key property. IIRC, this method also had the problem of working once and then not sending events anymore.

I've been trying all of these things in b2g/components/SystemAppProxy.jsm which is what responsivemode.jsm calls to send key events

Basically, I think I'm stuck. I hope someone can help me out.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Masayuki is most familiar with nsITextInputProcessor.

Comment 12

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

nsIDOMWindowUtils.sendKeyEvent() will be dropped, please use nsITextInputProcessor.

What's exception is fired when you call .keydown() at second time?

This document may help you:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITextInputProcessor

Comment 13

[David Flanagan [:djf]]

I don't get any exception at all for the second keydown/keyup pair. I just don't see any events delivered.

I've modified my code to be more like the example code in the document you linked to, but I'm still seeing the same issue.

Comment 14

[David Flanagan [:djf]]

Attachment: Modified SystemAppProxy.dispatchKeyboardEvent() function

I'm attaching the TextInputProcessor code I'm testing here. This is my modified version of the dispatchKeyboardEvent() method in m-c/b2g/components/SystemAppProxy.jsm

Do you see anything wrong with it?  The first pair of keydown and keyup events are sent correctly, but after that nothing is sent. For each new app in Gaia that I launch I can send one keydown/keyup pair, but then nothing after that.

Any idea what could be wrong Masayuki?

Comment 15

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

beginInputTransaction() should be called before every call of keydown() or keyup(). So, your TIP input transaction must be finished by another TIP instance. You need to steal the rights of dispatching key events.

Comment 16

[David Flanagan [:djf]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> beginInputTransaction() should be called before every call of keydown() or
> keyup().

I've tried it that way too, and it doesn't solve my problem, but it is good to know that this is the right way to do it.

 So, your TIP input transaction must be finished by another TIP
> instance. You need to steal the rights of dispatching key events.

Are you saying that I should create a new instance of TIP for every key event? That's not how it is done in EventUtils.js, so I think that is not what you mean.

Actually, I think I've figured out that this is a focus issue. When I click on the simulated buttons in Mulet, the iframe that holds the system app loses focus and subsequent events are not sent to it.  If I send a key event, then click on the system app I can then send further key events.

So I just need to figure out how to manage focus with XUL and ensure that focus always stays on the system app.

Comment 17

[David Flanagan [:djf]]

Yes, this was a focus issue. So I think I can fix this one myself. Patch coming later today.

:jryans would you be a good reviewer for a responsivemode patch?  I'm touching the responsivemode.inc.css file and also modifying SystemAppProxy.dispatchKeyboardEvent() in b2g/components/SystemAppProxy.jsm which I believe is used only by responsivemode.jsm

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to David Flanagan [:djf] from comment #17)
> Yes, this was a focus issue. So I think I can fix this one myself. Patch
> coming later today.
> 
> :jryans would you be a good reviewer for a responsivemode patch?  I'm
> touching the responsivemode.inc.css file and also modifying
> SystemAppProxy.dispatchKeyboardEvent() in b2g/components/SystemAppProxy.jsm
> which I believe is used only by responsivemode.jsm

Yes, I can review responsive mode changes.  I'm not a b2g reviewer, so someone else is needed for SystemAppProxy.

Comment 19

[David Flanagan [:djf]]

Attachment: patch

ochameau: would you review the SystemAppProxy part of this patch? It sends they key events with a TextInputProcessor which ends up generating the correct series of events including the mozbrowser[before|after]key[down|up] events, and allows events on the volume keys to properly get dispatched to individual apps.

jryans: would you review the responsive mode css change? It just ensures that the hardware buttons do not take keyboard focus.

I couldn't get Review Board to work (I probably need level 1 access or something) so I'm just having to attach the patch the old-fashioned way. Hope that isn't too much of a problem.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

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

Responsive changes seem good to me!

Comment 21

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

>> So, your TIP input transaction must be finished by another TIP
>> instance. You need to steal the rights of dispatching key events.
> 
> Are you saying that I should create a new instance of TIP for every key event? That's not how it is done in
> EventUtils.js, so I think that is not what you mean.

I repeated same explanation from different points. beginInputTransaction() gets the rights to dispatch key/composition events from the other TIP instances which are associated with same nsIWidget instance. If it returns false, nobody cannot (shouldn't) dispatch key/composition events. E.g., another TIP has composition.

Comment 22

[Alexandre Poirot [:ochameau]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> >> So, your TIP input transaction must be finished by another TIP
> >> instance. You need to steal the rights of dispatching key events.
> > 
> > Are you saying that I should create a new instance of TIP for every key event? That's not how it is done in
> > EventUtils.js, so I think that is not what you mean.

EventUtils does, but forms.js, which isn't tests but production code doesn't recreate TIP instance when beginInputTransaction fails.

http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1204
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#959

I imagine you can call beginInputTransaction multiple times and use one TIP instance multiple times for more than just one serie of key events. (That's my comprehension while reading these usages of it)

Comment 23

[Alexandre Poirot [:ochameau]]

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

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

Looks good, thanks for contributing to mulet!

::: b2g/components/SystemAppProxy.jsm
@@ +108,5 @@
> +
> +      // If we don't already have a TextInputProcessor, create one now
> +      if (!this.TIP) {
> +        this.TIP = Components.classes["@mozilla.org/text-input-processor;1"].
> +          createInstance(Components.interfaces.nsITextInputProcessor);

nit: please use Cc/Ci alias.

@@ +127,5 @@
> +        this.TIP.keyup(e);
> +      }
> +    }
> +    catch(e) {
> +      dump("dispatchKeyboardEvent: " + e + "\n");

nit:
Given that you have this exception handler,
instead of doing various dump() calls,
you could instead throw errors with just the error message, without the "dispatchKeyboardEvent: " prefix.
Like: throw new Error("no content window")

Comment 24

[David Flanagan [:djf]]

Attachment: patch, with review comments addressed

This patch addresses the review nits, but is otherwise the same.

Comment 25

[David Flanagan [:djf]]

Setting checkin-needed.

The patch obsoleted by the current one has r+, and I think I can carry those forward to this patch.

Sheriff: I'm gaia developer and don't know the protocol for gecko patches. And I don't even have Level 1 committer access, so I can't push to try.  This patch affects B2G and Mulet only, so I'm not actually worried about breaking anything.

I'm going to need to get this landed on 2.2r as well. Can you help with that?

Comment 26

[David Flanagan [:djf]]

Comment on attachment 8646459 [details] [diff] [review]
patch, with review comments addressed

[Triage Comment]

We need this on the v2.2r gecko branch. Since we don't have release management set up for that branch yet, I'm setting the approval flag myself, in my role as tech lead for the red square project.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/b2g-inbound/rev/6bcde12ee788

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6bcde12ee788

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/2be0f5a84b00

Comment 30

[David Flanagan [:djf]]

I screwed up by requesting this for 2.2r.  Gaia 2.2 has code landed to handle the new mozbrowserbeforekeydown and related events.  But Gecko 37 does not have the TextInputProcessor code that this patch uses to dispatch those events. That was added by bug 917322 which landed in gecko 38, I think.

So the patch in comment 29 needs to be reverted.  If we uplift bug 917322 to v2.2r, then we can land it again. But right now it just breaks the simulated hardware buttons in Mulet.

Ryan: sorry to make work for you. I don't have the permissions to revert this, nor even the Mercurial knowledge to know how to revert a patch. Could you help me with this, please?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Tomcat, can you please take care of backing out the push from comment 29? Thanks!

Comment 32

[Carsten Book [:Tomcat]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #31)
> Tomcat, can you please take care of backing out the push from comment 29?
> Thanks!

sure!

David this was done in https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/84a1c0d71454