1116762 - Home button not working anymore

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[:gerard-majax]

Noticed this on several devices since very recently. Home button just do not react. We have spotted this at least on Desire Z and Open C. I'm rebuilding for Flame to check.

Comment 1

[:gerard-majax]

Masayuki, I noticed that you landed a lot of patches yesterday, related to buttons on Gonk, maybe there is something related to those ?

Comment 2

[Xavier Laurent]

Confirming this on Open C, although home button is not completely broken since after having scrolled homescreen to bottom, pressing it brings me back to top.

Gaia-Rev        26d479f0fccb7174e06255121e4e938c1b280d67
Gecko-Rev       
Build-ID        20141231061125
Version         37.0a1
Device-Name     ZTE_P821A10
FW-Release      4.3
FW-Incremental  eng..20140505.045022
FW-Date         Mon May  5 04:51:06 CST 2014

Comment 3

[Xavier Laurent]

Furthermore, using emulated home button or home gesture works fine.

Comment 4

[:gerard-majax]

According to dattaz, regression range would be between 499bab536adc251463aeead2cdfaf57149f8da08 and 82e8b540fb334b21c6562b7b6e62e17d910ea75a

Comment 5

[:gerard-majax]

And no issue on my Flame :(

Comment 6

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

Could be. KEYCODE_HOME was mapped to "Exit". However, D3E WG said that it's wrong and it's going to be named in D4E. If Home button handler checks the KeyboardEvent.key value, we should name it with "Moz" prefix.

https://bugzilla.mozilla.org/attachment.cgi?id=8446448&action=diff

See also:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=21136

FYI: I'm almost offline until 1/6. (Starting to work from 1/7)

Comment 7

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

Perhaps, "MozHomeScreen"?

Comment 8

[:gerard-majax]

Sorry, but I don't get your point at all.

Comment 9

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

So, we should make:

1. Define "MozHomeScreen".
2. Map it to KEYCODE_HOME.
3. Find handlers in Gaia which are doing |event.key == "Exit"|.
4. Change "Exit" to "MozHomeScreen".

So, I'm surprised at somebody using .key for Home button handling.

Comment 10

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

I wonder why such handlers don't use the traditional hack with .keyCode...
http://mxr.mozilla.org/mozilla-central/source/widget/gonk/GonkKeyMapping.h#30

Comment 11

[:gerard-majax]

Ok, so reverting bug 1027477 does fix the issue on my Desire Z.

Comment 12

[:gerard-majax]

Masayuki, it looks like changing the mapping for Android, for the Home key from AKEYCODE_MOVE_HOME to AKEYCODE_HOME does fix this, in widget/NativeKeyToDOMKeyName.h.

Comment 13

[:gerard-majax]

Attachment: Gecko workaround

Here is the change that does the trick for me.

Comment 14

[:gerard-majax]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> I wonder why such handlers don't use the traditional hack with .keyCode...
> http://mxr.mozilla.org/mozilla-central/source/widget/gonk/GonkKeyMapping.h#30

This should not even be in Gaia, in fact, I can't find any trace of it. The logic is expected to be handled on Gecko side, in b2g/chrome/content/shell.js, where DOM_VK_HOME sends the proper event to Gaia.

Comment 15

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

Comment on attachment 8543005 [details]
Gecko workaround

Shouldn't use hack here.

We considered that Android and Gonk should use same key mapping.

Comment 16

[mac]

I can confirm the issue on:

Alcatel One Touch Fire production (got from T-mobile Poland)
B2G version: 2.2.0.0-prerelease master
Platform version: 37.0a1
Build Identifier: 20141231010205
Git commit info: 2014-12-30 16:51:38 26d479f0

Comment 17

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> And no issue on my Flame :(

Well, turns out I was wrong: Home button has a erratic behavior on the Flame. It's hard to get correct STR, but tapping/using it a lot, I can consistently and quickly get into states where pressing Home does nothing (no vibration included).

Comment 18

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

(In reply to Alexandre LISSY :gerard-majax from comment #14)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> > I wonder why such handlers don't use the traditional hack with .keyCode...
> > http://mxr.mozilla.org/mozilla-central/source/widget/gonk/GonkKeyMapping.h#30
> 
> This should not even be in Gaia, in fact, I can't find any trace of it. The
> logic is expected to be handled on Gecko side, in
> b2g/chrome/content/shell.js, where DOM_VK_HOME sends the proper event to
> Gaia.

That's odd. shell.js doesn't refer .key for "Exit".

Comment 19

[:gerard-majax]

That is fun: evt.keyCode on Gecko side when pressing the Home button is 0x24 (36), so that do matches DOM_VK_HOME: http://dxr.mozilla.org/mozilla-central/source/dom/webidl/KeyEvent.webidl#40

Comment 20

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

(In reply to Alexandre LISSY :gerard-majax from comment #19)
> That is fun: evt.keyCode on Gecko side when pressing the Home button is 0x24
> (36), so that do matches DOM_VK_HOME:
> http://dxr.mozilla.org/mozilla-central/source/dom/webidl/KeyEvent.webidl#40

Yes, that's a "traditional hack" which I said in comment 10. At designing Gonk, we've not implemented KeyboardEvent.key yet. Therefore, hackers needed to use keyCode with hacky value.

Comment 21

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

So, pressing home button should be handled here if it's handled only by shell.js:
http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#376

Comment 22

[:gerard-majax]

Attachment: Debug with the Gecko hack mapping Home to AKEYCODE_HOME

Comment 23

[:gerard-majax]

Attachment: Debug with Gecko mapping Home to AKEYCODE_MOVE_HOME

Comment 24

[:gerard-majax]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> So, pressing home button should be handled here if it's handled only by
> shell.js:
> http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#376

And according to attachment 8543013 [details] and attachment 8543015 [details], it looks like we do it properly :(

Comment 25

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

Looks like that somebody in Gaia refers "Exit"... E.g.,
https://github.com/mozilla-b2g/gaia/blob/7878734c7baf7d0c665c2f838168fce21cdc95b6/apps/system/js/browser_key_event_manager.js

Comment 26

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

Okay, if nobody can work on Gaia now, please make a backout patch of bug 1027477. The mapping is really a bug, but for now, we should back it out for Nightly B2G users.

Of course, ideally, we should take the approach mentioned in comment 9.

Comment 27

[:gerard-majax]

There is a use of .key at: https://github.com/mozilla-b2g/gaia/blob/3ab1b7086144bc2ae87ac8565144e44e411351c3/apps/system/js/browser_key_event_manager.js#L32

This is being used to identify if it's a ... Home key.

Comment 28

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

KeyboardEvent.key values are not so stable since the spec is still WD.

Gaia shouldn't compare .key values directly. Gaia should have a .jsm file which defines constants of .key values which are supported by Gecko. And each code should use the constants at referring .key value. Then, Gecko hackers can check if a patch around .key implementation will break Gaia and create a patch for Gaia easy.

Comment 29

[:gerard-majax]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> Okay, if nobody can work on Gaia now, please make a backout patch of bug
> 1027477. The mapping is really a bug, but for now, we should back it out for
> Nightly B2G users.
> 
> Of course, ideally, we should take the approach mentioned in comment 9.

Until proved otherwise, it seems hard enough to trigger on Flame, and some other users of Open C don't have issues. So I'm not sure it's worth backing out.

Comment 30

[:gerard-majax]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> KeyboardEvent.key values are not so stable since the spec is still WD.
> 
> Gaia shouldn't compare .key values directly. Gaia should have a .jsm file
> which defines constants of .key values which are supported by Gecko. And
> each code should use the constants at referring .key value. Then, Gecko
> hackers can check if a patch around .key implementation will break Gaia and
> create a patch for Gaia easy.

So this is indeed a wrong assumption on Gaia-side ?

Comment 31

[:gerard-majax]

Attachment: Gaia hack

This makes everything works.

Comment 32

[Tzu-Lin Huang [:dwi2][:tzhuang]]

I am confused here. I was told to use KeyboardEvent.key instead of .keyCode or any other properties to compare key meaning. What would be the most stable KeyboardEvent property to compare in Gaia for now?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> KeyboardEvent.key values are not so stable since the spec is still WD.
> 
> Gaia shouldn't compare .key values directly. Gaia should have a .jsm file
> which defines constants of .key values which are supported by Gecko. And
> each code should use the constants at referring .key value. Then, Gecko
> hackers can check if a patch around .key implementation will break Gaia and
> create a patch for Gaia easy.
If we are going this way, I'd prefer that we put this mapping table of .key value constants in Gecko, or at least in shell.js. Because I think it would be more clear and easier for Gecko developer to check if a patch around .key implementation will ever break Gaia.

And for now, I can make a gaia patch to work around this in case we need to land the patch of bug 1027477. But first I need to know which KeyboardEvent property is the most stable for now, and I also need to know the stable mapping of these 4 keys at least:
1. Home
2. Power
3. Volume Up
4. Volume Down

Thanks

Comment 33

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

(In reply to Tzu-Lin Huang [:dwi2][:tzhuang] from comment #32)
> I am confused here. I was told to use KeyboardEvent.key instead of .keyCode
> or any other properties to compare key meaning.

There are a couple of cases.

Basically, .key is _better_ to check the key event purpose. However, the spec is not stable yet. I *think* that almost all keys which are defined in the latest key value spec are stable. But some of them are not so.

FYI: Some keys which are specific for smart phones are not stable since they are not defined in D3E. They will be defined in D4E. So, if Gaia needs to check .key values for such buttons, Gaia should have using key value list which can change easy.

> What would be the most
> stable KeyboardEvent property to compare in Gaia for now?

If keyCode value is 0, .key is the only way to check the key event purpose.

If keyCode value isn't 0 and the keyCode mapping isn't hacky (i.e., when PC keyboard is connected, the key represented by the keyCode should work as same), the .key value is almost fine. However, it may be changed the key name in the future release if the key name is renamed in the future spec.

If keyCode value isn't 0 and the keyCode mapping is hacky (i.e., like Home button), neither .keyCode value nor .key value are stable. We should get rid of this case with prefixed key values.

So, the important issue is, currently, we cannot check that Gaia uses which key values and fix that easy and same time since Gaia is outside of m-c. At least, I hope that Gaia should define key names as constants and use it in all cases.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> > KeyboardEvent.key values are not so stable since the spec is still WD.
> > 
> > Gaia shouldn't compare .key values directly. Gaia should have a .jsm file
> > which defines constants of .key values which are supported by Gecko. And
> > each code should use the constants at referring .key value. Then, Gecko
> > hackers can check if a patch around .key implementation will break Gaia and
> > create a patch for Gaia easy.
> If we are going this way, I'd prefer that we put this mapping table of .key
> value constants in Gecko, or at least in shell.js. Because I think it would
> be more clear and easier for Gecko developer to check if a patch around .key
> implementation will ever break Gaia.

Yeah, it sounds great that shell.js has constant list of key values. If it can be generated by C++ macro, it's the best. Then, Gecko developers don't need to do additionally. I.e., that means that developers never forget to sync with Gaia. If we define key values in shell.js, can other modules refer the list?

> And for now, I can make a gaia patch to work around this in case we need to
> land the patch of bug 1027477. But first I need to know which KeyboardEvent
> property is the most stable for now, and I also need to know the stable
> mapping of these 4 keys at least:
> 1. Home
> 2. Power
> 3. Volume Up
> 4. Volume Down

VolumeUp and VolumeDown should be safer than the others. Next, Power, I guess.

.keyCode might be never changed. But I'm not sure because it's hacky. Therefore, I guess that when Gonk supports USB/Bluetooth keyboard, the mapping would be changed.

I think that the safest way is, using .key but Gaia shouldn't compare its value with literal string directly.

Comment 35

[:gerard-majax]

FYI, we can currently document that it impacts:
 - ICS devices (Hamachi, Nexus S, Desire Z)
 - JB devices (Open C)

I cannot explain why it works on Flame, whether it is because of KK system or because of the way the Home button is implemented on this device.

Comment 36

[:gerard-majax]

FYI, I'm not sure what you mean precisely by "Therefore, I guess that when Gonk supports USB/Bluetooth keyboard, the mapping would be changed.", but physical keyboard are already working on Gonk: this is one of the reason I keep my Desire Z for hacking. Some of the keys are missing, and there's a lot of room for improvements, but it works.

Pairing with a Bluetooth keyboard also works, but when I tried last year on my Flatfish, key input would not get through ; indeed, as far as I could debug, this was not a Gecko-side issue, but rather a kernel side one: there were some bluetooth hid parts missing.

Comment 37

[Gina Yeh [:gyeh] [:ginayeh]]

Sorry for the late reply.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> So, pressing home button should be handled here if it's handled only by
> shell.js:
> http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#376

We used to intercept all key events in shell.js and then send mozChromeEvent to Gaia. That is to say, we call event.preventDefault() and event.stopImmediatePropagation() in shell.js. Only System App can handle those chrome events at that time.

After before/after keyboard event proposal is introduced (bug 989198), all key events are sent to Gaia. Nevertheless, we keep those chrome events temporarily and will remove them after Gaia migrating the key event handling from chrome events to new key events. I think it's time to remove these code in shell.js and I'll file a follow-up.

Comment 38

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> So, we should make:
> 
> 1. Define "MozHomeScreen".
> 2. Map it to KEYCODE_HOME.
> 3. Find handlers in Gaia which are doing |event.key == "Exit"|.
> 4. Change "Exit" to "MozHomeScreen".

Looks great to me.

Comment 39

[Gina Yeh [:gyeh] [:ginayeh]]

I'd like to have someone to fix this. Patches for both Gecko and Gaia are needed. Sean, can you help on Gecko side? I think Tzu-Lin can work on the Gaia part. Thanks.

Comment 40

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Gaia work mentioned in comment 39 shall be fixed in bug 1117654.

Comment 41

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #33)
> FYI: Some keys which are specific for smart phones are not stable since they
> are not defined in D3E. They will be defined in D4E. So, if Gaia needs to
> check .key values for such buttons, Gaia should have using key value list
> which can change easy.

> So, the important issue is, currently, we cannot check that Gaia uses which
> key values and fix that easy and same time since Gaia is outside of m-c. At
> least, I hope that Gaia should define key names as constants and use it in
> all cases.

Thanks for sharing the information. I agree to create a list for the mapping and some details haven't confirmed yet. Let's file a follow-up for further discussions and keep this bug simple.

> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> > > KeyboardEvent.key values are not so stable since the spec is still WD.
> > > 
> > > Gaia shouldn't compare .key values directly. Gaia should have a .jsm file
> > > which defines constants of .key values which are supported by Gecko. And
> > > each code should use the constants at referring .key value. Then, Gecko
> > > hackers can check if a patch around .key implementation will break Gaia and
> > > create a patch for Gaia easy.
> > If we are going this way, I'd prefer that we put this mapping table of .key
> > value constants in Gecko, or at least in shell.js. Because I think it would
> > be more clear and easier for Gecko developer to check if a patch around .key
> > implementation will ever break Gaia.
> 
> Yeah, it sounds great that shell.js has constant list of key values. If it
> can be generated by C++ macro, it's the best. Then, Gecko developers don't
> need to do additionally. I.e., that means that developers never forget to
> sync with Gaia. If we define key values in shell.js, can other modules refer
> the list?

Unfortunately, the other modules/apps cannot refer to the list if we define these constants in shell.js. So, we cann't directly put these constands in shell.js

> I think that the safest way is, using .key but Gaia shouldn't compare its
> value with literal string directly.

Yep! Let's follow the suggestion.

Comment 42

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch Gecko, v1

Add MozHomeScreen key name for home button according to the proposal in comment 9.

Comment 43

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

Comment on attachment 8544467 [details] [diff] [review]
Patch Gecko, v1

See "MozPrintableKey" definition and our internal key name should be defined in the section. Additionally, I think that we should create DEFINE_KEYNAME_WITH_MOZ_PREFIX().

Comment 44

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch Gecko, v2

Comment 45

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

Comment on attachment 8544984 [details] [diff] [review]
Patch Gecko, v2

>+#define DEFINE_KEYNAME_WITH_MOZ_PREFIX(aName) \
>+  DEFINE_KEYNAME_INTERNAL(aName, "Moz" #aName)

Thanks!

>+// HomeScreen
>+KEY_MAP_ANDROID (HomeScreen, AKEYCODE_HOME)

In NativeKeyToDOMKeyName.h, we define the key mapping for each group defined in D3E-key spec. However, "HomeScreen" isn't defined in any standard. So, could you add new section header above this? E.g.,

/******************************************************************************
 * Keys not defined by any standards
 ******************************************************************************/

With this change, r=me.

Comment 46

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch Gecko, v3

Adding comments mentioned by Masayuki.

Comment 47

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch Gecko, v4

I found the original patch would fail to build on Windows platform with "error C2308: concatenating mismatched strings" due to the mismatch between wide and narrow string literals ("Moz" and #aName) in DEFINE_KEYNAME_WITH_MOZ_PREFIX. According to [1], it appears C99 and C++11 has supported concatenating wide and narrow string literals, whereas Visual C++ doesn't. So I just brought out the newly-added macro DEFINE_KEYNAME_WITH_MOZ_PREFIX since there seems no workaround to redefine it to bypass this constraint of Visual C++.

[1] http://stackoverflow.com/questions/21788652/how-do-i-concatenate-wide-string-literals-with-prid32-priu64-etc

Comment 48

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

Comment on attachment 8545648 [details] [diff] [review]
Patch Gecko, v4

Cannot use MOZ_UTF16("Moz") in the previous patch?

Comment 49

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #48)
> Comment on attachment 8545648 [details] [diff] [review]
> Patch Gecko, v4
> 
> Cannot use MOZ_UTF16("Moz") in the previous patch?

Actually I've tried to apply MOZ_UTF16() to either of "Moz" or #aName or both of them, but unfortunately none of them worked. After a further look, MOZ_UTF16() is defined as follows [1][2]:

#define MOZ_UTF16_HELPER(s) L##s
#define MOZ_UTF16(s) MOZ_UTF16_HELPER(s)

The situation looks quite the same as what in link [1] mentioned in comment 47. As what I found in that post, the root cause appears as below:

The macro appears to expand to multiple string literals, and there seems no way to prepend an L to the second and latter string literals. For example, "PrintableKey" may expand to "P" "r" ... , and the L may only prepend to "P" but not all the other literals. 

Thus there doesn't seem a way to define a workable DEFINE_KEYNAME_WITH_MOZ_PREFIX on Win.

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Char16.h#33
[2] https://dxr.mozilla.org/mozilla-central/source/mfbt/Char16.h#227

Comment 50

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

Comment on attachment 8545648 [details] [diff] [review]
Patch Gecko, v4

Okay, then, let's take this.

Comment 51

[Sean Lin [:seanlin] (inactive from Mar 2016)]

The try-run for the latest patch. FYI.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48aaa9e242c

Waiting for the correspondent Gaia patch in bug 1119673, and then the Gecko patch in this bug is ready to land.

Comment 52

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1893588af0a5

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1893588af0a5

Comment 54

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Do you know why Flame-KK v2.2 is unaffected?
If it makes sense, please remove the nomination.

Comment 55

[Sean Lin [:seanlin] (inactive from Mar 2016)]

This issue only affects to some devices (i.e. Hamachi) whose home button uses the specific key code |AKEYCODE_HOME|. Some other devices like Flame appear to use another key code (maybe |AKEYCODE_MOVE_HOME| or something else) for their home buttons. So that's probably the reason why they're not vulnerable to this issue.

Comment 56

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Removing the nom since it's not happening on current Flame-2.2.