755648 - Fix the key event dance between system app and apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1

Sadly we need this kind of logic to live in chrome to properly handle home button, back button and other hardware buttons.

After this fix, the precedence of the key events are:

A) back button
1) check if the keyboard is open, if open, close it,
2) else, send the event to app, if not canceled,
3) send another event to system

B) home button
1) dispatch a event to system, if not canceled
2) allowing the original event to continue to apps

C) other buttons
1) allowing the original event handled by the app all the way
2) if it's not canceled by the app, send another event to system

For A-1, mozChromeEvent with detail.type = 'maybe-show-keyboard' is used. webapi.js will not handle key events directly anymore.

This patch will fix the following Gaia issues:
https://github.com/andreasgal/gaia/issues/1347
https://github.com/andreasgal/gaia/issues/1342
https://github.com/andreasgal/gaia/issues/1321

This will not fix
https://github.com/andreasgal/gaia/issues/1398

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Regression: volume doesn't work when app is open. Will deliver patch v2.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v2

So ChromeWindow will not get another event from dispatchKeyToSystemApp() after all... my bad.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v2

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Any plan on this? Vivien?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #4)
> Any plan on this? Vivien?

The current plan for Key Events is the following:
 - fire key events on the current active window
 - authorize a white list of key events to bubble from windows to windows via the browser API
 - if the event if caught at some point and stop, then the system app don't do anything. Otherwise the default action can be initiated.
 

In the case of the HOME button, it should likely be caught by the platform to be converted into an HOME intent or something similar.

Does that make sense?

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

CC+ Thinker

Even though it's the desired and nature way to process events, I was told by Thinker that bubbling from one window to another is hard to implement. 

Anyway, do we have an ETA on that? If not, I would still suggest that we should land this first. :)

(In reply to Vivien Nicolas (:vingtetun) from comment #5)
> The current plan for Key Events is the following:
>  - fire key events on the current active window
>  - authorize a white list of key events to bubble from windows to windows
> via the browser API
>  - if the event if caught at some point and stop, then the system app don't
> do anything. Otherwise the default action can be initiated.
>  
> 
> In the case of the HOME button, it should likely be caught by the platform
> to be converted into an HOME intent or something similar.
> 
> Does that make sense?

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #6)
> CC+ Thinker
> 
> Even though it's the desired and nature way to process events, I was told by
> Thinker that bubbling from one window to another is hard to implement. 
> 
> Anyway, do we have an ETA on that? If not, I would still suggest that we
> should land this first. :)

Adding Mounir on the CC list since he his working on implementing the key events bubbling part.


> 
> (In reply to Vivien Nicolas (:vingtetun) from comment #5)
> > The current plan for Key Events is the following:
> >  - fire key events on the current active window
> >  - authorize a white list of key events to bubble from windows to windows
> > via the browser API
> >  - if the event if caught at some point and stop, then the system app don't
> > do anything. Otherwise the default action can be initiated.
> >  
> > 
> > In the case of the HOME button, it should likely be caught by the platform
> > to be converted into an HOME intent or something similar.
> > 
> > Does that make sense?

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to Vivien Nicolas (:vingtetun) from comment #7)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #6)
> > CC+ Thinker
> > 
> > Even though it's the desired and nature way to process events, I was told by
> > Thinker that bubbling from one window to another is hard to implement. 
> > 
> > Anyway, do we have an ETA on that? If not, I would still suggest that we
> > should land this first. :)
> 
> Adding Mounir on the CC list since he his working on implementing the key
> events bubbling part.

I have a working patch doing that.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bubbling is easy, capturing is hard.  We're going to break capturing semantics in general.  "Meh".

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> Bubbling is easy, capturing is hard.  We're going to break capturing
> semantics in general.  "Meh".

I would love to see bug 757486 implements both capturing phase and the bubbling phase of the key events, which makes every thing in consistent with how web works today, and also makes the HOME button not the exception.

Until them, we were just migrating from chrome script hack to <iframe mozbrowser> hack...

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We can't implement "true" capturing semantics because that would require blocking the window manager on windows, and recursively the browser-app UI on tabs, which we absolutely can't do for performance reasons.

We can't implement "true" bubbling semantics either, because that would require cross-process references to JS objects, but we can at least make the bubbling phase synchronous if we need to, and pick an event target in each process that sorta makes sense.  I'd prefer not to make it synchronous though, if we can avoid that.

Comment 12

[Mounir Lamouri (:mounir)]

I think this should be fixed by bug 757486. Tim and Vivien, could you confirm that and close the bug?

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #12)
> I think this should be fixed by bug 757486. Tim and Vivien, could you
> confirm that and close the bug?

The code in shell.js still need to be updated to reflect the changes.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 624333 [details] [diff] [review]
Patch v2

Removing r? until there is a new patch that takes in account the last change on the browser API.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've reported bug 759017 and specified the problem with the new browser API. We cannot change shell.js before that bug is fixed or the buttons will be not responsive entirely.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v3

Stripped down version of Patch v2, have bug 757486 to forward some keys instead.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v4

I am sorry, Patch v3 is not review-able due to improper diff.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 629699 [details] [diff] [review]
Patch v4

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

The more I see this key handling code the more I think it should be moved as much as possible to the system app if possible :)

::: b2g/chrome/content/shell.js
@@ +232,5 @@
> +          if (isDefaultPrevented) {
> +            evt.preventDefault();
> +            evt.stopPropagation();
> +          }
> +        }

Remove this code.

The keyboard bug is a different bug that should be fixed with a different solution.
For example the mozKeyboard component could listen for VK_ESCAPE itself and close the keyboard in this case if it is opened  or it could be that the event is captured on the capture phase from the system app to close the keyboard.

@@ +243,5 @@
> +            case evt.DOM_VK_HOME:
> +            case evt.DOM_VK_SLEEP:
> +              this.forwardKeyToContent(evt);
> +              evt.preventDefault();
> +              evt.stopPropagation();

I start to wonder if evt.DOM_VK_HOME should not also be a whitelisted keyevent that can't be stopped by calling event.stopPropagation() or event.preventDefault(). That would let us move this code to the main system app.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Vivien Nicolas (:vingtetun) from comment #18)
> The keyboard bug is a different bug that should be fixed with a different
> solution.

Bug 761953 submitted and patched!

> @@ +243,5 @@
> > +            case evt.DOM_VK_HOME:
> > +            case evt.DOM_VK_SLEEP:
> > +              this.forwardKeyToContent(evt);
> > +              evt.preventDefault();
> > +              evt.stopPropagation();
> 
> I start to wonder if evt.DOM_VK_HOME should not also be a whitelisted
> keyevent that can't be stopped by calling event.stopPropagation() or
> event.preventDefault(). That would let us move this code to the main system
> app.

I don't understand, some logical typo here? bug 757486 does not handle VK_HOME and VK_SLEEP buttons -- for that reason, at least, in shell.js, we would need to

1) react to some keyups for APIs not yet exposed to content, e.g., real volume change.
2) stop HOME and SLEEP from going to non-System App frame, and forward them to System App.

I will submit another patch to do exactly that.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v5

additionally, this patch do

1) Correctly evaluate the original event is going to by checking |(evt.target.ownerDocument == content.document)|. The original |(evt.target.ownerDocument.defaultView == content)| is incorrect -- one is ChromeWindow and another is Window, so it's always false.

2) Dispatch the forwarded event on <html>, not |window|.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #21)
> 1) Correctly evaluate the original event is going to by checking
> |(evt.target.ownerDocument == content.document)|. The original
> |(evt.target.ownerDocument.defaultView == content)| is incorrect -- one is
> ChromeWindow and another is Window, so it's always false.

Humm... this is not correct. |evt.target.ownerDocument.defaultView| can be ChromeWindow only if the user press the button *before* touching the screen. If we were focused on the System app frame, both of them is indeed |window|.

We do need to forward the key from ChromeWindow to content window in that case though.

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 630490 [details] [diff] [review]
Patch v5

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

Very close! Sorry for all the back and forth.

::: b2g/chrome/content/shell.js
@@ +228,5 @@
> +        let rootContentEvt = (evt.target.ownerDocument == content.document);
> +        if (!rootContentEvt && evt.eventPhase == evt.CAPTURING_PHASE) {
> +          switch (evt.keyCode) {
> +            case evt.DOM_VK_HOME:
> +            case evt.DOM_VK_SLEEP:

Why do you want to prevent VK_SLEEP to be caught by content? It can be used by the camera app for example to take a picture.

@@ +229,5 @@
> +        if (!rootContentEvt && evt.eventPhase == evt.CAPTURING_PHASE) {
> +          switch (evt.keyCode) {
> +            case evt.DOM_VK_HOME:
> +            case evt.DOM_VK_SLEEP:
> +              this.forwardKeyToContent(evt);

Wrap this call into a setTimeout to be sure to not stop the execution of the origina;l event because of thisduplicated event.

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Wait, can we clearly the desired behavior here?

For SLEEP key and the HOME key, my assumption is that the apps should not be allowed to interact with them _at all_. You seems to against that, and would like to have BOTH apps and the system to receive the keys.

I would say that does not make sense. If I want to have the camera key hijack the sleep button for taking pictures, I would also allow it to cancel the key -- in that case, the sleep button should fall to the processing logic of bug 757486 and not here.

Allowing both apps and system to receive the event while not allowing one from canceling another will results useless event -- the screen would turn off by system as the same moment when the camera pp is capturing the picture for you.

What do you want exactly for SLEEP and HOME keys? I am very sad that after 5 patches I am not being told what's the desired behavior before hand.

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #24)
> Wait, can we clearly the desired behavior here?
> 
> For SLEEP key and the HOME key, my assumption is that the apps should not be
> allowed to interact with them _at all_. You seems to against that, and would
> like to have BOTH apps and the system to receive the keys.
> 

It sounds fine for me if an app can interact with SLEEP (HOME is a different story). 

> I would say that does not make sense. If I want to have the camera key
> hijack the sleep button for taking pictures, I would also allow it to cancel
> the key -- in that case, the sleep button should fall to the processing
> logic of bug 757486 and not here.

It should be there!
  
> What do you want exactly for SLEEP and HOME keys? I am very sad that after 5
> patches I am not being told what's the desired behavior before hand.

In the short term I want the HOME key to not be dispatched/cancellable by apps. The other keys are fine.

In the long term, the HOME key could fire a key event too. If this event could be caught during the capture phase by the System App before apps proceed it that's fine. If it can't it should be something else. But for now I would like to focus on the other keys. Does that make sense?

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v6

Per discussion on IRC, this is the patch do what everyone wanted and not to process the SLEEP key.

We need to file another bug to add SLEEP key to the whilelist for handling by mozbrowser.

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v7

Syntax error in Patch v6 ... :'(

And I forget to revert the |(evt.target.ownerDocument.defaultView == content)| thing.

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch to commit

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3b3452a993

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e3b3452a993