First stab at MD text buttons

First stab at MD text buttons

These buttons are implemented as a new class separate from LabelButton, which we'll need to retain even when MD is default for certain use cases such as the bookmarks bar.

This new class is used on MD infobars.

This first rough cut doesn't support
a) different button attributes that might affect rendering such as being a Call to Action or default in a dialog. TODO: estade
b) hover or press effects. TODO: bruthig
c) images on the button. I doubt we'll ever want to support images on material buttons, but it's possible we'll decide to add them later.

BUG=520266, 571500
Committed: https://crrev.com/ae2205f5a4c1a0957c88f892776d26c37499b981
Cr-Commit-Position: refs/heads/master@{#367224}

[Evan Stade, 2015-12-21 23:18:47]

Description was changed from

==========
WIP - MD text buttons

BUG=
==========

to

==========
First stab at MD text buttons

These buttons are implemented as a new class separate from LabelButton, which
we'll need to retain even when MD is default for certain use cases such as the
bookmarks bar.

This new class is used on MD infobars. 

This first rough cut doesn't support
a) different button attributes that might affect rendering such as being a Call
to Action. TODO: estade
b) hover or press effects. TODO: bruthig
c) images on the button. I doubt we'll ever want to support images on material
buttons, but it's possible we'll decide to add them later.

BUG=520266,571500
==========

[Evan Stade, 2015-12-21 23:18:47]

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

[Evan Stade, 2015-12-21 23:19:22]

Description was changed from

==========
First stab at MD text buttons

These buttons are implemented as a new class separate from LabelButton, which
we'll need to retain even when MD is default for certain use cases such as the
bookmarks bar.

This new class is used on MD infobars. 

This first rough cut doesn't support
a) different button attributes that might affect rendering such as being a Call
to Action. TODO: estade
b) hover or press effects. TODO: bruthig
c) images on the button. I doubt we'll ever want to support images on material
buttons, but it's possible we'll decide to add them later.

BUG=520266,571500
==========

to

==========
First stab at MD text buttons

These buttons are implemented as a new class separate from LabelButton, which
we'll need to retain even when MD is default for certain use cases such as the
bookmarks bar.

This new class is used on MD infobars. 

This first rough cut doesn't support
a) different button attributes that might affect rendering such as being a Call
to Action or default in a dialog. TODO: estade
b) hover or press effects. TODO: bruthig
c) images on the button. I doubt we'll ever want to support images on material
buttons, but it's possible we'll decide to add them later.

BUG=520266,571500
==========

[bruthig, 2015-12-22 15:56:19]

It's not clear to me why a new MdTextButton needs to be born instead of using
the existing LabelButton?  I'm concerned that we are adding yet-another-button
when the existing LabelButton will do.

https://codereview.chromium.org/1525163004/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/1525163004/diff/40001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:44: UpdateColor();
The material design ripple is supposed to be drawn behind the text.  In order to
do that the text will need to be rendered to it's own layer so that the z-order
of the text and ripple can be controlled.
In other words, we won't be able to draw the text in this OnPaint() method.  The
MdTextButton should have a Label label_ instance instead so that the label_ can
be painted to it's own layer.

[Evan Stade, 2015-12-29 18:14:04]

On 2015/12/22 15:56:19, bruthig wrote:
> It's not clear to me why a new MdTextButton needs to be born instead of using
> the existing LabelButton?  I'm concerned that we are adding yet-another-button
> when the existing LabelButton will do.

I tried to explain this in the CL description but perhaps I didn't do a good
job. So,

1) We need LabelButton and MdTextButton to co-exist. Some things will never
convert to MdTextButton, and other things should be updated on a case-by-case
basis (not all at once).
2) They don't have much in common beyond "has text"; what they do have in common
is mostly provided by the shared super class (CustomButton)
3) LabelButton is complicated and supports things MdTextButton doesn't need.
Trying to make LabelButton more multi-purpose makes it too easy to break
existing LabelButton uses.

https://codereview.chromium.org/1525163004/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/1525163004/diff/40001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:44: UpdateColor();
On 2015/12/22 15:56:19, bruthig wrote:
> The material design ripple is supposed to be drawn behind the text.  In order
to
> do that the text will need to be rendered to it's own layer so that the
z-order
> of the text and ripple can be controlled.
> In other words, we won't be able to draw the text in this OnPaint() method. 
The
> MdTextButton should have a Label label_ instance instead so that the label_
can
> be painted to it's own layer.

If we need another layer, wouldn't we just create a new layer and call
layer()->Add(new_layer) and make this class a LayerDelegate?

Even if the best way to get a new layer is to create a child view, I would not
want to use Label for that. Label has all kinds of special behavior we don't
want and RenderText gives us all the functionality we need for drawing text.

[sky, 2015-12-29 19:20:17]

Beyond effects, what does MdTextButton do that is different than LabelButton? My
naive assumption is we should make LabelButton provide the effects and turn them
on when appropriate.

[Evan Stade, 2015-12-29 19:27:52]

On 2015/12/29 19:20:17, sky wrote:
> Beyond effects, what does MdTextButton do that is different than LabelButton?
My
> naive assumption is we should make LabelButton provide the effects and turn
them
> on when appropriate.

It's more about what it does not do:
- MdTextButton doesn't let you have multiline text or change text colors
- MdTextButton has no support for an image
- MdTextButton doesn't let you set the elide behavior
- Appearance will be different
 -- themed differently
 -- padding is different
 -- text is upper-cased
etc.

When you strip out all the things MdTextButton doesn't need or want, it has
almost nothing in common with LabelButton that isn't already handled by
CustomButton.

[sky, 2015-12-29 19:39:53]

Won't we eventually need this functionality for material design buttons?

On Tue, Dec 29, 2015 at 11:27 AM,  <estade@chromium.org> wrote:
> On 2015/12/29 19:20:17, sky wrote:
>>
>> Beyond effects, what does MdTextButton do that is different than
>> LabelButton?
>
> My
>>
>> naive assumption is we should make LabelButton provide the effects and
>> turn
>
> them
>>
>> on when appropriate.
>
>
> It's more about what it does not do:
> - MdTextButton doesn't let you have multiline text or change text colors
> - MdTextButton has no support for an image
> - MdTextButton doesn't let you set the elide behavior
> - Appearance will be different
>  -- themed differently
>  -- padding is different
>  -- text is upper-cased
> etc.
>
> When you strip out all the things MdTextButton doesn't need or want, it has
> almost nothing in common with LabelButton that isn't already handled by
> CustomButton.
>
> https://codereview.chromium.org/1525163004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2015-12-29 20:05:44]

I think the places that use that functionality will continue to use LabelButton.

For example,
- bookmark bar buttons use icons but aren't set to change to MD buttons.
- SetHorizontalAlignment is only used for buttons with very customized
appearances, like the profile name you hover in the avatar menu. The most common
use actually seems to be setting to ALIGN_CENTER, but that's already the default
so should not be necessary.
- SetElideBehavior is only used in two places, checkboxes and some random button
in the profile chooser view. I don't think checkboxes will want to use
MdTextButton ever and the appearance of the profile chooser view instance is
heavily customized, so not likely to change to MdTextButton.
- SetTextShadows is used in 3 places, one has to do with images, one is for
creating a drag image (doesn't really need to use a LabelButton, certainly
doesn't want to change based on MD design), one is
ash/wm/overview/window_selector_item.cc and I have no idea what that is
- SetStyle won't be necessary because there's just one style

I don't even know if we'll ever need to set the color on an MdTextButton because
thus far I've only seen it used on non-custom-themable surfaces (dialogs,
bubbles, infobars).


On 2015/12/29 19:39:53, sky wrote:
> Won't we eventually need this functionality for material design buttons?
> 
> On Tue, Dec 29, 2015 at 11:27 AM,  <mailto:estade@chromium.org> wrote:
> > On 2015/12/29 19:20:17, sky wrote:
> >>
> >> Beyond effects, what does MdTextButton do that is different than
> >> LabelButton?
> >
> > My
> >>
> >> naive assumption is we should make LabelButton provide the effects and
> >> turn
> >
> > them
> >>
> >> on when appropriate.
> >
> >
> > It's more about what it does not do:
> > - MdTextButton doesn't let you have multiline text or change text colors
> > - MdTextButton has no support for an image
> > - MdTextButton doesn't let you set the elide behavior
> > - Appearance will be different
> >  -- themed differently
> >  -- padding is different
> >  -- text is upper-cased
> > etc.
> >
> > When you strip out all the things MdTextButton doesn't need or want, it has
> > almost nothing in common with LabelButton that isn't already handled by
> > CustomButton.
> >
> > https://codereview.chromium.org/1525163004/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[Evan Stade, 2015-12-29 20:11:19]

On 2015/12/29 20:05:44, Evan Stade wrote:
> I think the places that use that functionality will continue to use
LabelButton.
> 
> For example,
> - bookmark bar buttons use icons but aren't set to change to MD buttons.
> - SetHorizontalAlignment is only used for buttons with very customized
> appearances, like the profile name you hover in the avatar menu. The most
common
> use actually seems to be setting to ALIGN_CENTER, but that's already the
default
> so should not be necessary.

(it's the default for STYLE_BUTTON LabelButtons, at least)

> - SetElideBehavior is only used in two places, checkboxes and some random
button
> in the profile chooser view. I don't think checkboxes will want to use
> MdTextButton ever and the appearance of the profile chooser view instance is
> heavily customized, so not likely to change to MdTextButton.
> - SetTextShadows is used in 3 places, one has to do with images, one is for
> creating a drag image (doesn't really need to use a LabelButton, certainly
> doesn't want to change based on MD design), one is
> ash/wm/overview/window_selector_item.cc and I have no idea what that is
> - SetStyle won't be necessary because there's just one style
> 
> I don't even know if we'll ever need to set the color on an MdTextButton
because
> thus far I've only seen it used on non-custom-themable surfaces (dialogs,
> bubbles, infobars).
> 
> 
> On 2015/12/29 19:39:53, sky wrote:
> > Won't we eventually need this functionality for material design buttons?
> > 
> > On Tue, Dec 29, 2015 at 11:27 AM,  <mailto:estade@chromium.org> wrote:
> > > On 2015/12/29 19:20:17, sky wrote:
> > >>
> > >> Beyond effects, what does MdTextButton do that is different than
> > >> LabelButton?
> > >
> > > My
> > >>
> > >> naive assumption is we should make LabelButton provide the effects and
> > >> turn
> > >
> > > them
> > >>
> > >> on when appropriate.
> > >
> > >
> > > It's more about what it does not do:
> > > - MdTextButton doesn't let you have multiline text or change text colors
> > > - MdTextButton has no support for an image
> > > - MdTextButton doesn't let you set the elide behavior
> > > - Appearance will be different
> > >  -- themed differently
> > >  -- padding is different
> > >  -- text is upper-cased
> > > etc.
> > >
> > > When you strip out all the things MdTextButton doesn't need or want, it
has
> > > almost nothing in common with LabelButton that isn't already handled by
> > > CustomButton.
> > >
> > > https://codereview.chromium.org/1525163004/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >

[sky, 2015-12-29 22:07:52]

Could we still put the md functionality in labelbutton but only enable
the new functionality for the cases you care about? I much prefer a
single class.

  -Scott

On Tue, Dec 29, 2015 at 12:05 PM,  <estade@chromium.org> wrote:
> I think the places that use that functionality will continue to use
> LabelButton.
>
> For example,
> - bookmark bar buttons use icons but aren't set to change to MD buttons.
> - SetHorizontalAlignment is only used for buttons with very customized
> appearances, like the profile name you hover in the avatar menu. The most
> common
> use actually seems to be setting to ALIGN_CENTER, but that's already the
> default
> so should not be necessary.
> - SetElideBehavior is only used in two places, checkboxes and some random
> button
> in the profile chooser view. I don't think checkboxes will want to use
> MdTextButton ever and the appearance of the profile chooser view instance is
> heavily customized, so not likely to change to MdTextButton.
> - SetTextShadows is used in 3 places, one has to do with images, one is for
> creating a drag image (doesn't really need to use a LabelButton, certainly
> doesn't want to change based on MD design), one is
> ash/wm/overview/window_selector_item.cc and I have no idea what that is
> - SetStyle won't be necessary because there's just one style
>
> I don't even know if we'll ever need to set the color on an MdTextButton
> because
> thus far I've only seen it used on non-custom-themable surfaces (dialogs,
> bubbles, infobars).
>
>
> On 2015/12/29 19:39:53, sky wrote:
>>
>> Won't we eventually need this functionality for material design buttons?
>
>
>> On Tue, Dec 29, 2015 at 11:27 AM,  <mailto:estade@chromium.org> wrote:
>> > On 2015/12/29 19:20:17, sky wrote:
>> >>
>> >> Beyond effects, what does MdTextButton do that is different than
>> >> LabelButton?
>> >
>> > My
>> >>
>> >> naive assumption is we should make LabelButton provide the effects and
>> >> turn
>> >
>> > them
>> >>
>> >> on when appropriate.
>> >
>> >
>> > It's more about what it does not do:
>> > - MdTextButton doesn't let you have multiline text or change text colors
>> > - MdTextButton has no support for an image
>> > - MdTextButton doesn't let you set the elide behavior
>> > - Appearance will be different
>> >  -- themed differently
>> >  -- padding is different
>> >  -- text is upper-cased
>> > etc.
>> >
>> > When you strip out all the things MdTextButton doesn't need or want, it
>> > has
>> > almost nothing in common with LabelButton that isn't already handled by
>> > CustomButton.
>> >
>> > https://codereview.chromium.org/1525163004/
>
>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/1525163004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-12-30 01:19:48]

Like bruthig I'm less than thrilled about two similar text related classes. I'm
hoping we can refactor with time though. Please add a comment to LabelButton as
to when you should use MdTextButton instead.

LGTM

[Evan Stade, 2015-12-31 01:33:00]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-12-31 01:33:09]

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

[commit-bot: I haz the power, 2015-12-31 01:34:50]

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

[commit-bot: I haz the power, 2015-12-31 01:34:50]

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

[Evan Stade, 2015-12-31 02:16:08]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-12-31 02:16:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1525163004/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-31 02:16:21]

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

[commit-bot: I haz the power, 2015-12-31 03:19:44]

Description was changed from

==========
First stab at MD text buttons

These buttons are implemented as a new class separate from LabelButton, which
we'll need to retain even when MD is default for certain use cases such as the
bookmarks bar.

This new class is used on MD infobars. 

This first rough cut doesn't support
a) different button attributes that might affect rendering such as being a Call
to Action or default in a dialog. TODO: estade
b) hover or press effects. TODO: bruthig
c) images on the button. I doubt we'll ever want to support images on material
buttons, but it's possible we'll decide to add them later.

BUG=520266,571500
==========

to

==========
First stab at MD text buttons

These buttons are implemented as a new class separate from LabelButton, which
we'll need to retain even when MD is default for certain use cases such as the
bookmarks bar.

This new class is used on MD infobars. 

This first rough cut doesn't support
a) different button attributes that might affect rendering such as being a Call
to Action or default in a dialog. TODO: estade
b) hover or press effects. TODO: bruthig
c) images on the button. I doubt we'll ever want to support images on material
buttons, but it's possible we'll decide to add them later.

BUG=520266,571500
==========

[commit-bot: I haz the power, 2015-12-31 03:19:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-31 03:20:51]

Description was changed from

==========
First stab at MD text buttons

These buttons are implemented as a new class separate from LabelButton, which
we'll need to retain even when MD is default for certain use cases such as the
bookmarks bar.

This new class is used on MD infobars. 

This first rough cut doesn't support
a) different button attributes that might affect rendering such as being a Call
to Action or default in a dialog. TODO: estade
b) hover or press effects. TODO: bruthig
c) images on the button. I doubt we'll ever want to support images on material
buttons, but it's possible we'll decide to add them later.

BUG=520266,571500
==========

to

==========
First stab at MD text buttons

These buttons are implemented as a new class separate from LabelButton, which
we'll need to retain even when MD is default for certain use cases such as the
bookmarks bar.

This new class is used on MD infobars.

This first rough cut doesn't support
a) different button attributes that might affect rendering such as being a Call
to Action or default in a dialog. TODO: estade
b) hover or press effects. TODO: bruthig
c) images on the button. I doubt we'll ever want to support images on material
buttons, but it's possible we'll decide to add them later.

BUG=520266,571500

Committed: https://crrev.com/ae2205f5a4c1a0957c88f892776d26c37499b981
Cr-Commit-Position: refs/heads/master@{#367224}
==========

[commit-bot: I haz the power, 2015-12-31 03:20:52]

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