Use correct theme for the off icon of NightLight, fix tooltip and a11y

Use correct theme for the off icon of NightLight, fix tooltip and a11y

1) Make the theme of the off icon of NightLight match that of other
inactive items in the system trays.
2) Make the tooltip text sentence case instead of title case.
3) Add proper a11y spoken feedback support.

Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=726083#c8

BUG=726083, 726089
Review-Url: https://codereview.chromium.org/2907503002
Cr-Commit-Position: refs/heads/master@{#474782}
Committed: https://chromium.googlesource.com/chromium/src/+/2144d0f2e6603b21c50df07a318fc37cf6b6d513

[afakhry, 2017-05-25 00:58:03]

afakhry@chromium.org changed reviewers:
+ jamescook@chromium.org

[afakhry, 2017-05-25 00:58:04]

James, can you please take a look? Thank you!

[James Cook, 2017-05-25 02:08:45]

https://codereview.chromium.org/2907503002/diff/1/ash/system/tiles/tiles_defa...
File ash/system/tiles/tiles_default_view.cc (right):

https://codereview.chromium.org/2907503002/diff/1/ash/system/tiles/tiles_defa...
ash/system/tiles/tiles_default_view.cc:59: gfx::CreateVectorIcon(*icon,
kMenuIconColorDisabled));
I'm finding this hard to follow. How about creating the 3 icons you need
(apparently normal-state enabled, normal-state disabled, disabled-state
disabled) once TilesDefaultView once and providing 1 or 2 utility functions that
returns the one you need?
 
That might avoid the strange "make it in the off state then fix it" trick below.

[afakhry, 2017-05-25 03:11:50]

Description was changed from

==========
Use the correct theme for the off icon of  NightLight

Make the theme of the off icon of NightLight match that of other
inactive items in the system trays.

Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=726083#c7

BUG=726083
==========

to

==========
Use correct theme for the off icon of NightLight, fix tooltip and a11y

1) Make the theme of the off icon of NightLight match that of other
inactive items in the system trays.
2) Make the tooltip text sentence case instead of title case.
3) Add proper a11y spoken feedback support.

Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=726083#c8

BUG=726083
==========

[afakhry, 2017-05-25 03:16:10]

Description was changed from

==========
Use correct theme for the off icon of NightLight, fix tooltip and a11y

1) Make the theme of the off icon of NightLight match that of other
inactive items in the system trays.
2) Make the tooltip text sentence case instead of title case.
3) Add proper a11y spoken feedback support.

Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=726083#c8

BUG=726083
==========

to

==========
Use correct theme for the off icon of NightLight, fix tooltip and a11y

1) Make the theme of the off icon of NightLight match that of other
inactive items in the system trays.
2) Make the tooltip text sentence case instead of title case.
3) Add proper a11y spoken feedback support.

Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=726083#c8

BUG=726083,726089
==========

[afakhry, 2017-05-25 03:17:54]

To save time, I also handled bug 726089 in this same CL, since it also needs to
deal with the same NL button.

PTAL. Thanks.

https://codereview.chromium.org/2907503002/diff/1/ash/system/tiles/tiles_defa...
File ash/system/tiles/tiles_default_view.cc (right):

https://codereview.chromium.org/2907503002/diff/1/ash/system/tiles/tiles_defa...
ash/system/tiles/tiles_default_view.cc:59: gfx::CreateVectorIcon(*icon,
kMenuIconColorDisabled));
On 2017/05/25 02:08:45, James Cook wrote:
> I'm finding this hard to follow. How about creating the 3 icons you need
> (apparently normal-state enabled, normal-state disabled, disabled-state
> disabled) once TilesDefaultView once and providing 1 or 2 utility functions
that
> returns the one you need?
>  
> That might avoid the strange "make it in the off state then fix it" trick
below.

Please take a look at the new code. I created a class for the NightLight button
so that I can change its accessibility settings.

I think we might also consider adding another constructor to SystemMenuButton
that doesn't require a VectorIcon.

[afakhry, 2017-05-25 16:06:38]

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

[afakhry, 2017-05-25 16:06:38]

derat@, James is OOO sick. Can you please take a look at this? Thanks!

[Daniel Erat, 2017-05-25 16:17:34]

lgtm after a comment is addressed

https://codereview.chromium.org/2907503002/diff/20001/ash/system/tiles/tiles_...
File ash/system/tiles/tiles_default_view.cc (right):

https://codereview.chromium.org/2907503002/diff/20001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:65:
NightLightToggleButton(views::ButtonListener* listener)
explicit

[afakhry, 2017-05-25 16:31:38]

https://codereview.chromium.org/2907503002/diff/20001/ash/system/tiles/tiles_...
File ash/system/tiles/tiles_default_view.cc (right):

https://codereview.chromium.org/2907503002/diff/20001/ash/system/tiles/tiles_...
ash/system/tiles/tiles_default_view.cc:65:
NightLightToggleButton(views::ButtonListener* listener)
On 2017/05/25 16:17:34, Daniel Erat wrote:
> explicit

Done.
I remember the presubmit script used to catch this before. It seems it doesn't
now.

[afakhry, 2017-05-25 16:31:52]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-05-25 16:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2907503002/#ps40001
(title: "Explicit")

[commit-bot: I haz the power, 2017-05-25 16:32:13]

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, 2017-05-25 16:56:37]

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

[commit-bot: I haz the power, 2017-05-25 16:56:38]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2017-05-25 17:28:14]

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

[commit-bot: I haz the power, 2017-05-25 17:28:45]

Dry run: CQ is trying da patch.

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

[afakhry, 2017-05-25 17:29:08]

derat@ Sorry! Would you mind taking a second look please? I had to extract the
NightLightToggleButton for tests to see it.

[Daniel Erat, 2017-05-25 17:59:51]

lgtm with a comment nit

https://codereview.chromium.org/2907503002/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_toggle_button.h (right):

https://codereview.chromium.org/2907503002/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_toggle_button.h:13: // The NightLight toggle
button in the system tray.
nit: "Night Light" instead throughout? that's how i've seen it referred to in
bugs and docs, at least.

[afakhry, 2017-05-25 18:03:31]

https://codereview.chromium.org/2907503002/diff/60001/ash/system/night_light/...
File ash/system/night_light/night_light_toggle_button.h (right):

https://codereview.chromium.org/2907503002/diff/60001/ash/system/night_light/...
ash/system/night_light/night_light_toggle_button.h:13: // The NightLight toggle
button in the system tray.
On 2017/05/25 17:59:51, Daniel Erat wrote:
> nit: "Night Light" instead throughout? that's how i've seen it referred to in
> bugs and docs, at least.

I've been consistently calling it NightLight in all comments in ash/. If you
still wish that I change all the comments everywhere, can I do it in a separate
CL?

[commit-bot: I haz the power, 2017-05-25 18:55:42]

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

[commit-bot: I haz the power, 2017-05-25 18:55:43]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2017-05-25 20:19:48]

On 2017/05/25 18:03:31, afakhry wrote:
>
https://codereview.chromium.org/2907503002/diff/60001/ash/system/night_light/...
> File ash/system/night_light/night_light_toggle_button.h (right):
> 
>
https://codereview.chromium.org/2907503002/diff/60001/ash/system/night_light/...
> ash/system/night_light/night_light_toggle_button.h:13: // The NightLight
toggle
> button in the system tray.
> On 2017/05/25 17:59:51, Daniel Erat wrote:
> > nit: "Night Light" instead throughout? that's how i've seen it referred to
in
> > bugs and docs, at least.
> 
> I've been consistently calling it NightLight in all comments in ash/. If you
> still wish that I change all the comments everywhere, can I do it in a
separate
> CL?

sure, that's fine.

and yes, i'd always like us to be more consistent in what we call things since
this kind of confusion inevitably ends up creeping out from c++ into user-facing
strings. :-) go/cros-nightowl consistently uses "Night Light" rather than
"NightLight", and the string that you're updating in this change also has a
space between the two words.

[afakhry, 2017-05-25 20:28:48]

On 2017/05/25 20:19:48, Daniel Erat wrote:
> On 2017/05/25 18:03:31, afakhry wrote:
> >
>
https://codereview.chromium.org/2907503002/diff/60001/ash/system/night_light/...
> > File ash/system/night_light/night_light_toggle_button.h (right):
> > 
> >
>
https://codereview.chromium.org/2907503002/diff/60001/ash/system/night_light/...
> > ash/system/night_light/night_light_toggle_button.h:13: // The NightLight
> toggle
> > button in the system tray.
> > On 2017/05/25 17:59:51, Daniel Erat wrote:
> > > nit: "Night Light" instead throughout? that's how i've seen it referred to
> in
> > > bugs and docs, at least.
> > 
> > I've been consistently calling it NightLight in all comments in ash/. If you
> > still wish that I change all the comments everywhere, can I do it in a
> separate
> > CL?
> 
> sure, that's fine.
> 
> and yes, i'd always like us to be more consistent in what we call things since
> this kind of confusion inevitably ends up creeping out from c++ into
user-facing
> strings. :-) go/cros-nightowl consistently uses "Night Light" rather than
> "NightLight", and the string that you're updating in this change also has a
> space between the two words.

Thanks. Agreed.

[afakhry, 2017-05-25 20:28:55]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-05-25 20:29:47]

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, 2017-05-25 20:34:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495744135070040,
"parent_rev": "7d80f975be42eb50579dc8ddd7e462e27e25cd11", "commit_rev":
"2144d0f2e6603b21c50df07a318fc37cf6b6d513"}

[commit-bot: I haz the power, 2017-05-25 20:34:42]

Description was changed from

==========
Use correct theme for the off icon of NightLight, fix tooltip and a11y

1) Make the theme of the off icon of NightLight match that of other
inactive items in the system trays.
2) Make the tooltip text sentence case instead of title case.
3) Add proper a11y spoken feedback support.

Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=726083#c8

BUG=726083,726089
==========

to

==========
Use correct theme for the off icon of NightLight, fix tooltip and a11y

1) Make the theme of the off icon of NightLight match that of other
inactive items in the system trays.
2) Make the tooltip text sentence case instead of title case.
3) Add proper a11y spoken feedback support.

Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=726083#c8

BUG=726083,726089

Review-Url: https://codereview.chromium.org/2907503002
Cr-Commit-Position: refs/heads/master@{#474782}
Committed:
https://chromium.googlesource.com/chromium/src/+/2144d0f2e6603b21c50df07a318f...
==========

[commit-bot: I haz the power, 2017-05-25 20:34:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2144d0f2e6603b21c50df07a318f...