Send screen on and off state as a11y alert

Send screen on and off state as a11y alert

BUG=646388
BUG=633304
TEST=manual test, test on convertible/tablet device power button pressed/released, keyboard/mouse events when on laptop mode caused screen off and on

Committed: https://crrev.com/5655b1b1746184cf8e99159f7915249404bfd10f
Cr-Commit-Position: refs/heads/master@{#433694}

[Qiang(Joe) Xu, 2016-11-16 20:09:21]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 20:10:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2016-11-16 20:12:26]

Description was changed from

==========
Send a11y alert when there is a switch betwen screen off and screen on

BUG=646388
TEST=manual test
==========

to

==========
Send screen on and off state as a11y alert

BUG=646388
TEST=manual test, tested on (1) tablet power button, (2) increase/decrease
brightness through keyboard, (3) idle screen off, all work well.
==========

[Qiang(Joe) Xu, 2016-11-16 20:12:27]

warx@chromium.org changed reviewers:
+ derat@chromium.org

[Qiang(Joe) Xu, 2016-11-16 21:39:32]

warx@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Qiang(Joe) Xu, 2016-11-16 21:39:33]

derat@ for ash/, and FYI dmazzoni@ for a11y related change

Thanks!

[dmazzoni, 2016-11-16 21:50:06]

lgtm

[Daniel Erat, 2016-11-16 22:08:46]

https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:69: bool new_brightness_is_zero = percent <=
0.001;
is PowerButtonController the best place for this to live, or should it be
somewhere else (e.g. the code that displays the brightness bubble, or
display-configuration code, or ...)?

i ask because the backlight brightness can be changed by many things unrelated
to the power button (the user pressing the brightness keys, the user being
inactive, etc.). this isn't the first place i'd look for code that handles that.

also, does this get triggered when the brightness is set to 0 due to e.g. the
lid being closed? i'm not sure that i'd expect an a11y alert in that case (and
it might actually get cut off due to the system suspending, right?).

[commit-bot: I haz the power, 2016-11-16 23:40:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 23:40:17]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2016-11-17 04:09:20]

Just the reply, need more investigation

https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:69: bool new_brightness_is_zero = percent <=
0.001;
On 2016/11/16 22:08:46, Daniel Erat wrote:
> is PowerButtonController the best place for this to live, or should it be
> somewhere else (e.g. the code that displays the brightness bubble, or
> display-configuration code, or ...)?
> 
> i ask because the backlight brightness can be changed by many things unrelated
> to the power button (the user pressing the brightness keys, the user being
> inactive, etc.). this isn't the first place i'd look for code that handles
that.
> 
> also, does this get triggered when the brightness is set to 0 due to e.g. the
> lid being closed? i'm not sure that i'd expect an a11y alert in that case (and
> it might actually get cut off due to the system suspending, right?).

I did the investigation more on this and found the case was more complex than I
thought.
About code location, I agree we might want to put in TrayBrightness or even
PowerEventObserver.

I missed the test for closelid this morning. It turns out that there is problem
with it. Closelid will announce nothing, but open lid will announce both "screen
off" and "screen on" sequentially. BrightnessChanged is sent first to trigger
a11y alert and then SuspendImminent is sent. I want to have a discussion with
dmazzoni@ to see if we can solve it on a11y side.

Also, we may only alert when BrightnessChanged is not user_initiated, since
chromevox will alert "xxx percent" when user presses brightness down/up keys.
(SetBacklightsForcedOff is not user_initiated for BrightnessChanged, which I
messed up in TabletButtonControllerTest. I will update it later.)

[Daniel Erat, 2016-11-17 17:26:53]

On 2016/11/17 04:09:20, Qiang (Joe) Xu wrote:
> Just the reply, need more investigation
> 
>
https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
> File ash/wm/power_button_controller.cc (right):
> 
>
https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
> ash/wm/power_button_controller.cc:69: bool new_brightness_is_zero = percent <=
> 0.001;
> On 2016/11/16 22:08:46, Daniel Erat wrote:
> > is PowerButtonController the best place for this to live, or should it be
> > somewhere else (e.g. the code that displays the brightness bubble, or
> > display-configuration code, or ...)?
> > 
> > i ask because the backlight brightness can be changed by many things
unrelated
> > to the power button (the user pressing the brightness keys, the user being
> > inactive, etc.). this isn't the first place i'd look for code that handles
> that.
> > 
> > also, does this get triggered when the brightness is set to 0 due to e.g.
the
> > lid being closed? i'm not sure that i'd expect an a11y alert in that case
(and
> > it might actually get cut off due to the system suspending, right?).
> 
> I did the investigation more on this and found the case was more complex than
I
> thought.
> About code location, I agree we might want to put in TrayBrightness or even
> PowerEventObserver.
> 
> I missed the test for closelid this morning. It turns out that there is
problem
> with it. Closelid will announce nothing, but open lid will announce both
"screen
> off" and "screen on" sequentially. BrightnessChanged is sent first to trigger
> a11y alert and then SuspendImminent is sent. I want to have a discussion with
> dmazzoni@ to see if we can solve it on a11y side.
> 
> Also, we may only alert when BrightnessChanged is not user_initiated, since
> chromevox will alert "xxx percent" when user presses brightness down/up keys.
> (SetBacklightsForcedOff is not user_initiated for BrightnessChanged, which I
> messed up in TabletButtonControllerTest. I will update it later.)

makes sense. and just to be clear about it, "user initiated" currently means
"should chrome show the brightness bubble?". it's how we provide visual feedback
when the user presses the brightness keys without also showing the bubble when
the brightness is adjusted due to e.g. ambient light or inactivity.

this is the powerd counterpart to the audio thing i was suggesting in
http://crbug.com/649360 .

i'll comment some more on the bug.

[Daniel Erat, 2016-11-17 17:32:12]

On 2016/11/17 17:26:53, Daniel Erat wrote:
> On 2016/11/17 04:09:20, Qiang (Joe) Xu wrote:
> > Just the reply, need more investigation
> > 
> >
>
https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
> > File ash/wm/power_button_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
> > ash/wm/power_button_controller.cc:69: bool new_brightness_is_zero = percent
<=
> > 0.001;
> > On 2016/11/16 22:08:46, Daniel Erat wrote:
> > > is PowerButtonController the best place for this to live, or should it be
> > > somewhere else (e.g. the code that displays the brightness bubble, or
> > > display-configuration code, or ...)?
> > > 
> > > i ask because the backlight brightness can be changed by many things
> unrelated
> > > to the power button (the user pressing the brightness keys, the user being
> > > inactive, etc.). this isn't the first place i'd look for code that handles
> > that.
> > > 
> > > also, does this get triggered when the brightness is set to 0 due to e.g.
> the
> > > lid being closed? i'm not sure that i'd expect an a11y alert in that case
> (and
> > > it might actually get cut off due to the system suspending, right?).
> > 
> > I did the investigation more on this and found the case was more complex
than
> I
> > thought.
> > About code location, I agree we might want to put in TrayBrightness or even
> > PowerEventObserver.
> > 
> > I missed the test for closelid this morning. It turns out that there is
> problem
> > with it. Closelid will announce nothing, but open lid will announce both
> "screen
> > off" and "screen on" sequentially. BrightnessChanged is sent first to
trigger
> > a11y alert and then SuspendImminent is sent. I want to have a discussion
with
> > dmazzoni@ to see if we can solve it on a11y side.
> > 
> > Also, we may only alert when BrightnessChanged is not user_initiated, since
> > chromevox will alert "xxx percent" when user presses brightness down/up
keys.
> > (SetBacklightsForcedOff is not user_initiated for BrightnessChanged, which I
> > messed up in TabletButtonControllerTest. I will update it later.)
> 
> makes sense. and just to be clear about it, "user initiated" currently means
> "should chrome show the brightness bubble?". it's how we provide visual
feedback
> when the user presses the brightness keys without also showing the bubble when
> the brightness is adjusted due to e.g. ambient light or inactivity.
> 
> this is the powerd counterpart to the audio thing i was suggesting in
> http://crbug.com/649360 .
> 
> i'll comment some more on the bug.

actually, i'll comment here. :-P

should TabletPowerButtonController just announce this when it asks powerd to
force the backlights off or on? that seems like it might be the cleanest
approach. are there any edge cases where that results in us doing the wrong
thing?

[Qiang(Joe) Xu, 2016-11-17 17:51:43]

On 2016/11/17 17:32:12, Daniel Erat wrote:
> On 2016/11/17 17:26:53, Daniel Erat wrote:
> > On 2016/11/17 04:09:20, Qiang (Joe) Xu wrote:
> > > Just the reply, need more investigation
> > > 
> > >
> >
>
https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
> > > File ash/wm/power_button_controller.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2507733004/diff/1/ash/wm/power_button_control...
> > > ash/wm/power_button_controller.cc:69: bool new_brightness_is_zero =
percent
> <=
> > > 0.001;
> > > On 2016/11/16 22:08:46, Daniel Erat wrote:
> > > > is PowerButtonController the best place for this to live, or should it
be
> > > > somewhere else (e.g. the code that displays the brightness bubble, or
> > > > display-configuration code, or ...)?
> > > > 
> > > > i ask because the backlight brightness can be changed by many things
> > unrelated
> > > > to the power button (the user pressing the brightness keys, the user
being
> > > > inactive, etc.). this isn't the first place i'd look for code that
handles
> > > that.
> > > > 
> > > > also, does this get triggered when the brightness is set to 0 due to
e.g.
> > the
> > > > lid being closed? i'm not sure that i'd expect an a11y alert in that
case
> > (and
> > > > it might actually get cut off due to the system suspending, right?).
> > > 
> > > I did the investigation more on this and found the case was more complex
> than
> > I
> > > thought.
> > > About code location, I agree we might want to put in TrayBrightness or
even
> > > PowerEventObserver.
> > > 
> > > I missed the test for closelid this morning. It turns out that there is
> > problem
> > > with it. Closelid will announce nothing, but open lid will announce both
> > "screen
> > > off" and "screen on" sequentially. BrightnessChanged is sent first to
> trigger
> > > a11y alert and then SuspendImminent is sent. I want to have a discussion
> with
> > > dmazzoni@ to see if we can solve it on a11y side.
> > > 
> > > Also, we may only alert when BrightnessChanged is not user_initiated,
since
> > > chromevox will alert "xxx percent" when user presses brightness down/up
> keys.
> > > (SetBacklightsForcedOff is not user_initiated for BrightnessChanged, which
I
> > > messed up in TabletButtonControllerTest. I will update it later.)
> > 
> > makes sense. and just to be clear about it, "user initiated" currently means
> > "should chrome show the brightness bubble?". it's how we provide visual
> feedback
> > when the user presses the brightness keys without also showing the bubble
when
> > the brightness is adjusted due to e.g. ambient light or inactivity.
> > 
> > this is the powerd counterpart to the audio thing i was suggesting in
> > http://crbug.com/649360 .
> > 
> > i'll comment some more on the bug.
> 
> actually, i'll comment here. :-P
> 
> should TabletPowerButtonController just announce this when it asks powerd to
> force the backlights off or on? that seems like it might be the cleanest
> approach. are there any edge cases where that results in us doing the wrong
> thing?

Yes, that would be the easy way. I would expect this user case: device with no
power connected and in locked screen-off state, then press-and-release power
button will announce screen on. But soon the screen will be off without alert,
if no user activity is seen. User may not get noticed that screen is already
off.

I am curious about this kind of user experience. However, the clean way may be
true as I see crbug.com/646388 implies tablet power button behavior to announce
alert right now. Let me comment on crbug.com/646388 to hear more comments (I
will join a volunteer activity and comment this afternoon : ).

[Qiang(Joe) Xu, 2016-11-21 22:03:53]

Hi Daniel, ptal, thanks

I commented on crbug.com/646388 for the new change.

[Daniel Erat, 2016-11-21 22:14:11]

lgtm, but please update the TEST line in the changelist description --
presumably only the tablet power button (or mouse/keyboard activity after
pressing the tablet power button to turn the screen off) triggers this now,
right?

[Qiang(Joe) Xu, 2016-11-21 22:15:07]

On 2016/11/21 22:14:11, Daniel Erat wrote:
> lgtm, but please update the TEST line in the changelist description --
> presumably only the tablet power button (or mouse/keyboard activity after
> pressing the tablet power button to turn the screen off) triggers this now,
> right?

Yes, I forget to update that. Will do.

[Qiang(Joe) Xu, 2016-11-21 22:18:10]

Description was changed from

==========
Send screen on and off state as a11y alert

BUG=646388
TEST=manual test, tested on (1) tablet power button, (2) increase/decrease
brightness through keyboard, (3) idle screen off, all work well.
==========

to

==========
Send screen on and off state as a11y alert

BUG=646388
BUG=633304
TEST=manual test, test on convertible/tablet device power button
pressed/released, keyboard/mouse events when on laptop mode caused screen off
and on
==========

[Qiang(Joe) Xu, 2016-11-21 22:18:28]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2016-11-21 22:18:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2507733004/#ps20001
(title: "alert when SetBacklightsForcedOff")

[commit-bot: I haz the power, 2016-11-21 22:19:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 23:38:05]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479766708383350,
"parent_rev": "b264a20f1a8857d651cc0445f54ab2968308a618", "commit_rev":
"86b9143375ad216d606ae0f7d9e6685bb0da7e21"}

[commit-bot: I haz the power, 2016-11-21 23:38:44]

Description was changed from

==========
Send screen on and off state as a11y alert

BUG=646388
BUG=633304
TEST=manual test, test on convertible/tablet device power button
pressed/released, keyboard/mouse events when on laptop mode caused screen off
and on
==========

to

==========
Send screen on and off state as a11y alert

BUG=646388
BUG=633304
TEST=manual test, test on convertible/tablet device power button
pressed/released, keyboard/mouse events when on laptop mode caused screen off
and on
==========

[commit-bot: I haz the power, 2016-11-21 23:38:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-21 23:44:11]

Description was changed from

==========
Send screen on and off state as a11y alert

BUG=646388
BUG=633304
TEST=manual test, test on convertible/tablet device power button
pressed/released, keyboard/mouse events when on laptop mode caused screen off
and on
==========

to

==========
Send screen on and off state as a11y alert

BUG=646388
BUG=633304
TEST=manual test, test on convertible/tablet device power button
pressed/released, keyboard/mouse events when on laptop mode caused screen off
and on

Committed: https://crrev.com/5655b1b1746184cf8e99159f7915249404bfd10f
Cr-Commit-Position: refs/heads/master@{#433694}
==========

[commit-bot: I haz the power, 2016-11-21 23:44:12]

Patchset 2 (id:??) landed as
https://crrev.com/5655b1b1746184cf8e99159f7915249404bfd10f
Cr-Commit-Position: refs/heads/master@{#433694}