Closed Bug 1014418 Opened 10 years ago Closed 10 years ago

Dispatching hardware button event in Gaia

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

(Reporter: dwi2, Assigned: dwi2)

References

Details

(Whiteboard: [FT:Stream3])

Attachments

(2 files)

46 bytes, text/x-github-pull-request
timdream
: review+
alive
: review+
zcampbell
: review+
timdream
: feedback+
Details | Review
46 bytes, text/x-github-pull-request
timdream
: review+
Details | Review
Currently all hardware key event are filtered in shell.js, and they are wrapped in mozChromeEvent and get dispatched to system app.

After landing of bug 989198, do we need to adopt this new key event dispatching mechanism in Gaia?
(Bug 989198 will not remove mozChromeEvent-wrapped hardware key events. So current hardware key handling mechanism should work as usual after bug 989198)

see https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent#Dispatch_KeyboardEvent_across_BrowserElements
Depends on: 989198
blocking-b2g: --- → 2.1?
Whiteboard: [FT:Stream3]
Blocks: 1026354
feature-b2g: --- → 2.1
blocking-b2g: 2.1? → ---
Blocks: 1038073
Blocks: Stream3_2.1
Assignee: nobody → tzhuang
Status update: here's WIP. https://github.com/dwi2/gaia/tree/bug1014418
Once bug 989198 landed, I'll start to set review flag.
QA Whiteboard: [2.1-feature-qa+]
QA Whiteboard: [2.1-feature-qa+] → [2.1-feature-qa?]
QA Whiteboard: [2.1-feature-qa?]
Whiteboard: [FT:Stream3] → [FT:Stream3][2.1-feature-qa+]
Flags: in-moztrap?(fyen)
QA Contact: fyen
Attached file Pull request
Hi Tim,

Since this is a somewhat not-small patch and we are still waiting bug 989198 to land, I'll like to have your feedback on my patch first.

Many thanks
Attachment #8471485 - Flags: feedback?(timdream)
Plan to land it by the end of v2.1 sprint 3 (August 29).
Target Milestone: --- → 2.1 S3 (29aug)
Here is some info that might help people to be more clear about the dependency of this bug:

TL;DR  the order of landing MUST be: bug 989198 -> this bug (bug 1014418) -> bug 1038073


bug 989198 is Gecko modification on implementation of new sets of hardware key events (called BeforeAfterKeybaordEvent). It will NOT remove mozChromeEvent with these detail.type yet:
  * home-button-press
  * home-button-release
  * sleep-button-press
  * sleep-button-release
  * volume-up-button-press
  * volume-up-button-release
  * volume-down-button-press
  * volume-down-button-release

The patch in bug 989198 does two things: 
1. implement BeforeAfterKeybaordEvent which will be deliver to mozbrowser-embedder iframe
2. unleash KeyboardEvent (In shell.js, KeyboardEvent was stopped from propagating, see http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#434)

After bug 989198 landed, Gaia could receive all KeyBoardEvent, BeforeAfterKeybaordEvent and the mozChromeEvent listed above. Ideally the original hardware buttons module in Gaia should work as usual.

However, the patch of this bug (bug 1014418) listens to KeyboardEvent and BeforeAfterKeybaordEvent only. That is why we must wait for bug 989198 fixed.
Comment on attachment 8471485 [details] [review]
Pull request

Thank you very much. This code is structurally correct so I have no good feedback about it besides some naming changes.

I wonder if the patch handles the key events within the System frame itself? Will the key events got ignored if the current focused element is a, say, system dialog <div> within the keyboard app?

What if the current focused frame is a normal iframe within the System app (although there are not such use case currently)?
Attachment #8471485 - Flags: feedback?(timdream) → feedback+
BTW, your patch includes changes renaming mozChromeEvent to softwareButtonEvent, but comment 4 didn't clarify anything about this event. Is that the event we will be getting if the focused frame is the System app itself?
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #5)
> Comment on attachment 8471485 [details] [review]
> Pull request
> 
> Thank you very much. This code is structurally correct so I have no good
> feedback about it besides some naming changes.
> 
> I wonder if the patch handles the key events within the System frame itself?
> Will the key events got ignored if the current focused element is a, say,
> system dialog <div> within the keyboard app?
When current focused element is in system app, system app will receive 'keyup' and 'keydown' event. That's why we also register event listener for these two event in |HardwareButtons.prototype.start| to handle the case.

> 
> What if the current focused frame is a normal iframe within the System app
> (although there are not such use case currently)?
These new 'BeforeAfterKeybaordEvent' dispatch to embedder frame ONLY IF embedded frame is mozbrowser iframe and has 'embed-apps' privilege. 
So there will be no 'BeforeAfterKeybaordEvent' for system app if current focused frame is a normal iframe.
> These new 'BeforeAfterKeybaordEvent' dispatch to embedder frame ONLY IF
> embedded frame is mozbrowser iframe and has 'embed-apps' privilege. 
> So there will be no 'BeforeAfterKeybaordEvent' for system app if current
> focused frame is a normal iframe.
Sorry, I think I state it wrong in comment 7. 

After discussion with Gina, no matter what type of iframe is within system app, system app will always received 'BeforeAfterKeybaordEvent'
No longer blocks: Stream3_2.1
Confirmed with EM/EPM, and this can be landed before FL.
Hi Zac and Malini,

We are about to land bug 989198 and this bug soon. After we land the patch https://bugzilla.mozilla.org/attachment.cgi?id=8471485, Gaia will no longer listen to mozChromeEvent of these detail.type below:
  * home-button-press
  * home-button-release
  * sleep-button-press
  * sleep-button-release
  * volume-up-button-press
  * volume-up-button-release
  * volume-down-button-press
  * volume-down-button-release

Instead, we will listen to KeyboardEvent (bug 989198 will unleash KeyboardEvent from shell.js) and newly implemented BeforeAfterKeyboardEvent as indication of pressing or releasing hardware key button. 
These event.type are listed below:

  * mozbrowserbeforekeydown
  * mozbrowserafterkeydown
  * mozbrowserbeforekeyup
  * mozbrowserafterkeyup

For detail description of this new mechanism, please refer to https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent

I am not sure if these changes break any automation test or not. (At least I didn't see anything broken in Gip and Gij, see https://tbpl.mozilla.org/?rev=4ef13344b56f6047010ea5618785e94ba96cde7d&tree=Gaia-Try)
So I think I need to inform you about these change up front. Thanks
Flags: needinfo?(zcampbell)
Flags: needinfo?(mdas)
Hi :tzhuang, it doesn't break anything on TBPL now because 989198 is not landed (as I understand the chain of dependency).

Although this patch will almost certainly break Gip once that bug is landed. 

You just need to update the events in here in your patch:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L476

Thanks!
Flags: needinfo?(zcampbell)
Thanks for letting me know. Zac has already addressed the only concern I have :)
Flags: needinfo?(mdas)
See Also: → 1057198
bug 1057198 is the case that Tim mentioned in comment 5, a normal iframe in system app.
QA Whiteboard: [2.1-feature-slip+]
Kevin - Is this still happening for 2.1? Just want to double check if I still need to have QA resources here, given that this feature missed the 2.1 merge cutoff for features.
Flags: needinfo?(khu)
I was told by Jenny Liu that connected device team is targeting to land this by this week. 
Jenny / Joe, do you have comments about this? 

Thank you.
Flags: needinfo?(khu)
Flags: needinfo?(jeliu)
Flags: needinfo?(jcheng)
redirecting ni to ehung
but yes the goal is to ask for approval to land this
Flags: needinfo?(jcheng)
Flags: needinfo?(ehung)
Yes, but it depends on bug 989198.
Flags: needinfo?(ehung)
Move to 2.2 because bug 989198 was postponed to 2.2. Also clean needinfo to Jenny.
feature-b2g: 2.1 → 2.2?
Flags: needinfo?(jeliu)
Flags: in-moztrap?(fyen)
Whiteboard: [FT:Stream3][2.1-feature-qa+] → [FT:Stream3]
QA Whiteboard: [2.1-feature-slip+]
To update status here, bug 989198 has landed.
But there is one thing in landed patch different than what we expected before:
According to https://bugzilla.mozilla.org/show_bug.cgi?id=989198#c194 and related discussion,
when we have focused on an OOP embedded mozbrowser iframe, every 'keyup' and 'keydown' event will ALSO be dispatched to embedder iframe (system app). For example, say we press volumedown, the event sequence will be like
mozbrowserbeforekeydown event in system app -> keydown in embedded iframe -> keydown in system app -> mozbrowserafterkeydown in embedded iframe.

I am still investigating whether this impact current implementation (https://bugzilla.mozilla.org/attachment.cgi?id=8471485) and how much is the impact.
The correct 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. 
This does not affect 'system-only', 'system-first', and 'app-first' key. (see https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent#Dispatch_KeyboardEvent_across_BrowserElements)

However, this does affect our current implementation on 'app-cancelled' keys like VolumeUp ad VolumeDown. That is because we listen to both 'keydown'/'keyup' and 'mozbrowserafterkeydown'/'mozbrowserafterkeyup' in System app. We listen to 'keydown'/'keyup' in system app is for the case of handling volume key when focus is on System app.

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.
See Also: → 1014405
After discussed with John Hu and Gina Yeh, we think that we should file a follow up bug for the problem in comment 19 and comment 20. Because currently there are no apps listen to volume-up and volume-down key event, and of course no app would override behavior of volume-up and volume-down key event. That means we still have time to fix app-cancelled scenario.
Comment on attachment 8471485 [details] [review]
Pull request

Evelyn suggest that we should keep landing window between this patch and bug 989198 as small as possible. Now 989198 is pretty closed to landing, so I am sending review now.

Hi Tim,
Please help to review hardware_buttons and browser_key_event_manager part. This should be pretty much the same as the version you gave feedback before, just some naming change.

Hi Alive,
Please help to review accessibility part. The point of change is to make accessibility module listens to system internal event ('volumeup' and 'volumedown') instead of 'volume-up-button-press'/'volume-down-button-press' wrapped in mozChromeEvent.

Hi Zac, 
I modified gaia_test.py as you suggest in comment 11. Please help to review it.

Thanks
Attachment #8471485 - Flags: review?(zcampbell)
Attachment #8471485 - Flags: review?(timdream)
Attachment #8471485 - Flags: review?(alive)
Comment on attachment 8471485 [details] [review]
Pull request

Hi Eitan,

We change accessibility module to listen to system app internal event 'volumeup' and 'volumedown'. Because all key events should be processed by hardware_buttons module for clean separation. And Gecko might remove mozChromeEvent of these type in near future:

  'home-button-press'
  'home-button-release'
  'sleep-button-press'
  'sleep-button-release'
  'volume-up-button-press'
  'volume-up-button-release'
  'volume-down-button-press'
  'volume-down-button-release'

I'd like to have your feedback, thanks
Attachment #8471485 - Flags: feedback?(eitan)
Comment on attachment 8471485 [details] [review]
Pull request

Looks good.
Attachment #8471485 - Flags: review?(timdream) → review+
Comment on attachment 8471485 [details] [review]
Pull request

Well done. Let's wait a11y guy feedback on accessibility.js change.
Attachment #8471485 - Flags: review?(alive) → review+
Comment on attachment 8471485 [details] [review]
Pull request

Theoretically, this is fine. I couldn't test this, since it requires the entire queue of patches from bug 989198, and they didn't easily apply here. As long as the functionality is actually tested, and proves to work I am fine with it.

Since I can't test this here, I can't actually provide feedback.
Attachment #8471485 - Flags: feedback?(eitan)
Comment on attachment 8471485 [details] [review]
Pull request

Like Eitan I don't want to review this until bug 989198 has landed.

I want to test it on device because I don't trust CI to catch functionality bugs.
Attachment #8471485 - Flags: review?(zcampbell)
Comment on attachment 8471485 [details] [review]
Pull request

Hi Zac,

Bug 989198 has finally landed yesterday.

Could you please review my patch? Thanks.
Attachment #8471485 - Flags: review?(zcampbell)
Comment on attachment 8471485 [details] [review]
Pull request

r+, checked on device and it works well.
Attachment #8471485 - Flags: review?(zcampbell) → review+
Thanks.

Gaia-Try result seems ok.
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=69cca15b6b70

Landed on master
https://github.com/mozilla-b2g/gaia/commit/c4b403023bb657d4d5e1cadfb5cc55e757c337cf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 1093955
Backed out for breaking the sleep button and causing bugs like bug 1093955. I believe this can make it impossible to turn the screen off or back on, causing users to get stuck.

https://github.com/mozilla-b2g/gaia/commit/3c5f8c398cf13df65bc6d1b0eed795e3f42e7160
Status: RESOLVED → REOPENED
Flags: needinfo?(tzhuang)
Resolution: FIXED → ---
Thanks for the catch.


Firefox Accounts input field is inside a plain iframe in System app. 

  <iframe id="fxa-iframe" src="../fxa/fxa_module.html#login">

That cause issue like 1093955. Because in this case, Gecko will not dispatch BeforeAfterKeyboardEvent or KeyEvent of embedded frame to embedder frame.

According to bug 989198, the embedder frame (which is System app) needs a new permission 'before-after-keyboard-event' to receive BeforeAfterKeyboardEvent.

I'll fix it.
Flags: needinfo?(tzhuang)
Blocks: 1094066
Hi Tim,

This patch is pretty much the same as https://bugzilla.mozilla.org/attachment.cgi?id=8471485, 
except that I add one more permission to system app: 'before-after-keyboard-event'.

This permission is introduced in bug 989198. In order to allow embedder frame (system app) to receive BeforeAfterKeybaordEvent of its plain embedded iframe, embedder frame need this permission. Otherwise, embedder frame could only receive BeforeAfterKeybaordEvent of its mozbrowser embedded iframe. This is the case you mentioned in comment 5.

I've test this patch with STR in bug 1093955. And it works fine.
But I think I still need your review before landing this. 

Thanks
Attachment #8517882 - Flags: review?(timdream)
Comment on attachment 8517882 [details] [review]
pull request to master (reland)

Sounds good! Thank you very much.
Attachment #8517882 - Flags: review?(timdream) → review+
Gaia try is green, landed on master.

https://github.com/mozilla-b2g/gaia/commit/940d52127168dc694882967ccae37f6e294c9566
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Either this patch of bug 989198 lead to a very annoying regression. The STR are:

- open any web page in a browser frame.
- scroll down.
- tap the "home" button to go to the homescreen.
- use the cardview to go back to the web page.

Expected:
The scroll position hasn't change.

Observerd:
We scrolled up to the top.
Keywords: qawanted
Blocks: 1096146
Hi Fabrice,

Thanks. 
I filed a bug https://bugzilla.mozilla.org/show_bug.cgi?id=1096146 and I am working on it.
Depends on: 1103339
Depends on: 1106844
Depends on: 1106818
Blocks: 1108447
feature-b2g: 2.2? → ---
Per Comment 38, clear qawanted.
Keywords: qawanted
See Also: → 1220996
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: