NO LONGER FOR REVIEW - [ash-md] Implements a square focus rectangle for ToggleButton

[ash-md] Implements a square focus rectangle for ToggleButton

BUG=652492
TEST=manual
  Bring focus using Tab to a toggle button in Network detailed page.
  (focus rectangle appears).
  Press Space (toggle switches)
  Press Enter (toggle switches)
  Tab (focus switches to the next row).

[varkha, 2016-11-17 00:58:45]

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

[commit-bot: I haz the power, 2016-11-17 00:59:38]

Dry run: 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-17 01:05:12]

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

[commit-bot: I haz the power, 2016-11-17 01:05:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[varkha, 2016-11-17 01:11:12]

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

[varkha, 2016-11-17 01:11:32]

varkha@chromium.org changed reviewers:
+ tdanderson@chromium.org

[varkha, 2016-11-17 01:11:33]

tdanderson@, can you please take a look?

[commit-bot: I haz the power, 2016-11-17 01:12:17]

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

[varkha, 2016-11-17 01:51:48]

https://codereview.chromium.org/2509943002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_menu_toggle_button.h (right):

https://codereview.chromium.org/2509943002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_menu_toggle_button.h:25: // crbug.com/614453.
self review - remove the 3 lines above.

[Evan Stade, 2016-11-17 01:59:01]

why have we introduced a wrapper instead of adding focus states to ToggleButton?

[Evan Stade, 2016-11-17 02:13:44]

On 2016/11/17 01:59:01, Evan Stade wrote:
> why have we introduced a wrapper instead of adding focus states to
ToggleButton?

I imagine the answer to this question is that you want the focus rect to be
drawn outside of the toggle's bounds. Yet I think all of those focus rects in
the spec have the same issue (they use some greater bounds than the thing that's
actually focused). Also, I tried this out and it doesn't match the spec, which
shows the focus rect going all the way to the right side, plus it doesn't cover
any other toggle buttons, such as the bluetooth one or the ime onscreen keyboard
one.

I would say that if we absolutely need a focus state for toggle buttons in m56
the best thing to do would be to add something to ToggleButton for now. Since
this patch doesn't match the spec and we don't have time to do so, whatever we
add in ToggleButton should be fine. Going forward I'm not sure what the best
plan would be --- perhaps we need TriView to help us out?

[varkha, 2016-11-17 02:50:59]

I think the best will be best to add ability to size to the right size as in
spec inside ToggleButton, maybe via a setup method that would add additional
border without affecting what's painted.
Still I tried to address the comments here so that the focus rect does not look
too different from other controls in the menu.

[varkha, 2016-11-17 02:51:11]

Patchset #3 (id:40001) has been deleted

[Evan Stade, 2016-11-17 03:27:38]

Can we go with the simplest approach for now, i.e. add something to ToggleButton
and do not introduce SystemMenuToggleButton? We're not matching spec either way.

[tdanderson, 2016-11-17 04:28:30]

On 2016/11/17 03:27:38, Evan Stade wrote:
> Can we go with the simplest approach for now, i.e. add something to
ToggleButton
> and do not introduce SystemMenuToggleButton? We're not matching spec either
way.

I will let the two of you finish this discussion; please re-ping when the CL
is ready for review.

[varkha, 2016-11-17 05:06:09]

On 2016/11/17 03:27:38, Evan Stade wrote:
> Can we go with the simplest approach for now, i.e. add something to
ToggleButton
> and do not introduce SystemMenuToggleButton?

I'll see how far I can get with the changes to just ToggleView. What I will try
to
do is to add an optional call that will manipulate the bounds but have the
default
behavior be such that the ToggleButton will be 68x48 and will have a focus rect
of
that size.

>  We're not matching spec either way.

I think it is important to match the spec at least as far as consistency goes
and
the current patch should be making the focus rects on the toggles to be at least
as matching as the rest of the controls in the system menu (i.e. matching except
for color and thickness - but that is OK for M-56).

[varkha, 2016-11-17 07:16:03]

Posted https://codereview.chromium.org/2506133003 instead.

[varkha, 2016-11-18 01:17:45]

Description was changed from

==========
[ash-md] Implements a square focus rectangle for ToggleButton

BUG=652492
TEST=manual
  Bring focus using Tab to a toggle button in Network detailed page.
  (focus rectangle appears).
  Press Space (toggle switches)
  Press Enter (toggle switches)
  Tab (focus switches to the next row).
==========

to

==========
[ash-md] Implements a square focus rectangle for ToggleButton

BUG=652492
TEST=manual
  Bring focus using Tab to a toggle button in Network detailed page.
  (focus rectangle appears).
  Press Space (toggle switches)
  Press Enter (toggle switches)
  Tab (focus switches to the next row).
==========