Closed Bug 762362 Opened 12 years ago Closed 12 years ago

Block hardware key events to content in shell.js, send mozChromeEvent instead

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-kilimanjaro:+, blocking-basecamp:+)

Status:
RESOLVED FIXED
Milestone:
DeveloperPhone
Project Flags:
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: justin.lebar+bug, Assigned: djf)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

address review comments
9.53 KB, patch
Details | Diff | Splinter Review 
Once bug 674739 is fixed, we can use proper volume keycodes for the volume keys.  Right now we use pageup/pagedown, which means that pages scroll when you press the volume keys.  It's awesome.

I'd really like this fixed sooner rather than later, because we're hardcoding VK_PAGE_UP into various places (e.g. bug 757486), and we're going to have to change all of these once we fix this bug.  The sooner we fix it, the fewer we'll have to change.
OS: Linux → All
Hardware: x86_64 → All
Thanks for opening that bug!
Unfortunately, Olli told me D3E aren't going to be implemented very soon. Are you planning to help with that?
> Unfortunately, Olli told me D3E aren't going to be implemented very soon.

Isn't this a matter of adding two items to an enum somewhere?  We don't have to implement a whole spec to get this.
According to D3E, those should be returned as DOMString by .char or .key. We don't support those attributes, right?
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> According to D3E, those should be returned as DOMString by .char or .key. We
> don't support those attributes, right?

I believe you!

If nobody has time to do this, we need to either find time, or violate the spec.  Or we could block the volume keys from being seen by content and handle them only in shell.js.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> Thanks for opening that bug!
> Unfortunately, Olli told me D3E aren't going to be implemented very soon.
Yes, supporting all D3E key event stuff is a huge task. Masayuki has started the work, but there is more to do.
We should figure out if we can do without all of D3E, then.
(In reply to Justin Lebar [:jlebar] from comment #5)
> If nobody has time to do this, we need to either find time, or violate the
> spec.  Or we could block the volume keys from being seen by content and
> handle them only in shell.js.

You cannot do that. We will be moving volume control to Gaia system itself (bug 767311). I would suggest that we use other keys (say, F23/F24) that doesn't do anything natively -- use another hack to replace this hack.

BTW, volume key events is exposed to apps (bug 757486), so if we keep this mapping for v1 then web authors would need to use these key codes to identify the volume keys.
Sounds like this should block.
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
Required for B2G. We can't overload pageup/down for this in production.
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
If that's the case we probably want to cover the hardware key code exposed to the apps altogether ...
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #8)

> You cannot do that. We will be moving volume control to Gaia system itself
> (bug 767311). I would suggest that we use other keys (say, F23/F24) that
> doesn't do anything natively -- use another hack to replace this hack.
> 

I'm in favor of this suggestions until there is D3E. I think applications should have the opportunity to catch HW keys events (except the home key).
> I think applications should have the opportunity to catch HW keys events (except the home key).

I totally agree, they should.  But it would be a real shame to report fake keys, and then change them later!
(In reply to Justin Lebar [:jlebar] from comment #13)
> > I think applications should have the opportunity to catch HW keys events (except the home key).
> 
> I totally agree, they should.  But it would be a real shame to report fake
> keys, and then change them later!

+1. That would be quite ugly. Not allowing applications to play with those keys for v1 wouldn't be so bad IMO. In the other hand, putting that kind of hack in production will not be well perceived.
(In reply to Mounir Lamouri (:mounir) from comment #14)
> (In reply to Justin Lebar [:jlebar] from comment #13)
> > > I think applications should have the opportunity to catch HW keys events (except the home key).
> > 
> > I totally agree, they should.  But it would be a real shame to report fake
> > keys, and then change them later!
> 
> +1. That would be quite ugly. Not allowing applications to play with those
> keys for v1 wouldn't be so bad IMO. In the other hand, putting that kind of
> hack in production will not be well perceived.

+1. In favor of not to expose the hardware key over expose the hacked key code. Let's ask UX on this. If we are going to do this I guess that means we are nullify the feature introduced in bug 757486 and process the key events back in shell.js again.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #15)
> (In reply to Mounir Lamouri (:mounir) from comment #14)
> > (In reply to Justin Lebar [:jlebar] from comment #13)
> > > > I think applications should have the opportunity to catch HW keys events (except the home key).
> > > 
> > > I totally agree, they should.  But it would be a real shame to report fake
> > > keys, and then change them later!
> > 
> > +1. That would be quite ugly. Not allowing applications to play with those
> > keys for v1 wouldn't be so bad IMO. In the other hand, putting that kind of
> > hack in production will not be well perceived.
> 
> +1. In favor of not to expose the hardware key over expose the hacked key
> code. Let's ask UX on this. If we are going to do this I guess that means we
> are nullify the feature introduced in bug 757486 and process the key events
> back in shell.js again.

Processing events in shell.js is painful. In the worst case those pageup/pagedown keys can be caught by shell.js which can use the incredible 'embedding' API (aka mozChromeEvent/mozContentEvent) to discuss with the System App. So most of the UI code will still live in Gaia and once D3E is here, the embedding code will be happily removed.
In my mental model, the v1 apps that use HW inputs are Home and System. 
System displays UI feedback for use of HW Power (both press, and press-and-hold), HW Vol Up, and HW Vol Down.
Home app handles Home HW button presses, toggling users between different pages / views of Home app. eg: First press returns user to Home app > primary view > last-page-viewed, while the next HW press toggles user to Home app > secondary view (task switcher).

We _have_ discussed a dedicated Camera button. It would be used to quickly wake device + open camera app when device is Locked, and then act as a shutter release when Camera app is open. TF would like to have this HW button, but it is looking increasingly less likely for v1, as of now.

I hope that helps the decision making a bit?
FWIW, have pinged TF for their input on this and will follow up, here and on the other dev-gaia thread.
Hi everyone, re: dedicated HW camera buttons, we are NOT doing them for v1. As per following from Pamela (July 8):

"Following up on conversation with Steve and Beatriz last week, it sounds like we need to hold on planning for a hardware camera button for the near term as it is a custom requirement and would require volume to make cost effective. I consider it now a mid-to-long term conversation. Hopefully more midterm than not."

"There is the option to use volume buttons for camera use but I suggest that we make this a V2 – for same reason as you originally suggested. Let´s explore this from UX perspective when we are through our next deliverables."
@Josh Does that means UX can live with no HW key access for apps for v1?

Let's make a actionable decision base on that then.
We'll still need support for the Home and System buttons, as per: https://bugzilla.mozilla.org/show_bug.cgi?id=762362#c17

Another one I _just_ thought of (that I believe we may already have): taking screenshots via some button press combo.
> Another one I _just_ thought of (that I believe we may already have): taking screenshots via some 
> button press combo.

The key combo for screenshots is captured in shell.js, not Gaia, so we can use whatever page-up/page-down hacks we want.
@Josh "no HW key access for apps" means all keys are handled in Gaia System and Gecko. Volume up / volume down / screen off / take screen shot will continue to work because they are handled in the Gaia System. That just means that app developers (our Gaia apps and 3rd party apps) cannot do anything with hardware key presses. Don't worry about that. :)
Makes sense. Thanks Tim.
@jlebar @vingtetun So we are going to close hw key access for v1. Does that allow us to use whatever key for key events? Or, we should switch to mozChromeEvent instead?
> So we are going to close hw key access for v1.

Meaning the key events won't be delivered to Gaia at all?

If so, you can use whatever keys you want...
Sorry Tim, just re-read your post (23), and what about Home screen? I saw an issue on GitHub where we had made a "necessary evil" exception, in the words of Etienne. Can we do what I described in my previous comment? https://bugzilla.mozilla.org/show_bug.cgi?id=762362#c17
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #25)
> @jlebar @vingtetun So we are going to close hw key access for v1. Does that
> allow us to use whatever key for key events? Or, we should switch to
> mozChromeEvent instead?

I would say that shell.js will receive KeyEvents and convert them to mozChromeEvent sent to the System App.

(In reply to Josh Carpenter from comment #27)
> Sorry Tim, just re-read your post (23), and what about Home screen? I saw an
> issue on GitHub where we had made a "necessary evil" exception, in the words
> of Etienne. Can we do what I described in my previous comment?
> https://bugzilla.mozilla.org/show_bug.cgi?id=762362#c17

This is something else and you don't have to worry about it. I have introduce this because the only way to exit Edit Mode was: reboot the phone. And whatever we do here it won't change anything for the HOME button, this one has always be special because it should *not* be intercept by the content.
(In reply to Vivien Nicolas (:vingtetun) from comment #28)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #25)
> > @jlebar @vingtetun So we are going to close hw key access for v1. Does that
> > allow us to use whatever key for key events? Or, we should switch to
> > mozChromeEvent instead?
> 
> I would say that shell.js will receive KeyEvents and convert them to
> mozChromeEvent sent to the System App.

Is this the conclusion? Any objections? Can we resolve bug 767311 first and then deal with shell.js changes on this bug later? Just to prevent the patch being bitrotted. Thanks.
We should implement what vivien suggested in comment 28 for now. The necessary Gaia change should be addressed in https://github.com/mozilla-b2g/gaia/issues/1663
Summary: Don't use pageup/pagedown for volume keys in gonk → Block hardware key events to content in shell.js, send mozChromeEvent instead
Should shell.js capture and stop propagation of all key events?  Or only hardware key events? That is: when running in b2g desktop, do we want to allow users to use their real keyboard, or force them to have the same virtual keyboard experience they'd have on a real device?
I'm assigned to Gaia issue #1663, so I'm grabbing this one as well.
Assignee: nobody → dflanagan
(In reply to David Flanagan [:djf] from comment #31)
> Should shell.js capture and stop propagation of all key events?  Or only
> hardware key events? That is: when running in b2g desktop, do we want to
> allow users to use their real keyboard, or force them to have the same
> virtual keyboard experience they'd have on a real device?

I suspect capturing all the key events will break mozKeyboard and vkb actually.

Let's only capture the key events for the actual hardware keys from Gonk, i.e. sleep/vol up/vol down/menu/search/home?
(In reply to David Flanagan [:djf] from comment #32)
> I'm assigned to Gaia issue #1663, so I'm grabbing this one as well.

I think you want to not use Key Events when a HW key is hit. This way you would not be confused by the key generated from the content and the one from the user action on the device.

Something similar to what has been done in http://hg.mozilla.org/mozilla-central/diff/94b5440efa60/widget/src/gonk/nsAppShell.cpp and reverted since will generate appcommand (you can then listen them similarly to http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1013)
(In reply to Vivien Nicolas (:vingtetun) from comment #34)

> Something similar to what has been done in
> http://hg.mozilla.org/mozilla-central/diff/94b5440efa60/widget/src/gonk/
> nsAppShell.cpp and reverted since will generate appcommand (you can then
> listen them similarly to
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#1013)

nsAppShell.cpp has changed substantially since that patch was made.  Key events are no longer being generated there, so I'm going the simpler route.  I'm going to let gecko continue to generate key events for hardware buttons, but I'll filter them out in shell.js and send mozChromeEvents to notify Gaia.
I'll issue a pull request for the corresponding Gaia patch.  We must be careful to land them both at the same time
Attachment #642146 - Flags: review?(21)
The matching Gaia pull request is https://github.com/mozilla-b2g/gaia/pull/2423
Attached patch new version that applies cleanly (obsolete) — — Splinter Review 
I've attached a new version of the patch that applies cleanly.
Attachment #642146 - Attachment is obsolete: true
Attachment #642146 - Flags: review?(21)
Attachment #642491 - Flags: review?(21)
Comment on attachment 642491 [details] [diff] [review]
new version that applies cleanly

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

r+ if you use a addSystemEventListener instead.

::: b2g/chrome/content/shell.js
@@ +165,5 @@
>  
>    stop: function shell_stop() {
> +    window.removeEventListener('keydown', this, true);
> +    window.removeEventListener('keypress', this, true);
> +    window.removeEventListener('keyup', this, true);

While we are here, let's use a systemEventListener to make really sure the event is taken before the content can get it.

See http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChild.js#132

@@ +207,5 @@
>  
> +    // If we didn't return, then the key event represents a hardware key
> +    // and we need to prevent it from propagating to Gaia
> +    evt.stopImmediatePropagation();
> +    evt.preventDefault(); // This may prevent keypress events

Why 'it may'?

@@ +312,5 @@
>      event.initCustomEvent(type, true, true, details ? details : {});
>      content.dispatchEvent(event);
> +  },
> +  sendChromeEvent: function shell_sendChromeEvent(details) {
> +    this.sendEvent(shell.contentBrowser.contentWindow,

use getContentWindow() instead of shell.contentBrowser.contentWindow
Attachment #642491 - Flags: review?(21) → review+
(In reply to Vivien Nicolas (:vingtetun) from comment #39)
 
> >    stop: function shell_stop() {
> > +    window.removeEventListener('keydown', this, true);
> > +    window.removeEventListener('keypress', this, true);
> > +    window.removeEventListener('keyup', this, true);
> 
> While we are here, let's use a systemEventListener to make really sure the
> event is taken before the content can get it.
> 
I have no idea what this code is doing, but event listeners in system event group are
called *after* calling listeners in default group. It is just that stopPropagation() doesn't prevent
listeners in system group to be called.
Per IRC smaug explained me system-group versus default-group and I was wrong. So ignore my comment about addSystemEventListener.
I've attached a revised patch that addresses Vivien's comments.

Vivien already gave an r+, so this is ready for checkin.

I'm not going to set checkin-needed myself, though, since this can't land without https://github.com/mozilla-b2g/gaia/pull/2423
Attachment #642491 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5e2318fc6d1a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Regressions: 1762703
No longer regressions: 1762703
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: