Make the extra padding around VectorIconButtons configurable.

Make the extra padding around VectorIconButtons configurable.

Pre-Harmony, these always assume 4 px of padding on every side.  They also
assume the default ripple size will work (it is normally 24 DIP, which lines up
with a 16 DIP icon + 2 * 4 DIP padding).

In Harmony, we want to change the size of the close buttons to 16 px, with no
padding.  This change makes the padding amount configurable through the various
delegates, and changes the ripple to be sized to the icon button size itself.

I think there is no functional change pre-Harmony, because I think
VectorIconButton is always used for 16 DIP vectors, but I didn't audit.

BUG=686962
TEST=Launch Chrome with --secondary-ui-md, open the Page Info dialog, and hover the close button.  The hover area should be 16 DIP.

[Peter Kasting, 2017-02-01 04:47:22]

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

[commit-bot: I haz the power, 2017-02-01 04:47:38]

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

[Peter Kasting, 2017-02-01 04:48:12]

pkasting@chromium.org changed reviewers:
+ ellyjones@chromium.org, estade@chromium.org, sky@chromium.org

[Peter Kasting, 2017-02-01 04:48:13]

estade: vector_icon_button.*
ellyjones: *layout_delegate*
sky: rest

[commit-bot: I haz the power, 2017-02-01 05:35:11]

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

[commit-bot: I haz the power, 2017-02-01 05:35:11]

Dry run: This issue passed the CQ dry run.

[Elly Fong-Jones, 2017-02-01 16:06:02]

c/b/ui/views/harmony/* lgtm

These touch targets are too small for touch. We should make sure that issue
doesn't fall through the cracks & if there's a filed bug it should be P1.

[sky, 2017-02-01 16:57:29]

https://codereview.chromium.org/2671443002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/vector_icon_button.cc (right):

https://codereview.chromium.org/2671443002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/vector_icon_button.cc:65: const gfx::Insets insets =
ViewsDelegate::GetInstance()->GetButtonPadding();
Is the plan to use GetButtonPadding in all buttons? If not, I think the name
should be more specific. Perhaps VectorIconButtonPadding.

[Evan Stade, 2017-02-01 17:09:51]

I have the same questions that you pose on the bug, as well as: do we want no
the same size even in touch mode?

If we only want to change the dialog close buttons then it seems like all we
need/want is to assign an empty border to the close button in BubbleFrameView
(as well as the changes to the ripple size possibly).

[Peter Kasting, 2017-02-01 19:48:38]

On 2017/02/01 17:09:51, Evan Stade wrote:
> I have the same questions that you pose on the bug, as well as: do we want no
> the same size even in touch mode?

Yeah, that worries me too.  Raised to P1 and posted that explicitly on the bug.

> If we only want to change the dialog close buttons then it seems like all we
> need/want is to assign an empty border to the close button in BubbleFrameView
> (as well as the changes to the ripple size possibly).

I don't think we want to change only the dialog close buttons.

Maybe the real fix here is that VectorIconButton should never add padding, and
its consumers should add padding if they need to?  I'm worried about the
knowledge of this sort of padding living in such a low-level place; it seems
like the kind of thing that, conceptually, the View containing the button would
know.

We use this in:
Find Bar
Infobars
Download shelf
Payments-related UI

Still thinking... maybe we should remove the concept of VectorIconButton and add
vector support directly to ImageButton?  I dunno if that makes this easier or
harder to solve, but at least then from a concept perspective, this is Just
Another Image Button, and if you want a specific size you set it.

[Peter Kasting, 2017-02-01 19:48:45]

https://codereview.chromium.org/2671443002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/vector_icon_button.cc (right):

https://codereview.chromium.org/2671443002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/vector_icon_button.cc:65: const gfx::Insets insets =
ViewsDelegate::GetInstance()->GetButtonPadding();
On 2017/02/01 16:57:28, sky wrote:
> Is the plan to use GetButtonPadding in all buttons? If not, I think the name
> should be more specific. Perhaps VectorIconButtonPadding.

I think that concern is fair.  I hate being so specific with a constant that
lives in such a global place, but the real issue there is the constant needing
to live there to begin with.

If I keep this constant, I will change the name.  It can always be renamed later
if we generalize.

[sky, 2017-02-01 21:58:12]

On 2017/02/01 19:48:38, Peter Kasting wrote:
> On 2017/02/01 17:09:51, Evan Stade wrote:
> > I have the same questions that you pose on the bug, as well as: do we want
no
> > the same size even in touch mode?
> 
> Yeah, that worries me too.  Raised to P1 and posted that explicitly on the
bug.
> 
> > If we only want to change the dialog close buttons then it seems like all we
> > need/want is to assign an empty border to the close button in
BubbleFrameView
> > (as well as the changes to the ripple size possibly).
> 
> I don't think we want to change only the dialog close buttons.
> 
> Maybe the real fix here is that VectorIconButton should never add padding, and
> its consumers should add padding if they need to?  I'm worried about the
> knowledge of this sort of padding living in such a low-level place; it seems
> like the kind of thing that, conceptually, the View containing the button
would
> know.
> 
> We use this in:
> Find Bar
> Infobars
> Download shelf
> Payments-related UI
> 
> Still thinking... maybe we should remove the concept of VectorIconButton and
add
> vector support directly to ImageButton?  I dunno if that makes this easier or
> harder to solve, but at least then from a concept perspective, this is Just
> Another Image Button, and if you want a specific size you set it.

I like one button class that works with both types, in fact in general I like
way *less* button classes then we have now.

If you can push setting the border to actual consumers of this class (find-bar,
infobars...), that SGTM.

[Evan Stade, 2017-02-01 22:07:47]

On 2017/02/01 21:58:12, sky wrote:
> On 2017/02/01 19:48:38, Peter Kasting wrote:
> > On 2017/02/01 17:09:51, Evan Stade wrote:
> > > I have the same questions that you pose on the bug, as well as: do we want
> no
> > > the same size even in touch mode?
> > 
> > Yeah, that worries me too.  Raised to P1 and posted that explicitly on the
> bug.
> > 
> > > If we only want to change the dialog close buttons then it seems like all
we
> > > need/want is to assign an empty border to the close button in
> BubbleFrameView
> > > (as well as the changes to the ripple size possibly).
> > 
> > I don't think we want to change only the dialog close buttons.
> > 
> > Maybe the real fix here is that VectorIconButton should never add padding,
and
> > its consumers should add padding if they need to?  I'm worried about the
> > knowledge of this sort of padding living in such a low-level place; it seems
> > like the kind of thing that, conceptually, the View containing the button
> would
> > know.
> > 
> > We use this in:
> > Find Bar
> > Infobars
> > Download shelf
> > Payments-related UI
> > 
> > Still thinking... maybe we should remove the concept of VectorIconButton and
> add
> > vector support directly to ImageButton?  I dunno if that makes this easier
or
> > harder to solve, but at least then from a concept perspective, this is Just
> > Another Image Button, and if you want a specific size you set it.
> 
> I like one button class that works with both types, in fact in general I like
> way *less* button classes then we have now.
> 
> If you can push setting the border to actual consumers of this class
(find-bar,
> infobars...), that SGTM.

I think we should determine a good default border rather than assuming the
default border should be none. I'd be worried that new consumers would forget
subtle details like setting a particular border. If 9/10 times we want a 4px
border and 1/10 times we want no border, it seems better to make the odd one out
worry about special configuration.

[Peter Kasting, 2017-02-01 22:27:52]

On 2017/02/01 22:07:47, Evan Stade wrote:
> On 2017/02/01 21:58:12, sky wrote:
> > On 2017/02/01 19:48:38, Peter Kasting wrote:
> > > On 2017/02/01 17:09:51, Evan Stade wrote:
> > > > I have the same questions that you pose on the bug, as well as: do we
want
> > no
> > > > the same size even in touch mode?
> > > 
> > > Yeah, that worries me too.  Raised to P1 and posted that explicitly on the
> > bug.
> > > 
> > > > If we only want to change the dialog close buttons then it seems like
all
> we
> > > > need/want is to assign an empty border to the close button in
> > BubbleFrameView
> > > > (as well as the changes to the ripple size possibly).
> > > 
> > > I don't think we want to change only the dialog close buttons.
> > > 
> > > Maybe the real fix here is that VectorIconButton should never add padding,
> and
> > > its consumers should add padding if they need to?  I'm worried about the
> > > knowledge of this sort of padding living in such a low-level place; it
seems
> > > like the kind of thing that, conceptually, the View containing the button
> > would
> > > know.
> > > 
> > > We use this in:
> > > Find Bar
> > > Infobars
> > > Download shelf
> > > Payments-related UI
> > > 
> > > Still thinking... maybe we should remove the concept of VectorIconButton
and
> > add
> > > vector support directly to ImageButton?  I dunno if that makes this easier
> or
> > > harder to solve, but at least then from a concept perspective, this is
Just
> > > Another Image Button, and if you want a specific size you set it.
> > 
> > I like one button class that works with both types, in fact in general I
like
> > way *less* button classes then we have now.
> > 
> > If you can push setting the border to actual consumers of this class
> (find-bar,
> > infobars...), that SGTM.
> 
> I think we should determine a good default border rather than assuming the
> default border should be none. I'd be worried that new consumers would forget
> subtle details like setting a particular border. If 9/10 times we want a 4px
> border and 1/10 times we want no border, it seems better to make the odd one
out
> worry about special configuration.

I'm pretty sure in Harmony the default case is no border, but we do need to
worry about how touch should affect things -- that said, that's once again
likely going to be based on the specific context of either the button size
(small buttons need a bigger change for touch) or the hosting control.

If VectorIconButton somehow got collapsed into ImageButton, then probably the
default case really would be no border even pre-Harmony.  It feels like that's
where things should go long-term anyway (people should have to care less about
whether they're consuming bitmap or vector resources), but I don't know how
feasible it is to fix that right now.

I certainly agree that we should do "the right thing by default" where possible
and have fewer one-offs.  I'm hoping that Harmony helps here from both a design
and an implementation perspective.  I am not sure how to balance the various
different tradeoffs, e.g. the tradeoff between "solve everything correctly at
the start" and "move incrementally to a better world".

Evan, you don't really have engineering time for this, correct?  I'd love to
task you with "unify VectorIconButton with ImageButton and determine the right
future-proof way to do borders and things", but I suspect you are not available
to own it.  If not, maybe I will spin Bret up to speed on this stuff.

[Peter Kasting, 2017-03-03 02:33:28]

I've uploaded a new snap here so it is easier to try this patch locally, mostly
for Bret's benefit.

However, the Harmony spec has now changed, and close buttons have a 24 px hover
size, rather than 16 px.  So this CL is not strictly necessary at the moment.

I'm going to close this for now.