Use ink drop hover for focus states on toolbar buttons and location

Use ink drop hover for focus states on buttons. Remove the dashed border.

Only button-style buttons (i.e. MdTextButton) retain the blue ring for focus, as per sgabriel's orders.

BUG=591140
Committed: https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3
Cr-Commit-Position: refs/heads/master@{#396328}

[Evan Stade, 2016-05-20 21:59:35]

estade@chromium.org changed reviewers:
+ bruthig@chromium.org

[Evan Stade, 2016-05-20 21:59:40]

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

[commit-bot: I haz the power, 2016-05-20 22:00:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001843002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001843002/20001

[commit-bot: I haz the power, 2016-05-20 22:15:26]

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

[commit-bot: I haz the power, 2016-05-20 22:15:27]

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

[Evan Stade, 2016-05-24 01:32:00]

(you can hold off on reviewing, I have a different version coming up soon)

[Evan Stade, 2016-05-24 01:52:34]

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

[commit-bot: I haz the power, 2016-05-24 01:52:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001843002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001843002/40001

[commit-bot: I haz the power, 2016-05-24 02:09:53]

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

[commit-bot: I haz the power, 2016-05-24 02:09:54]

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

[Evan Stade, 2016-05-24 18:44:06]

Description was changed from

==========
Use ink drop ripple for focus states on toolbar buttons and location
icons. Remove the dashed border.

Other buttons (like the app menu button) need it also, those are TODO.

BUG=591140
==========

to

==========
Use ink drop hover for focus states on buttons. Remove the dashed border.

Only button-style buttons (i.e. MdTextButton) retain the blue ring for focus, as
per sgabriel's orders.

BUG=591140
==========

[Evan Stade, 2016-05-24 18:44:12]

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

[commit-bot: I haz the power, 2016-05-24 18:44:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001843002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001843002/60001

[Evan Stade, 2016-05-24 18:47:18]

ptal

https://codereview.chromium.org/2001843002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2001843002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:113: void InkDropImpl::SnapToHovered(bool
is_hovered) {
I specifically avoided turning |is_hovered_| into a counter, with the outcome
that if you mouse over a button while it's focused, then unhover, the focus
shadow goes away. I actually believe this makes more sense because the focus
effect is supposed to be for people using the keyboard. Hence,
request_focus_on_press_ for buttons is almost always false. If you use the
keyboard, then switch to the mouse, it makes sense to stop showing the hover
effect. (Honestly I think this is rare enough that it doesn't matter much one
way or another.)

[commit-bot: I haz the power, 2016-05-24 18:57:30]

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

[commit-bot: I haz the power, 2016-05-24 18:57:32]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Evan Stade, 2016-05-25 17:12:18]

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

[commit-bot: I haz the power, 2016-05-25 17:12:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001843002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001843002/80001

[commit-bot: I haz the power, 2016-05-25 17:52:21]

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

[commit-bot: I haz the power, 2016-05-25 17:52:24]

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

[bruthig, 2016-05-25 18:46:37]

1.) [Discussed offline but summarized here] Can you introduce a Focus concept on
InkDrop instead of overloading the Hover concept.  It might make sense to rename
some of the 'Hover' classes/variables to 'Highlight' if we are adding dual
purpose to the visual.
2.) I think only updating the visual IsFocused/NotFocused state will be super
confusing for users. e.g. a button has logical focus but not visual focus and an
enter/space press triggers the action.   I think it would be better if moving
the mouse out of the bounds actually updates the logical focus state of the View
and then as a side effect the highlight would disappear.  Either way I think
this is a considerable behavioral change that we should probably run by
accessibility.
3.) The InkDropDelegate lifetime was intentionally managed by the leaf Views and
not the CustomButton/InkDropHostView to ensure proper destruction ordering.  I
believe this is what's causing the test failures in your dry-run for patch set
5.  I think there are also potential problems with the leaf Views owning the
lifetime but fixing them are outside the scope of this CL.

[Evan Stade, 2016-05-25 19:21:17]

On 2016/05/25 18:46:37, bruthig wrote:
> 1.) [Discussed offline but summarized here] Can you introduce a Focus concept
on
> InkDrop instead of overloading the Hover concept.  It might make sense to
rename
> some of the 'Hover' classes/variables to 'Highlight' if we are adding dual
> purpose to the visual.

Sure, I can rename some things. I don't know if that classifies as introducing a
new concept.

> 2.) I think only updating the visual IsFocused/NotFocused state will be super
> confusing for users. e.g. a button has logical focus but not visual focus and
an
> enter/space press triggers the action.   I think it would be better if moving
> the mouse out of the bounds actually updates the logical focus state of the
View
> and then as a side effect the highlight would disappear.  Either way I think
> this is a considerable behavioral change that we should probably run by
> accessibility.

I think this is a marginal (not considerable) difference, as users aren't likely
to simultaneously use mouse and keyboard. Pinged folks on the bug.

> 3.) The InkDropDelegate lifetime was intentionally managed by the leaf Views
and
> not the CustomButton/InkDropHostView to ensure proper destruction ordering.  I
> believe this is what's causing the test failures in your dry-run for patch set
> 5.  I think there are also potential problems with the leaf Views owning the
> lifetime but fixing them are outside the scope of this CL.

I'll look into that.

[Evan Stade, 2016-05-25 22:24:08]

OK, I believe I've resolved all 3 points of feedback. There are many more places
where hover could/should be renamed to highlight, but I'd like to make it a TODO
so this CL and its behavioral changes are not buried under a million renames.

[Evan Stade, 2016-05-25 22:24:15]

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

[commit-bot: I haz the power, 2016-05-25 22:24:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001843002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001843002/120001

[commit-bot: I haz the power, 2016-05-25 23:46:24]

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

[commit-bot: I haz the power, 2016-05-25 23:46:26]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 23:46:48]

Dry run: None

[bruthig, 2016-05-26 00:40:52]

Overall looking good.

Can you add some tests to ink_drop_impl_unittest.cc to verify the interplay
between the |is_focused_| and |is_hovered_| flags and the highlight visibility?

So a Keyboard highlight will disappear when the button is activated and fade
back in after the ripple disappears?

I'm excited to see this in action!!

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.cc:48: // No need to do anything when
called during shutdown, and if a derived
Can add the following TODO here?

TODO(bruthig): Fix InkDropImpl to be safer about calling back to potentially
destroyed InkDropHosts and remove |destroying_|.  Consider using a WeakPtr from
InkDropImpl to the InkDropHost.

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:59: bool destroying_;
Is it worth adding a note that |destroying_| is intentionally declared  after
|ink_drop_delegate_| to ensure destruction ordering?

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
File ui/views/animation/ink_drop_impl.h (right):

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_impl.h:101: // Returns true if this ink drop is
showing an effect for hovered or focused.
Can you rephrase this to be clear that this is the logical state and not the
visual state.  e.g. "Returns true if this is logically hovered or focused."

[Evan Stade, 2016-05-26 18:45:01]

test added

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.cc:48: // No need to do anything when
called during shutdown, and if a derived
On 2016/05/26 00:40:52, bruthig wrote:
> Can add the following TODO here?
> 
> TODO(bruthig): Fix InkDropImpl to be safer about calling back to potentially
> destroyed InkDropHosts and remove |destroying_|.  Consider using a WeakPtr
from
> InkDropImpl to the InkDropHost.

Done.

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.h:59: bool destroying_;
On 2016/05/26 00:40:52, bruthig wrote:
> Is it worth adding a note that |destroying_| is intentionally declared  after
> |ink_drop_delegate_| to ensure destruction ordering?

huh? I don't think it matters where destroying_ is declared.

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
File ui/views/animation/ink_drop_impl.h (right):

https://codereview.chromium.org/2001843002/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_impl.h:101: // Returns true if this ink drop is
showing an effect for hovered or focused.
On 2016/05/26 00:40:52, bruthig wrote:
> Can you rephrase this to be clear that this is the logical state and not the
> visual state.  e.g. "Returns true if this is logically hovered or focused."

Done.

[Evan Stade, 2016-05-26 18:45:13]

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

[commit-bot: I haz the power, 2016-05-26 18:45:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001843002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001843002/140001

[commit-bot: I haz the power, 2016-05-26 19:00:32]

Dry run: None

[bruthig, 2016-05-26 19:04:36]

lgtm

https://codereview.chromium.org/2001843002/diff/140001/ui/views/animation/ink...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2001843002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_impl.cc:115: NOTIMPLEMENTED() << " DAS BLUNT";
is this wanted?

[Evan Stade, 2016-05-26 19:06:54]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-05-26 19:06:55]

+sky for ownership review

https://codereview.chromium.org/2001843002/diff/140001/ui/views/animation/ink...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2001843002/diff/140001/ui/views/animation/ink...
ui/views/animation/ink_drop_impl.cc:115: NOTIMPLEMENTED() << " DAS BLUNT";
On 2016/05/26 19:04:36, bruthig wrote:
> is this wanted?

must have been a stray cosmic ray

[sky, 2016-05-26 21:13:01]

https://codereview.chromium.org/2001843002/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/2001843002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:128: void MdTextButton::Layout() {
Don't you need to call LabelButton::Layout ?

https://codereview.chromium.org/2001843002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:186:
set_request_focus_on_press(false);
Do any places using MdTextButton and expecting true need to be updated?

[Evan Stade, 2016-05-26 22:26:30]

https://codereview.chromium.org/2001843002/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/2001843002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:128: void MdTextButton::Layout() {
On 2016/05/26 21:13:01, sky wrote:
> Don't you need to call LabelButton::Layout ?

oops, yes.

https://codereview.chromium.org/2001843002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:186:
set_request_focus_on_press(false);
On 2016/05/26 21:13:01, sky wrote:
> Do any places using MdTextButton and expecting true need to be updated?

I don't think so. This was already being done by virtue of UseMdFocusRing()
calling set_request_focus_on_press(false);

[sky, 2016-05-26 22:45:18]

LGTM

[Evan Stade, 2016-05-26 23:28:24]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-05-26 23:28:25]

The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2001843002/#ps180001
(title: "layout")

[commit-bot: I haz the power, 2016-05-26 23:29:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001843002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001843002/180001

[commit-bot: I haz the power, 2016-05-27 00:30:50]

Description was changed from

==========
Use ink drop hover for focus states on buttons. Remove the dashed border.

Only button-style buttons (i.e. MdTextButton) retain the blue ring for focus, as
per sgabriel's orders.

BUG=591140
==========

to

==========
Use ink drop hover for focus states on buttons. Remove the dashed border.

Only button-style buttons (i.e. MdTextButton) retain the blue ring for focus, as
per sgabriel's orders.

BUG=591140
==========

[commit-bot: I haz the power, 2016-05-27 00:30:52]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-27 00:32:00]

Description was changed from

==========
Use ink drop hover for focus states on buttons. Remove the dashed border.

Only button-style buttons (i.e. MdTextButton) retain the blue ring for focus, as
per sgabriel's orders.

BUG=591140
==========

to

==========
Use ink drop hover for focus states on buttons. Remove the dashed border.

Only button-style buttons (i.e. MdTextButton) retain the blue ring for focus, as
per sgabriel's orders.

BUG=591140

Committed: https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3
Cr-Commit-Position: refs/heads/master@{#396328}
==========

[commit-bot: I haz the power, 2016-05-27 00:32:01]

Patchset 10 (id:??) landed as
https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3
Cr-Commit-Position: refs/heads/master@{#396328}