Fix regression in dragging location bar right edge (Mac).

Fix regression in dragging location bar right edge (Mac).

This change fixes a regression where it was no longer possible to drag
the right edge of the location bar to resize it.

This fix involves adding an explicit left and right padding for the
items at the start and end of the toolbar actions container. Previously
the ToolbarActionsBar used the item spacing as the padding when
computing the width of the container. With Material Design,
the padding is 0pt, causing the left and right padding to disappear.

BUG=593268

[shrike, 2016-03-10 00:01:11]

shrike@chromium.org changed reviewers:
+ avi@chromium.org, pkasting@google.com

[shrike, 2016-03-10 00:01:11]

PTAL

pkasting@ for
  chrome/browser/ui/layout_constants.cc
  chrome/browser/ui/layout_constants.h
  chrome/browser/ui/toolbar/toolbar_actions_bar.cc
  chrome/browser/ui/toolbar/toolbar_actions_bar.h

avi@ for
  chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm
  chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm

[Peter Kasting, 2016-03-10 00:05:47]

I'm confused.  You say that on Mac these values differ, but I see no evidence of
that in this CL.  Am I missing something?

[shrike, 2016-03-10 00:10:30]

On 2016/03/10 00:05:47, Peter Kasting wrote:
> I'm confused.  You say that on Mac these values differ, but I see no evidence
of
> that in this CL.  Am I missing something?

If you look in layout_constants.cc you'll see that kToolbarStandardSpacing is 0
for the Mac on MD and 4 for everyone else. The code elsewhere uses the spacing
as the padding but I need the padding to be 4 on the Mac on MD, not 0, so I
added an explicit kToolbarActionsLeftPadding and kToolbarActionsRightPadding.

[Peter Kasting, 2016-03-10 00:23:13]

On 2016/03/10 00:10:30, shrike wrote:
> On 2016/03/10 00:05:47, Peter Kasting wrote:
> > I'm confused.  You say that on Mac these values differ, but I see no
evidence
> of
> > that in this CL.  Am I missing something?
> 
> If you look in layout_constants.cc you'll see that kToolbarStandardSpacing is
0
> for the Mac on MD and 4 for everyone else. The code elsewhere uses the spacing
> as the padding but I need the padding to be 4 on the Mac on MD, not 0, so I
> added an explicit kToolbarActionsLeftPadding and kToolbarActionsRightPadding.

Is 0 really correct for the spacing on Mac, especially when the non-MD and
hybrid cases still match non-Mac?  Why does Mac differ -- can the difference be
done in a different way than having the value of this constant differ?

(It seems like this value is only read in cross-platform views code, so it's not
immediately obvious to me why Mac wants the difference.)

If ultimately we wind up with something like what this CL does, I would still
condense to having just one new layout constant, and I would define things more
like:

  const int kToolbarStandardSpacing[] = {3, 4, 8};  // No #ifdefs here

...

  case TOOLBAR_ACTIONS_END_PADDING:
    return kToolbarStandardSpacing[mode];
  case TOOLBAR_ACTIONS_SPACING:
#if defined(OS_MACOSX)
    if (mode == Mode::MATERIAL_NORMAL)
      return 0;
#endif
    return kToolbarStandardSpacing[mode];

[shrike, 2016-03-10 00:31:40]

On 2016/03/10 00:23:13, Peter Kasting wrote:
> Is 0 really correct for the spacing on Mac, 

Yes.

> especially when the non-MD and
> hybrid cases still match non-Mac?

Non-MD is a different design. As for hybrid, there is no hybrid case on the Mac,
so that value is meaningless.

>  Why does Mac differ -- can the difference be
> done in a different way than having the value of this constant differ?
> 
> (It seems like this value is only read in cross-platform views code, so it's
not
> immediately obvious to me why Mac wants the difference.)

The Mac toolbar code uses a ToolbarActionsBar to compute button locations, etc.,
so the Mac needs to have this class contain the correct spacing value.

> If ultimately we wind up with something like what this CL does, I would still
> condense to having just one new layout constant, and I would define things
more
> like:

OK, I will make that change.

[Peter Kasting, 2016-03-10 00:47:56]

On 2016/03/10 00:31:40, shrike wrote:
> >  Why does Mac differ -- can the difference be
> > done in a different way than having the value of this constant differ?
> > 
> > (It seems like this value is only read in cross-platform views code, so it's
> not
> > immediately obvious to me why Mac wants the difference.)
> 
> The Mac toolbar code uses a ToolbarActionsBar to compute button locations,
etc.,
> so the Mac needs to have this class contain the correct spacing value.

I guess what I'm saying is that I don't understand how you're using the layout
that other platforms use, except you're changing the spacing value to zero, and
that winds up with correct results.  I'd like to understand more detail about
precisely what UI Mac ends up with compared to non-Mac and how we do and don't
use cross-platform code getting there.  This is so I can understand whether
instead of mucking with these values we should be doing something else, like
factoring out part of the toolbar code to be better shared or something.

[shrike, 2016-03-10 01:00:27]

On 2016/03/10 00:47:56, Peter Kasting wrote:
> I guess what I'm saying is that I don't understand how you're using the layout
> that other platforms use, except you're changing the spacing value to zero,
and
> that winds up with correct results.  I'd like to understand more detail about
> precisely what UI Mac ends up with compared to non-Mac and how we do and don't
> use cross-platform code getting there.  This is so I can understand whether
> instead of mucking with these values we should be doing something else, like
> factoring out part of the toolbar code to be better shared or something.

OK, first off let me back up and say that I should have realized that your
proposed alternative implementation will not work. The problem isn't just that I
need the button spacing to be 0, it's that I need the code in ToolbarActionsBar
to use a padding of 4 and and spacing of 0. With your change, the spacing is 0
(correct), but the ToolbarActionsBar uses that 0 as the padding. I need a
padding value that's separate from the spacing.

To your question of why the Mac needs a different spacing, the Mac toolbar spec
(SPEC-core-ui-mac-toolbar.png) dictates a 28x28 click target for each button,
and no space between neighboring click targets. Looking at what I believe is the
Windows spec (SPEC-core-ui-toolbar+overflow.png), I think the spacing there is
also 0?

[Peter Kasting, 2016-03-10 01:35:24]

On 2016/03/10 01:00:27, shrike wrote:
> On 2016/03/10 00:47:56, Peter Kasting wrote:
> > I guess what I'm saying is that I don't understand how you're using the
layout
> > that other platforms use, except you're changing the spacing value to zero,
> and
> > that winds up with correct results.  I'd like to understand more detail
about
> > precisely what UI Mac ends up with compared to non-Mac and how we do and
don't
> > use cross-platform code getting there.  This is so I can understand whether
> > instead of mucking with these values we should be doing something else, like
> > factoring out part of the toolbar code to be better shared or something.
> 
> OK, first off let me back up and say that I should have realized that your
> proposed alternative implementation will not work. The problem isn't just that
I
> need the button spacing to be 0, it's that I need the code in
ToolbarActionsBar
> to use a padding of 4 and and spacing of 0. With your change, the spacing is 0
> (correct), but the ToolbarActionsBar uses that 0 as the padding. I need a
> padding value that's separate from the spacing.

I think you misread my proposal.  My proposal does use 4 as the padding.  The
only 0 that gets returned is the Mac case of TOOLBAR_ACTIONS_SPACING.

> To your question of why the Mac needs a different spacing, the Mac toolbar
spec
> (SPEC-core-ui-mac-toolbar.png) dictates a 28x28 click target for each button,
> and no space between neighboring click targets. Looking at what I believe is
the
> Windows spec (SPEC-core-ui-toolbar+overflow.png), I think the spacing there is
> also 0?

I don't know how big your resulting click targets will be, but either the
toolbar layout code includes the spacing in the click target or it doesn't.  If
it doesn't, then Mac and non-Mac would presumably have the same size click
targets, but the Mac code would position them visibly closer together.  If it
does, then Mac and non-Mac would have different-size click targets, both with 0
spacing between them, and they'd still look different.

I don't think Mac and non-Mac should look visibly different.  (As to whether
they should have larger targets with no space between or smaller ones with space
between, I have little opinion.)  So it seems like using a different value here
for Mac versus non-Mac isn't ultimately desirable, and we should make some other
change.

[Avi (use Gerrit), 2016-03-10 04:08:16]

I have no code issues, but come to a resolution with Peter and ping me.

[shrike, 2016-03-10 06:05:10]

On 2016/03/10 01:35:24, Peter Kasting wrote:
> I don't know how big your resulting click targets will be, but either the
> toolbar layout code includes the spacing in the click target or it doesn't.

It does not and it cannot. The click target is the area in which clicking the
mouse activates the button (and over which positioning the mouse activates the
hover state). Any inactive space between the click targets is the spacing. In
terms of the toolbar layout code, the button width is the click target width,
and the spacing is the spacing.

> If it doesn't, then Mac and non-Mac would presumably have the same size click
> targets,

Yes.

> but the Mac code would position them visibly closer together.

No. The Mac and non-Mac/non-hybrid specs seem to say the same thing, which is:

    * Toolbar click targets (aka the clickable bounds of toolbar buttons) are
28x28 points
    * The spacing between the click targets (except for hybrid) is 0 points

This means the current spacing value of 4 in layout_constants.cc is incorrect -
it should be zero for non-Mac/non-hybrid MD as well.

[Peter Kasting, 2016-03-10 06:15:04]

On 2016/03/10 06:05:10, shrike wrote:
> On 2016/03/10 01:35:24, Peter Kasting wrote:
> > I don't know how big your resulting click targets will be, but either the
> > toolbar layout code includes the spacing in the click target or it doesn't.
> 
> It does not and it cannot. The click target is the area in which clicking the
> mouse activates the button (and over which positioning the mouse activates the
> hover state). Any inactive space between the click targets is the spacing. In
> terms of the toolbar layout code, the button width is the click target width,
> and the spacing is the spacing.
> 
> > If it doesn't, then Mac and non-Mac would presumably have the same size
click
> > targets,
> 
> Yes.
> 
> > but the Mac code would position them visibly closer together.
> 
> No. The Mac and non-Mac/non-hybrid specs seem to say the same thing, which is:
> 
>     * Toolbar click targets (aka the clickable bounds of toolbar buttons) are
> 28x28 points
>     * The spacing between the click targets (except for hybrid) is 0 points
> 
> This means the current spacing value of 4 in layout_constants.cc is incorrect
-
> it should be zero for non-Mac/non-hybrid MD as well.

I like to get confirmation on this sort of thing before changing it, just
because specs are basically preliminary suggestions rather than a final
authority, and we like to see stuff in action to confirm; also too many MD
things have had multiple specs saying multiple things.  Also it's hard to
believe we'd want 0 in non-hybrid MD but 8 in hybrid; I'd think if we were
serious about 0, we'd do 0 in both, and just maybe enlarge the click targets in
hybrid.  Mac doesn't have to worry about hybrid, but non-Mac does.

Of course, your primary concern is getting things working on Mac, so it's a bit
of a distraction to be talking about all this.  But I don't think it should take
too much time to come to agreement on how this should work everywhere, so
hopefully it's OK to say: file a bug about this, put up some screenshots of the
alternatives, let's get the opinion of Sebastien and maybe me or another UI
engineer, get Mac and non-Mac onto the same page, and then depending on that
outcome we will or won't need to commit an additional fix along the lines of the
one in this patch (maybe as part of the same change).

Is this plan OK?

[shrike, 2016-03-10 17:08:07]

On 2016/03/10 06:15:04, Peter Kasting wrote:
> On 2016/03/10 06:05:10, shrike wrote:
> > On 2016/03/10 01:35:24, Peter Kasting wrote:
> > > I don't know how big your resulting click targets will be, but either the
> > > toolbar layout code includes the spacing in the click target or it
doesn't.
> > 
> > It does not and it cannot. The click target is the area in which clicking
the
> > mouse activates the button (and over which positioning the mouse activates
the
> > hover state). Any inactive space between the click targets is the spacing.
In
> > terms of the toolbar layout code, the button width is the click target
width,
> > and the spacing is the spacing.
> > 
> > > If it doesn't, then Mac and non-Mac would presumably have the same size
> click
> > > targets,
> > 
> > Yes.
> > 
> > > but the Mac code would position them visibly closer together.
> > 
> > No. The Mac and non-Mac/non-hybrid specs seem to say the same thing, which
is:
> > 
> >     * Toolbar click targets (aka the clickable bounds of toolbar buttons)
are
> > 28x28 points
> >     * The spacing between the click targets (except for hybrid) is 0 points
> > 
> > This means the current spacing value of 4 in layout_constants.cc is
incorrect
> -
> > it should be zero for non-Mac/non-hybrid MD as well.
> 
> I like to get confirmation on this sort of thing before changing it, just
> because specs are basically preliminary suggestions rather than a final
> authority, and we like to see stuff in action to confirm; also too many MD
> things have had multiple specs saying multiple things.  Also it's hard to
> believe we'd want 0 in non-hybrid MD but 8 in hybrid;

As I understand it the hybrid-MD is designed for a touch interface, so the
buttons need to be spaced further apart. You could effectively move the icons
further apart by making the click targets wider, but you would still run the
risk of the user tapping the wrong button if the buttons still abut.

> I'd think if we were
> serious about 0, we'd do 0 in both, and just maybe enlarge the click targets
in
> hybrid.  Mac doesn't have to worry about hybrid, but non-Mac does.
> 
> Of course, your primary concern is getting things working on Mac, so it's a
bit
> of a distraction to be talking about all this.  But I don't think it should
take
> too much time to come to agreement on how this should work everywhere, so
> hopefully it's OK to say: file a bug about this, put up some screenshots of
the
> alternatives, let's get the opinion of Sebastien and maybe me or another UI
> engineer, get Mac and non-Mac onto the same page, and then depending on that
> outcome we will or won't need to commit an additional fix along the lines of
the
> one in this patch (maybe as part of the same change).
> 
> Is this plan OK?

Yes it is, thank you. I will get an e-mail going between us and Sebastien to
make sure we're all on the same page.

[shrike, 2016-03-11 17:26:24]

Our e-mail discussion was not helpful, and really got light years away from
being relevant to this cl. To clarify:

The purpose of this cl is to fix a regression on the Mac under Material Design,
where it is currently not possible to drag the location bar's right edge to
resize it. The resizing is controlled by the ToolbarActionsView on the Mac,
which is a view that holds the extension buttons that appear between the
location bar and the actions button.

The Mac's ToolbarActionsView uses a ToolbarActionsBar to determine the extent of
the view and the locations of its buttons. The ToolbarActionsBar assumes that
the padding at either end of the view is equal to the button spacing. As a
result, when button spacing is 0 (as it is under Material Design), padding is
also 0. The problem with resizing the location bar is a result of 0 padding on
the left edge of the ToolbarActionsView.

To fix, this cl modifies ToolbarActionsBar to create an explicit left and right
padding. Doing so makes the ToolbarActionsBar implementation more correct, as
padding should not be assumed to be equal to the spacing. It does not modify the
UI layout for any platform, except for restoring the padding on either end of
the bar for the Mac under MD.

That is all.

[shrike, 2016-03-11 17:28:02]

Description was changed from

==========
Fix regression in dragging location bar right edge (Mac).

This change fixes a regression where it was no longer possible to drag
the right edge of the location bar to resize it.

This fix involves adding an explicit left and right padding for the
items at the start and end of the toolbar actions container. Previously
the ToolbarActionsBar used the item spacing as the padding when
computing the width of the container. With Material Design on the Mac,
the padding and spacing are different (but remain the same on all
other platforms).

BUG=593268
==========

to

==========
Fix regression in dragging location bar right edge (Mac).

This change fixes a regression where it was no longer possible to drag
the right edge of the location bar to resize it.

This fix involves adding an explicit left and right padding for the
items at the start and end of the toolbar actions container. Previously
the ToolbarActionsBar used the item spacing as the padding when
computing the width of the container. With Material Design,
the padding is 0pt, causing the left and right padding to disappear.

BUG=593268
==========

[Peter Kasting, 2016-03-11 19:20:10]

On 2016/03/11 17:26:24, shrike wrote:
> Our e-mail discussion was not helpful, and really got light years away from
> being relevant to this cl. To clarify:
> 
> The purpose of this cl is to fix a regression on the Mac under Material
Design,
> where it is currently not possible to drag the location bar's right edge to
> resize it. The resizing is controlled by the ToolbarActionsView on the Mac,
> which is a view that holds the extension buttons that appear between the
> location bar and the actions button.
> 
> The Mac's ToolbarActionsView uses a ToolbarActionsBar to determine the extent
of
> the view and the locations of its buttons. The ToolbarActionsBar assumes that
> the padding at either end of the view is equal to the button spacing. As a
> result, when button spacing is 0 (as it is under Material Design), padding is
> also 0. The problem with resizing the location bar is a result of 0 padding on
> the left edge of the ToolbarActionsView.

The reason I'm trying to have the email discussion is that I strongly suspect
that the correct fix is actually to not have the button spacing value be 0. 
Even if we actually want zero space between some of the toolbar buttons that
value _still_ probably should not be zero.  I don't think it's doing what you
think it's doing.  (But much of the reason for that thread is to confirm what,
precisely, it _is_ doing.)

If somehow this is wrong and we really want zero there, then it's still unlikely
this fix is precisely correct as-is.  We probably need to modify the toolbar
layout code a bit.  We may not want a padding value in layout_constants.cc at
all, and if we do, we probably don't want to define it completely separately
from the spacing value, and if we _do_ want that, we probably only want one such
constant, not two.

Figuring out all this is the point of the email thread, which is why I remain
convinced that it is eminently relevant :)

[shrike, 2016-03-11 20:03:14]

On 2016/03/11 19:20:10, Peter Kasting wrote:
> On 2016/03/11 17:26:24, shrike wrote:
> > Our e-mail discussion was not helpful, and really got light years away from
> > being relevant to this cl. To clarify:
> > 
> > The purpose of this cl is to fix a regression on the Mac under Material
> Design,
> > where it is currently not possible to drag the location bar's right edge to
> > resize it. The resizing is controlled by the ToolbarActionsView on the Mac,
> > which is a view that holds the extension buttons that appear between the
> > location bar and the actions button.
> > 
> > The Mac's ToolbarActionsView uses a ToolbarActionsBar to determine the
extent
> of
> > the view and the locations of its buttons. The ToolbarActionsBar assumes
that
> > the padding at either end of the view is equal to the button spacing. As a
> > result, when button spacing is 0 (as it is under Material Design), padding
is
> > also 0. The problem with resizing the location bar is a result of 0 padding
on
> > the left edge of the ToolbarActionsView.
> 
> The reason I'm trying to have the email discussion is that I strongly suspect
> that the correct fix is actually to not have the button spacing value be 0. 
> Even if we actually want zero space between some of the toolbar buttons that
> value _still_ probably should not be zero.  I don't think it's doing what you
> think it's doing.  (But much of the reason for that thread is to confirm what,
> precisely, it _is_ doing.)
> 
> If somehow this is wrong and we really want zero there, then it's still
unlikely
> this fix is precisely correct as-is.  We probably need to modify the toolbar
> layout code a bit.  We may not want a padding value in layout_constants.cc at
> all, and if we do, we probably don't want to define it completely separately
> from the spacing value, and if we _do_ want that, we probably only want one
such
> constant, not two.

I think this cl is fine as is. Even if spacing needed to be 0 across all
platforms, and even if we wanted 0 padding as a result, we would still want to
land this cl so that in the future when the design changes again we can easily
fill in appropriate spacing and padding values and everything will just work.

That said, sgabriel@ says that buttons should be 24x24 on all platforms so that
means spacing can go back to 4pt on the Mac. Therefore this cl is no longer
needed (although toolbar_actions_bar should really be cleaned up to not assume
that spacing == padding). I will need to land a cl to change the Mac spacing in
layout_constants back to 4pt.

Goodbye for now.