Closed Bug 1106844 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified)

VERIFIED FIXED
2.2 S2 (19dec)
blocking-b2g 2.2+
Tracking Status
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified

People

(Reporter: cinnes, Assigned: dwi2)

References

()

Details

(Keywords: regression, Whiteboard: [2.2-exploratory-1] [ft:conndevices])

Attachments

(2 files, 3 obsolete files)

Attached file 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
Flags: needinfo?(dharris)
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
QA Whiteboard: [QAnalyst-Triage?]
[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.
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?]
Component: Gaia::System → Gaia::System::Window Mgmt
Flags: needinfo?(dharris)
QA Contact: pcheng
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Caused by patch for Bug 1014418 - can you take a look Tzu-Lin?
Blocks: 1014418
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(tzhuang)
QA Contact: pcheng
I am working on it. 

keep the NI
Removing SystemsFE tag, due to this being a windows management bug.
Whiteboard: [2.2-exploratory-3][systemsfe] → [2.2-exploratory-1]
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
Flags: needinfo?(tzhuang) → needinfo?(gyeh)
regression
blocking-b2g: 2.2? → 2.2+
I think it's a gecko bug. Let me fix it and thanks Tzu-Lin for his investigation. :)
Assignee: nobody → gyeh
Component: Gaia::System::Window Mgmt → DOM: Events
Flags: needinfo?(gyeh)
Product: Firefox OS → Core
Whiteboard: [2.2-exploratory-1] → [2.2-exploratory-1] [ft:conndevices]
Target Milestone: --- → 2.2 S2 (19dec)
We skipped System app unexpectedly when building up the target chain. That's why System App failed to receive before/after events.
Summary: [Notifications] Utility tray "clear all" function causes home button to become unresponsive → [Stingray] iframe with permission 'before-after-keyboard-event' should receive before/after key events
Depends on: 989198
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.
Attachment #8536404 - Flags: review?(bugs)
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.)
Attachment #8536404 - Flags: review?(bugs) → review-
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?
Attachment #8536404 - Attachment is obsolete: true
Attachment #8537040 - Flags: review?(bugs)
Wrong patch was attached. :|
Attachment #8537040 - Attachment is obsolete: true
Attachment #8537040 - Flags: review?(bugs)
Attachment #8537041 - Flags: review?(bugs)
(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 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?
Attachment #8537041 - Flags: review?(bugs) → review-
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.)
Attachment #8537041 - Attachment is obsolete: true
Tzu-Lin, can you check the implementation of System App? I think we should transfer focus to other elements when the button is disabled.
Flags: needinfo?(tzhuang)
I think this could be solved by blurring focus away when user click on 'clearAll' button
Assignee: gyeh → tzhuang
Flags: needinfo?(tzhuang)
Component: DOM: Events → Gaia::System
Product: Core → Firefox OS
Summary: [Stingray] iframe with permission 'before-after-keyboard-event' should receive before/after key events → [Stingray] prevent focus stays on disabled elements in order to receive keyboard event
Attached file 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
Attachment #8538350 - Flags: review?(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.
Flags: needinfo?(gyeh)
Comment on attachment 8538350 [details] [review]
pull request

r+ for blocker, better having test
Attachment #8538350 - Flags: review?(alive) → review+
Blocks: 1113461
Thanks,

I filed a follow up bug for adding tests - https://bugzilla.mozilla.org/show_bug.cgi?id=1113461
(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
Flags: needinfo?(gyeh)
treeherder is green, landed on master

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

I'll be working on tests right away
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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"
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.