Closed Bug 1094066 Opened 5 years ago Closed 5 years ago

APP-CANCELLED key should handled by app first then fall back to system app

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dwi2, Assigned: dwi2)

References

Details

(Whiteboard: [ft:conndevices])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
johnhu
: review+
Details | Review
There is a known issue after bug 1014418 lands. According to https://bugzilla.mozilla.org/show_bug.cgi?id=1014418#c20 and https://bugzilla.mozilla.org/show_bug.cgi?id=989198#c194, the actual key event sequence of the patch in bug 989198 should be:
  1. mozbrowserbeforekeydown event in embedder frame (i.e. System app)
  2. keydown event in embedder frame (*)
  3. keydown event in embedded frame
  4. mozbrowserafterkeydown event in embedder frame

(*): Sequence number 2 is not in our assumption when we start implementing this mechanism. 

The sequence number 2 above might cause newly implemented browser_key_event_manager in System app accidentally intercepts and processes key event even before other app does. This would make 'app-cancelled' scenario failed.

However this has no effect on all current apps. Because no app directly listens to KeyEvent of VolumeUp/VolumeDown now.
To post my findings here:

TL;DR, keydown/keyup events for System app when focus is on System and keydown/keyup events described in https://bugzilla.mozilla.org/show_bug.cgi?id=1094066#c0 have different |event.target|. We could use |event.target| to distinguish them.

In current implementation in System app, we listen to keydown/keyup for all keys except 'Home' and 'Power' because we treat them as 'APP-CANCELLED' scenario keys. (See https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/browser_key_event_manager.js#L65 and https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent#Part_2_-_Key_event_handling_customization )

However, according to https://bugzilla.mozilla.org/show_bug.cgi?id=1094066#c0 we might intercept and process 'VolumeDown' and 'VolumeUp' keys in system app earlier than foreground apps have any chance to process it.

I found out that when focus is on System app, target of keydown/keyup event is element in system app (usually body element). But target of keydown/keyup event described in https://bugzilla.mozilla.org/show_bug.cgi?id=1094066#c0 are always mozbrowser iframe of the focused app. We probably could utilize event.target to decides whether we should process keydown/keyup event in System app.
Assignee: nobody → tzhuang
Whiteboard: [ft:conndevices]
Summary: VolumeUp and VolumeDown key should handled by app first then fall back to system app → APP-CANCELLED key should handled by app first then fall back to system app
Change the title because this should apply to all APP-CANCELLED keys, not just VolumeUp and VolumeDown.
The definition of APP-CANCELLED keys are at https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent#Dispatch_KeyboardEvent_across_BrowserElements

VolumeUp and VolumeDown are also defined as APP-CANCELLED keys.
Status: NEW → ASSIGNED
Basically, this patch is just the same as the one in https://bugzilla.mozilla.org/show_bug.cgi?id=1103339

But we'll patch it in tv_apps/
Depends on: 1014405
Attached file pull request
Hi John, 
Could you please review the patch? Thanks

With this patch, we could let embedded app to handle APP-CANCELLED keys first, then fallback to system app if key event is not processed.
Attachment #8529465 - Flags: review?(im)
Comment on attachment 8529465 [details] [review]
pull request

Thanks for this patch. It looks good to me.
Attachment #8529465 - Flags: review?(im) → review+
Thanks,

Landed on master
https://github.com/mozilla-b2g/gaia/commit/677f5b2f87dccb57d8c41134586bf4d55ea4a53f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.