Update LabelButtons in the Toolbar for Material Design

Update LabelButtons in the Toolbar for Material Design

Update PlatformStyle to return different borders for Material Design
LabelButtons. Removing asset borders on Chrome OS, and hover effects on Windows
and Linux.

Updated ToolbarButton to remove its local implementation of this behaviour.

Updated WrenchToolbarButton to use a image asset for Material Design, and to not
initialize WrenchIconPainter.

TEST=manual testing on device
BUG=505801, 495654
Committed: https://crrev.com/b84603444e43433e355b7d1ab30c71c7abb4b95d
Cr-Commit-Position: refs/heads/master@{#342414}

[jonross, 2015-08-07 15:19:03]

jonross@chromium.org changed reviewers:
+ pkasting@chromium.org

[jonross, 2015-08-07 15:19:04]

Hi Peter,

Could you please review the changes under: 

chrome/browser/ui/views/toolbar/*

Thanks!

[jonross, 2015-08-07 15:20:06]

jonross@chromium.org changed reviewers:
+ sadrul@chromium.org

[jonross, 2015-08-07 15:20:07]

Hey Sadrul,

Could you provide an owners review to the changes under

ui/views/style/*

Thanks!

[Peter Kasting, 2015-08-07 15:36:16]

Code LGTM, but I'm concerned about this behavior.

* If we remove all the signalling ability from the Chrome menu button how are we
planning to signal the user about upgrades needed and the like?

* Removing all hover effects from toolbar buttons is problematic.  First, since
the buttons don't have borders, it's a usability loss (for both mouse and touch
users), because you can't tell what the target area is for the button.  This is
a UX design problem that's been cropping up a lot elsewhere and I don't want it
extended to Chrome desktop.  Second, we found when initially designing Chrome
that things like these hover effects were important to emphasize that the
browser was still responsive and functional during times when page content might
look unresponsive.  When we lacked subtle hover animations and the like the
browser felt more unreliable to users.  What's the plan here to avoid these
issues?  One option would be to do a subtle color shift of the entire button
background area on hover.

https://codereview.chromium.org/1270343004/diff/1/ui/views/style/platform_sty...
File ui/views/style/platform_style.cc (right):

https://codereview.chromium.org/1270343004/diff/1/ui/views/style/platform_sty...
ui/views/style/platform_style.cc:20: style == Button::STYLE_TEXTBUTTON) {
Nit: Reverse this condition so you can do the simple return first and eliminate
{}.

Then move the comment above below the conditional.

https://codereview.chromium.org/1270343004/diff/1/ui/views/style/platform_sty...
ui/views/style/platform_style.cc:36: if
(ui::MaterialDesignController::IsModeMaterial()) {
No {}

[jonross, 2015-08-07 15:45:28]

* On Chrome OS the menu is not used for signaling. For desktop Chrome we are
going to look at coloration of the changed menu asset. Either via masking color
over the asset, or via the programmatic asset rendering work being done. The
animation in WrenchIconPainter will be removed, as the dots are too small to
make the fill animation worth it.

* Current plans do not address the hover concerns you are raising. The
subsequent mouse/touch interactions with the buttons will present visual
feedback, including demonstrating the target areas. However this does require a
user to commit an action. sgabriel@ is the UX lead on this, I can bring up these
concerns during our weekly sync.

[Peter Kasting, 2015-08-07 15:57:05]

On 2015/08/07 15:45:28, jonross wrote:
> * On Chrome OS the menu is not used for signaling. For desktop Chrome we are
> going to look at coloration of the changed menu asset. Either via masking
color
> over the asset, or via the programmatic asset rendering work being done. The
> animation in WrenchIconPainter will be removed, as the dots are too small to
> make the fill animation worth it.

OK.

> * Current plans do not address the hover concerns you are raising. The
> subsequent mouse/touch interactions with the buttons will present visual
> feedback, including demonstrating the target areas. However this does require
a
> user to commit an action. sgabriel@ is the UX lead on this, I can bring up
these
> concerns during our weekly sync.

Please do, I'd appreciate it.  (Having some way of telling when your pointer has
moved into a target area, and thus a click will do something, is surprisingly
reassuring and useful as a user, and I'm not the only one that's bemoaned the
increasing loss of this feedback in current UI design trends in the name of
cleanliness.)

[jonross, 2015-08-07 15:57:15]

https://codereview.chromium.org/1270343004/diff/1/ui/views/style/platform_sty...
File ui/views/style/platform_style.cc (right):

https://codereview.chromium.org/1270343004/diff/1/ui/views/style/platform_sty...
ui/views/style/platform_style.cc:20: style == Button::STYLE_TEXTBUTTON) {
On 2015/08/07 15:36:16, Peter Kasting wrote:
> Nit: Reverse this condition so you can do the simple return first and
eliminate
> {}.
> 
> Then move the comment above below the conditional.

Done.

https://codereview.chromium.org/1270343004/diff/1/ui/views/style/platform_sty...
ui/views/style/platform_style.cc:36: if
(ui::MaterialDesignController::IsModeMaterial()) {
On 2015/08/07 15:36:16, Peter Kasting wrote:
> No {}

Done.

[bruthig, 2015-08-07 16:43:14]

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

[bruthig, 2015-08-07 16:43:15]

https://codereview.chromium.org/1270343004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_toolbar_button.cc (right):

https://codereview.chromium.org/1270343004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_toolbar_button.cc:44: if
(!wrench_icon_painter_)
Consider using "if (ui::MaterialDesignController::IsModeMaterial())" instead. 
That makes it a lot more clear to the next person to read/update the code AND it
ensures that the two logical things don't drift at all.  Same for below.

[sadrul, 2015-08-07 16:45:30]

lgtm

On 2015/08/07 15:57:05, Peter Kasting wrote:
> On 2015/08/07 15:45:28, jonross wrote:
> > * On Chrome OS the menu is not used for signaling. For desktop Chrome we are
> > going to look at coloration of the changed menu asset. Either via masking
> color
> > over the asset, or via the programmatic asset rendering work being done. The
> > animation in WrenchIconPainter will be removed, as the dots are too small to
> > make the fill animation worth it.
> 
> OK.
> 
> > * Current plans do not address the hover concerns you are raising. The
> > subsequent mouse/touch interactions with the buttons will present visual
> > feedback, including demonstrating the target areas. However this does
require
> a
> > user to commit an action. sgabriel@ is the UX lead on this, I can bring up
> these
> > concerns during our weekly sync.
> 
> Please do, I'd appreciate it.  (Having some way of telling when your pointer
has
> moved into a target area, and thus a click will do something, is surprisingly
> reassuring and useful as a user, and I'm not the only one that's bemoaned the
> increasing loss of this feedback in current UI design trends in the name of
> cleanliness.)

+1

jonross@ Would you mind filing a blocking bug for this to make sure it doesn't
get lost?

[jonross, 2015-08-07 17:33:47]

I have filed crbug.com/517923 to discuss the hover feedback concerns raised
here. It blocks the crbug.com/495654 for the ToolbarView work.

https://codereview.chromium.org/1270343004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_toolbar_button.cc (right):

https://codereview.chromium.org/1270343004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_toolbar_button.cc:44: if
(!wrench_icon_painter_)
On 2015/08/07 16:43:15, bruthig wrote:
> Consider using "if (ui::MaterialDesignController::IsModeMaterial())" instead. 
> That makes it a lot more clear to the next person to read/update the code AND
it
> ensures that the two logical things don't drift at all.  Same for below.

Done.

[jonross, 2015-08-07 17:33:56]

The CQ bit was checked by jonross@chromium.org

[jonross, 2015-08-07 17:33:57]

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

[commit-bot: I haz the power, 2015-08-07 17:34:13]

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

[bruthig, 2015-08-07 17:36:45]

lgtm

[bruthig, 2015-08-07 17:36:47]

lgtm

[commit-bot: I haz the power, 2015-08-07 18:55:01]

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

[commit-bot: I haz the power, 2015-08-07 18:55:02]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jonross, 2015-08-07 19:06:18]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2015-08-07 19:07:25]

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

[commit-bot: I haz the power, 2015-08-07 19:44:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-07 19:44:55]

Patchset 3 (id:??) landed as
https://crrev.com/b84603444e43433e355b7d1ab30c71c7abb4b95d
Cr-Commit-Position: refs/heads/master@{#342414}