Last Comment Bug 722953 - Cause hardware buttons to illuminate when button is pressed
: Cause hardware buttons to illuminate when button is pressed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: ---
Assigned To: Jim Straus
:
Mentors:
Depends on: 712378
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-31 18:18 PST by Jim Straus
Modified: 2012-02-23 15:32 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Enables the button lights, with a timer to turn them off (3.72 KB, patch)
2012-02-01 20:35 PST, Jim Straus
mwu.code: review-
Details | Diff | Splinter Review
Enables the button lights, with a timer to turn them off (13.94 KB, patch)
2012-02-14 17:28 PST, Jim Straus
cjones.bugs: review-
Details | Diff | Splinter Review
Turn on the button backlight along with the screen backlight (480 bytes, patch)
2012-02-14 19:24 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
gal: review+
Details | Diff | Splinter Review

Description Jim Straus 2012-01-31 18:18:51 PST

    
Comment 1 Jim Straus 2012-01-31 18:20:18 PST
Cause the hardware buttons to illuminate when the buttons are pressed.  This will have a timer associated with it to cause the buttons to go off after a period.
Comment 2 Ben Francis [:benfrancis] 2012-02-01 04:22:00 PST
Should these not be illuminated all the time the screen is on? I've seen users not notice there are back & menu buttons at all because they're not visible.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-01 12:04:30 PST
Would be great to get UX feedback on the question in comment 2.  This may not be a problem we need to solve in the near future.
Comment 4 Jim Straus 2012-02-01 20:15:50 PST
That is an option.  Android supports a couple of different times, always on, always off, and based on the light sensor.
Comment 5 Jim Straus 2012-02-01 20:35:48 PST
Created attachment 593718 [details] [diff] [review]
Enables the button lights, with a timer to turn them off

Patches the key dispatcher to enable the button lights for the menu and back buttons.  Turning on the lights triggers a timer to turn them off after a set period.  To make this completely useful, we'll need a javascript function to set the timeout.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-01 21:49:13 PST
Comment on attachment 593718 [details] [diff] [review]
Enables the button lights, with a timer to turn them off

This may be a good reason to create an ffi to let JS poke the lights.  Let's see what mwu thinks.
Comment 7 Andreas Gal :gal 2012-02-01 22:14:39 PST
Watch out for trailing whitespace. I use

[color]
diff.trailingwhitespace = bold red_background

in ~/.hgrc

and then hg diff (or hg qdiff)
Comment 8 Ben Francis [:benfrancis] 2012-02-02 05:23:07 PST
Just FYI regarding comment 2, UX are going for a software back button in every app as a convention so having the hardware button illuminated all the time as I suggested may not be so important. I'm not sure about the menu button though, that wasn't clear.
Comment 9 Michael Wu [:mwu] 2012-02-07 12:34:26 PST
Comment on attachment 593718 [details] [diff] [review]
Enables the button lights, with a timer to turn them off

Sorry about the review delay!

I don't think this sort of policy belongs in the widget backend. Some sort of FFI for JS to control turning lights on/off would be better.
Comment 10 Andreas Gal :gal 2012-02-07 12:45:47 PST
Need some direction here. Chris?
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-07 16:33:02 PST
Jim, let's twiddle the keyboard backlight inside hal::SetScreenBrightness(), in sync with the screen backlight.

Moving this out into content JS is a big can o' worms that I don't want to open right now, without clear use cases.
Comment 12 Jim Straus 2012-02-07 17:12:08 PST
Ok, I cam move the function that actually twiddles the button lights to the hal area (GonkHal should do it).  nsAppShell will have to call the function, since that is where we can tell a button has been pressed.  Do you want the JS interface to the parameter in this patch?

On doing it from JS, SetLight() is defined in the previous patch, which could be used to implement this, but I agree, JS should set the parameter, but letting the underlying engine actually implement it seems more reasonable.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-07 17:14:02 PST
My suggestion is to make the button backlight operate in sync with the screen backlight.  I don't know of any use cases for controlling them separately yet, and operating in sync is the simplest implementation.
Comment 14 Jim Straus 2012-02-07 17:16:47 PST
I was just emulating the functionality that Android provides, but doing it in sync with the screen is easy.
Comment 15 Jim Straus 2012-02-14 17:28:50 PST
Created attachment 597253 [details] [diff] [review]
Enables the button lights, with a timer to turn them off

Restructured to move the button light handling and timer into the Hal area.  Added interface to allow JS to set the timeout.  Note that I consider SetLight to be a lower level interface to the lights and this to be a higher level way to handle the button lights.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-14 19:24:02 PST
Comment on attachment 597253 [details] [diff] [review]
Enables the button lights, with a timer to turn them off

This is the approach that mwu already r-'d.  It doesn't implement the follow-up suggestion in comment 11 and comment 13.  This duplicates what our idle service already does.

There's a very simple implementation of this, which treats the button backlight as part of the screen backlight.  There's also a very generic and flexible implementation which would fully expose the lights to content JS, the Gaia homescreen e.g.  Android makes use of this kind of interface to do things like turn on/off the key backlight when the lock screen is dismissed, and off idle timeouts.  We don't have a strong reason yet to add all this flexibility and doing so would be a fair amount of work, so it's not really worth the time atm.  Here we're somewhere in between, but with some hacks in gonk widgetry that mwu isn't in favor of, and some gonk hal hacks that I'm not in favor of.

I'm going to post a patch for the dead-simple impl.  Let's re-open this discussion if we get feedback from UX about button lighting.
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-14 19:24:36 PST
Created attachment 597280 [details] [diff] [review]
Turn on the button backlight along with the screen backlight
Comment 18 Justin Lebar (not reading bugmail) 2012-02-16 14:44:58 PST
(In reply to Andreas Gal :gal from comment #7)
> Watch out for trailing whitespace. I use
> 
> [color]
> diff.trailingwhitespace = bold red_background
> 
> in ~/.hgrc
> 
> and then hg diff (or hg qdiff)

Git seems to warn about trailing whitespace if you put

  [color]
    ui = true

in your gitconfig.  Also, git rebase --whitespace=fix is great.
Comment 19 Chris Lee [:clee] 2012-02-21 06:24:36 PST
Josh, not needed for demo milestone, but we should look at this for M3.

Cjones is looking for back light behavior of the screen vs. the buttons itself and how they tie together.
Comment 20 Andreas Gal :gal 2012-02-23 15:22:37 PST
Comment on attachment 597280 [details] [diff] [review]
Turn on the button backlight along with the screen backlight

Nice simple fix. This is what we need. Please land.
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-23 15:32:48 PST
https://hg.mozilla.org/mozilla-central/rev/6fe899b9b372

Note You need to log in before you can comment on or make changes to this bug.