825498 - Hardware keyboard doesn't work in content since 20121223 nightly on ASUS Transformer TF101

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Mike Hommey [:glandium]]

Last working nightly: ftp://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2012-12-22-03-08-36-mozilla-central-android/
First failing nightly: ftp://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2012-12-23-03-08-32-mozilla-central-android/

Hardware keyboard stopped working in content (i've been testing on bugzilla main page text fields), but still works in the awesomebar (not quite unexpected).

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

CC+ Jim Chen in case the IME refactor was in play here.  (?)

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Bisected to Bug 817706. Usually the content LayerView has focus and receives key events which get dispatched to Gecko, but in this case LayerView is not getting any key events (maybe because of loss of focus?)

Sriram, do you have any ideas?

Comment 3

[Sriram Ramasubramanian [:sriram]]

Aaah. I think it has to do with the Tabs. I have added standard Android Tabs to the Gecko layout. So, they might gain the access. There was a similar problem in Awesomebar and I think Lucas fixed it long long ago.

(cc-ing him and trying to find what fixes that).

Comment 5

[Dave Miller [:justdave]]

There's an adb logcat attached to bug 826590 if that's helpful at all.

Also affects Galaxy Tab 10.1 with a bluetooth keyboard.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> Aaah. I think it has to do with the Tabs. I have added standard Android Tabs
> to the Gecko layout. So, they might gain the access. There was a similar
> problem in Awesomebar and I think Lucas fixed it long long ago.
> 
> (cc-ing him and trying to find what fixes that).

You're using TabWidget/TabHost in the tabs panel now. They will steal focus when you switch tabs. I guess you can avoid that by doing something like:

setDescendantFocusability(ViewGroup.FOCUS_BLOCK_DESCENDANTS);
setCurrentTab(...)
setDescendantFocusability(ViewGroup.FOCUS_AFTER_DESCENDANTS);

But I wonder if this wouldn't cause a11y issues somehow?

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This seems to work. I could interact with the content with this.
(However we don't show any focused state or anything on any of the widgets .. which feels soooo wrong!)

Comment 8

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae0673777ec8

Comment 9

[Dave Miller [:justdave]]

This is still broken in the 01-18 build.

Comment 10

[Aaron Train [:aaronmt]]

This never merged to central yet.

Comment 11

[Dave Miller [:justdave]]

(In reply to Aaron Train [:aaronmt] from comment #10)
> This never merged to central yet.

ah, that would explain it. I saw the commit and failed to notice it was only to inbound.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/ae0673777ec8

Comment 13

[Eitan Isaacson [:eeejay]]

This still affects beta. Is that what the tracking flag is for. I hope so!

Comment 14

[Sriram Ramasubramanian [:sriram]]

These patches are not even in aurora yet.

Comment 15

[Aaron Train [:aaronmt]]

If it still affects beta (19), then let's check if it needs to uplift.

Comment 16

[Marco Zehe (:MarcoZ)]

So, do we need to get this patch moved into Aurora and Beta? has a final determination of that happened? If so, please request approvals, or ask me to, since this affects users of TalkBack, too.

Comment 17

[Sriram Ramasubramanian [:sriram]]

This might not be needed. We are moving away from Spinner and Tabs approach. Hence not uplifting there. Aurora should be stable in a couple of days once the icon tabs in Bug 836043 lands.

Comment 18

[Marco Zehe (:MarcoZ)]

Comment on attachment 702577 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): 817706
User impact if declined: Blind users won't be able to navigate web content with hardware keyboard devices.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky):

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 817706
User impact if declined: Blind users won't be able to navigate web content on hardware keyboard devices.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: None.

Also requesting approval on release in case we build a 19.0.x to take this as a ride-along, since the current release on Play Store is broken for affected blind users.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 702577 [details] [diff] [review]
Patch

Let's get this out asap on Beta to verify fixed there, flagged for tracking 19 in case there is a respin.

Comment 20

[Marco Zehe (:MarcoZ)]

Attachment: Test for uint32_t plug-in value.

Comment 21

[Marco Zehe (:MarcoZ)]

Comment on attachment 718057 [details] [diff] [review]
Test for uint32_t plug-in value.

Sorry for the spam! I fat-fingered an HG command and it attached this patch to the bug. :(

Comment 22

[Marco Zehe (:MarcoZ)]

Landed on beta in slightly rebased form: https://hg.mozilla.org/releases/mozilla-beta/rev/e0811ecae310

Comment 23

[Marco Zehe (:MarcoZ)]

Also, since the bug has a target milestone of Firefox21, I believe the Firefox21 tracking flag can be set to fixed as well.

Comment 24

[Marco Zehe (:MarcoZ)]

Sriram, can you give me pointers where this patch would be applied to Firefox19 to fix the problem? The code this was inserted into for 21 and 20 didn't exist yet in 19, but the problem does.

Comment 25

[Sriram Ramasubramanian [:sriram]]

I dont think this is available in 19. Which means, we might not need this there.

Comment 26

[Marco Zehe (:MarcoZ)]

Hm, the problem is the same, though: Navigation into and inside the web content via a hardware keyboard is broken in 19. Different kind of bug?

Comment 27

[Sriram Ramasubramanian [:sriram]]

That could be a different bug. The two states were need for a TabWidget, that was added and removed in 20. So, that shouldn't be a problem for 19.

Comment 28

[Alex Keybl [:akeybl]]

Is the FF19 behavior suspected to be a regression from FF18? If so, let's definitely get a separate bug on file. It sounds like we can remove the FF19 tracking nomination here though.

Comment 29

[Marco Zehe (:MarcoZ)]

(In reply to Alex Keybl [:akeybl] from comment #28)
> Is the FF19 behavior suspected to be a regression from FF18? If so, let's
> definitely get a separate bug on file.

Yes, this is definitely a regression from FF 18. However, as for a separate bug:

a) It only affects 19 now, since the above patch in slightly rebased form fixes it in 20, and 21 and 22 already have it fixed, and
b) we have no guarantee we'll actually find and fix the problem before we spin a 19.0.x, if that is at all being spun in the next 2-3 weeks.

So our consensus is to inform the user base of the problem and suggest to them to use 20 beta once the next one containing the fix comes out onto the store.