Home button not working anymore

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gerard-majax, Assigned: selin)

Tracking

({regression})

unspecified
mozilla37
ARM
Gonk (Firefox OS)
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

4 years ago
Masayuki, I noticed that you landed a lot of patches yesterday, related to buttons on Gonk, maybe there is something related to those ?
Flags: needinfo?(masayuki)

Comment 2

4 years ago
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

4 years ago
Furthermore, using emulated home button or home gesture works fine.
(Reporter)

Comment 4

4 years ago
According to dattaz, regression range would be between 499bab536adc251463aeead2cdfaf57149f8da08 and 82e8b540fb334b21c6562b7b6e62e17d910ea75a
(Reporter)

Comment 5

4 years ago
And no issue on my Flame :(
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)
Blocks: 1027477
Flags: needinfo?(masayuki)
Perhaps, "MozHomeScreen"?
(Reporter)

Comment 8

4 years ago
Sorry, but I don't get your point at all.
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.
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
(Reporter)

Comment 11

4 years ago
Ok, so reverting bug 1027477 does fix the issue on my Desire Z.
(Reporter)

Comment 12

4 years ago
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.
Flags: needinfo?(masayuki)
(Reporter)

Comment 13

4 years ago
Created attachment 8543005 [details]
Gecko workaround

Here is the change that does the trick for me.
Attachment #8543005 - Flags: feedback?(masayuki)
(Reporter)

Comment 14

4 years ago
(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 on attachment 8543005 [details]
Gecko workaround

Shouldn't use hack here.

We considered that Android and Gonk should use same key mapping.
Flags: needinfo?(masayuki)
Attachment #8543005 - Flags: feedback?(masayuki) → feedback-

Comment 16

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

Comment 17

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

Comment 19

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

Comment 22

4 years ago
Created attachment 8543013 [details]
Debug with the Gecko hack mapping Home to AKEYCODE_HOME
(Reporter)

Comment 23

4 years ago
Created attachment 8543015 [details]
Debug with Gecko mapping Home to AKEYCODE_MOVE_HOME
(Reporter)

Comment 24

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

Comment 27

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

Comment 29

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

Comment 30

4 years ago
(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 ?
(Reporter)

Updated

4 years ago
Component: GonkIntegration → Gaia::System::Input Mgmt
Flags: needinfo?(tzhuang)
(Reporter)

Comment 31

4 years ago
Created attachment 8543024 [details]
Gaia hack

This makes everything works.
Attachment #8543005 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
blocking-b2g: --- → 2.2?
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
Flags: needinfo?(tzhuang) → needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
Duplicate of this bug: 1117213
(Reporter)

Comment 35

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

Comment 36

4 years ago
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.
Flags: needinfo?(tzhuang)
Flags: needinfo?(masayuki)
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.
(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.
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.
Component: Gaia::System::Input Mgmt → DOM: Events
Flags: needinfo?(selin)
Product: Firefox OS → Core
Assignee: nobody → selin
Flags: needinfo?(selin)
Gaia work mentioned in comment 39 shall be fixed in bug 1117654.
Flags: needinfo?(tzhuang)
(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.
Created attachment 8544467 [details] [diff] [review]
Patch Gecko, v1

Add MozHomeScreen key name for home button according to the proposal in comment 9.
Attachment #8544467 - Flags: review?(masayuki)
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().
Flags: needinfo?(masayuki)
Attachment #8544467 - Flags: review?(masayuki)
Created attachment 8544984 [details] [diff] [review]
Patch Gecko, v2
Attachment #8544467 - Attachment is obsolete: true
Attachment #8544984 - Flags: review?(masayuki)
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.
Attachment #8544984 - Flags: review?(masayuki) → review+
Created attachment 8545033 [details] [diff] [review]
Patch Gecko, v3

Adding comments mentioned by Masayuki.
Attachment #8544984 - Attachment is obsolete: true
Attachment #8545033 - Flags: review+
Created attachment 8545648 [details] [diff] [review]
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
Attachment #8545033 - Attachment is obsolete: true
Attachment #8545648 - Flags: review?(masayuki)
Comment on attachment 8545648 [details] [diff] [review]
Patch Gecko, v4

Cannot use MOZ_UTF16("Moz") in the previous patch?
(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
Flags: needinfo?(masayuki)
Comment on attachment 8545648 [details] [diff] [review]
Patch Gecko, v4

Okay, then, let's take this.
Flags: needinfo?(masayuki)
Attachment #8545648 - Flags: review?(masayuki) → review+
Depends on: 1119673
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1893588af0a5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Do you know why Flame-KK v2.2 is unaffected?
If it makes sense, please remove the nomination.
Flags: needinfo?(selin)
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.
Flags: needinfo?(selin)
Removing the nom since it's not happening on current Flame-2.2.
blocking-b2g: 2.2? → -
You need to log in before you can comment on or make changes to this bug.