1106844 - [Stingray] prevent focus stays on disabled elements in order to receive keyboard event

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Corinne Innes [:CorinneI]]

Attachment: log.txt

Description:
If the user selects the "clear all" function, while in the utility tray, the home button becomes unresponsive. Also, if the screen is allowed to time out after selecting "clear all" the phone will require a restart/battery pull before it is usuable again. 
   
Repro Steps:
1) Update a Flame device to BuildID: 20141202040207
2) Take a screenshot/ receive a text, voicemail etc...
3) Wait for blue notification banner to disappear 
4) Pull down utility tray
5) Tap blue "clear all" option at the top right of tray 
6) Tap or long press home button 
7) Allow screen to time out
8) Press power button

Actual:
For step 6: Home button is non-functional
For step 8: Phone is unresponsive and requires restart
  
Expected: 
Home button should ALWAYS take the user home and screen time out should NOT require a hard restart
  
Environmental Variables:
Device: Flame 2.2 Master kk (319mb)(Full Flash)
BuildID: 20141202040207
Gaia: 725685831f5336cf007e36d9a812aad689604695
Gecko: 2c9781c3e9b5
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
  
Repro frequency: 100%
See attached: Logcat, Youtube video

Youtube Link: https://www.youtube.com/watch?v=hW9QoOHHAOk

Comment 1

[Corinne Innes [:CorinneI]]

Issue does NOT occur on 2.1

Environmental variables:

Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141202001201
Gaia: ccb49abe412c978a4045f0c75abff534372716c4
Gecko: 18fb67530b22
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Actual:
Home button works as intended and power button wakes up the phone

Comment 2

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

[Blocking Requested - why for this release]:

Nominating to block 2.2. The user can get completely get locked out of their phone and have to restart to access the phone again. Also a regression.

Also changing the component to windows management, as it seems more well suited.

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141107024412
Gaia: 3ca3172b9122fedf46c8411e92c48b724104aae5
Gecko: 45b2fb3afbce
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141107030312
Gaia: 940d52127168dc694882967ccae37f6e294c9566
Gecko: 60cc0b101ce9
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Gaia & Last Working Gecko - issue DOES repro
Gaia: 940d52127168dc694882967ccae37f6e294c9566
Gecko: 45b2fb3afbce

First Broken Gecko & Last Working Gaia - issue does NOT repro
Gaia: 3ca3172b9122fedf46c8411e92c48b724104aae5
Gecko: 60cc0b101ce9

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/3ca3172b9122fedf46c8411e92c48b724104aae5...940d52127168dc694882967ccae37f6e294c9566

Caused by Bug 1014418.

Comment 4

[Joshua Mitchell (Inactive)]

Caused by patch for Bug 1014418 - can you take a look Tzu-Lin?

Comment 5

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

I am working on it. 

keep the NI

Comment 6

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Removing SystemsFE tag, due to this being a windows management bug.

Comment 7

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

Pots my findings here:
After user click 'clear all' button, the focus changed from body element to 'clear all' button. After that, system app could not receive any key events. That's the reason why home button is not working.

at step 5) Tap blue "clear all" option at the top right of tray, 
if you tap any area other than 'clear all' button to make focus change back to body element, home button will become responsive again.

Gina, Could you help to check this on Gecko sides? I suspect there is something wrong after focus changed. 
Thanks

Comment 8

[Gregor Wagner [:gwagner]]

regression

Comment 9

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

I think it's a gecko bug. Let me fix it and thanks Tzu-Lin for his investigation. :)

Comment 10

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

We skipped System app unexpectedly when building up the target chain. That's why System App failed to receive before/after events.

Comment 11

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

Attachment: Patch 1(v1): Add iframe with permission 'before-after-keyboard-event' to target chain.

Comment 12

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

Comment on attachment 8536404 [details] [diff] [review]
Patch 1(v1): Add iframe with permission 'before-after-keyboard-event' to target chain.

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

smaug, this is introduced by bug 989198. Could you help to review the patch? Thanks.

Comment 13

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

Comment on attachment 8536404 [details] [diff] [review]
Patch 1(v1): Add iframe with permission 'before-after-keyboard-event' to target chain.


>   } else {
>+    frameElement = aTarget->OwnerDoc()->AsElement();
This makes no sense. Document isn't an element. This should in fact assert pretty hard in debug builds.


(We need a test for this bug.)

Comment 14

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

Attachment: Patch 1(v2): Add iframe with permission 'before-after-keyboard-event' to target chain.

Thanks for pointing that out. I've changed my implementation a little bit. nsIDocument::GetRootElement() is used to retrieve event target. Does it make sense to you?

Comment 15

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

Attachment: Patch 1(v2): Add iframe with permission 'before-after-keyboard-event' to target chain

Wrong patch was attached. :|

Comment 16

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

(In reply to Olli Pettay [:smaug] from comment #13)
> (We need a test for this bug.)

I'm still working on the test. Will send a review request once I finish it.

Comment 17

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

Comment on attachment 8537041 [details] [diff] [review]
Patch 1(v2): Add iframe with permission 'before-after-keyboard-event' to target chain

I don't understand this. Why do we want to dispatch event to root element?
The patch is against the comment above the 'if'.

What am I missing here? Do we want to dispatch before/after also in the document
of aTarget? Why not then put aTarget to aChain?

Comment 18

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

I realized that I was wrong...

The behaviour of event dispatching stuff is quite correct. Thus, no change in Gecko is needed for fixing this bug. Sorry for wasting your time, smaug.

From my observation, after clicking the clearAll button, the button is disabled (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/notifications.js#L708) and no more event can be dispatched to the disabled button. At this time, when a key is pressed, the button should be the event target but it cannot handle the event since it's been disabled before. As a result, no event is dispatched to content. That's why we failed to receive keydown event in this case. (Smaug, please correct me if I was wrong again.)

Comment 19

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

Tzu-Lin, can you check the implementation of System App? I think we should transfer focus to other elements when the button is disabled.

Comment 20

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

I think this could be solved by blurring focus away when user click on 'clearAll' button

Comment 21

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

Attachment: pull request

Hi Alive,

In this patch, we blur the focus right after user click on 'clearAll' button in utility tray, in order to let System app receives keyboard events. This is because Gecko will not dispatch keyboard event when event target is a disabled element.

Please help to review it. Thanks

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

Interesting.. does that mean every app could have this issue once
* It listens to keydown event
* It mistakenly focuses on an element which is disabled

If so, I tend to think we need to fix it before exposing hardware key events to the web.

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8538350 [details] [review]
pull request

r+ for blocker, better having test

Comment 24

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

Thanks,

I filed a follow up bug for adding tests - https://bugzilla.mozilla.org/show_bug.cgi?id=1113461

Comment 25

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> Interesting.. does that mean every app could have this issue once
> * It listens to keydown event
> * It mistakenly focuses on an element which is disabled
> 
> If so, I tend to think we need to fix it before exposing hardware key events
> to the web.

From my understanding, when focusing on one of the elements in the following list which is disabled, all events won't be dispatched to the element.

* HTMLButtonElement
* HTMLFieldSetElement
* HTMLInputElement
* HTMLSelectElement
* HTMLTextAreaElement

Comment 26

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

treeherder is green, landed on master

https://github.com/mozilla-b2g/gaia/commit/deb90229734e5397cda806fb11f2e001fba83399

I'll be working on tests right away

Comment 27

[sarsenyev]

Verified the issue is fixed on 2.2 Flame

The "home button" is active after tapping the "Clear All" in notification tray

"Flame 2.2

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141219040202
Gaia: deb90229734e5397cda806fb11f2e001fba83399
Gecko: 021b09e92d30
Gonk: e5c6b275d77ca95fb0f2051c3d2242e6e0d0e442
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0"