Forgot to attach screenshots.

Deleted: mVBToolbar.png 4.7 KB

	mVBToolbar.png 4.7 KB View Download

Deleted: MacNativeToolbar.png 9.2 KB

	MacNativeToolbar.png 9.2 KB View Download

Thanks for all of these! I'll do a big triage of  Issue 671916  blockers when the current phase is complete. See go/macviewstracking

spqchan@: can you take a peek at this please? :)

[Bulk Edit]
Applying M-68 milestone per email discussion with ellyjones@. Pls change it if milestone is incorrectly applied. 

MacViews triage: assigning to spqchan@ for M-68.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b9a46a98f6334cf6701e79acfe239796dc018a2

commit 2b9a46a98f6334cf6701e79acfe239796dc018a2
Author: spqchan <spqchan@chromium.org>
Date: Wed Mar 28 17:41:04 2018

[MacViews] Fix for incorrect extension padding

Move the extensions 2pt to the right on Mac to match the native Mac
placements.

Bug:  792593 
Change-Id: I579520a37043667dfa289b110287c034447c3812
Reviewed-on: https://chromium-review.googlesource.com/982353
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546536}
[modify] https://crrev.com/2b9a46a98f6334cf6701e79acfe239796dc018a2/chrome/browser/ui/layout_constants.cc
[modify] https://crrev.com/2b9a46a98f6334cf6701e79acfe239796dc018a2/chrome/browser/ui/layout_constants.h
[modify] https://crrev.com/2b9a46a98f6334cf6701e79acfe239796dc018a2/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
[modify] https://crrev.com/2b9a46a98f6334cf6701e79acfe239796dc018a2/chrome/browser/ui/toolbar/toolbar_actions_bar.h
[modify] https://crrev.com/2b9a46a98f6334cf6701e79acfe239796dc018a2/chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc

(I'm not familiar with mac) 

The button was already 24x24 with 2pt (in the case of the normal MD mode) padding from all sides, see ToolbarActionsBar::GetViewSize() [1] and TOOLBAR_ACTION_VIEW [2]. I don't think there was a need to add that left_padding. You probably just needed to reduce the size of the button highlight (if possible), or maybe change ToolbarActionsBar::GetViewSize(), or how the buttons are laid out in the mac implementation.


[1]: https://cs.chromium.org/chromium/src/chrome/browser/ui/toolbar/toolbar_actions_bar.cc?type=cs&q=ToolbarActionsBar::GetViewSize&sq=package:chromium&l=137

[2]: https://cs.chromium.org/chromium/src/chrome/browser/ui/layout_constants.cc?type=cs&q=TOOLBAR_ACTION_VIEW&sq=package:chromium&l=125

Also, if we will keep the left_padding, then BrowserActionsContainer::OnDragUpdated() and BrowserActionsContainer::OnPaint() should be updated accordingly.

I don't think this was the right change.  First, there's nothing Mac-specific about this; the placements are the same on Windows.  I don't think our concern should be pixel-matching the Cocoa toolbar, but first matching other platforms and then ensuring the cross-platform behavior is correct.  If there's a bug here, it's a cross-platform bug.

Second, looking at buttons on each side of the omnibox on Windows, both have 6 px (at 100%) between the adjacent button's ink drop and the omnibox border.  Moving extensions over by some amount removes this symmetry, which is a regression rather than a fix.

What's more interesting is that the left-side buttons have 4 px between adjacent buttons (instead of 6) and the browser actions have 8 px (again, instead of 6).  The fact that these don't match each other seems like the biggest issue.  The fact that they don't match the 6 value seems like a potential second issue.

Third, we're revamping this stuff in both touchable CrOS and MD refresh, and it would be nice to be coherent across all these instead of fixing this as an isolated issue.

So my preference here would be to revert the CL in comment 8, look at the touchable/refresh specs to see what they suggest, and then probably fix the cross-platform code so that the buttons before and after the omnibox agree with each other on intra-button spacing, and that spacing value is to spec in at least the above modes.

Reopening to at least get a comment in reply to #11, if not a revert of the CL here in favor of fixing a different way.

Hm, good point. I'll go ahead and revert this. We should probably create a new bug for the issue you mentioned with the browser actions. It would probably make sense to have 6px for both sides. Is there someone we should rope in to decide on that?

I suspect what we really want is more like "4 everywhere", but my hope is that this is made more clear already in the touchable and refresh specs.  If not, then what we'd want is for bettes/sgabriel to agree on some route forward.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6472927282796d57f26365da62ca10eec13c469d

commit 6472927282796d57f26365da62ca10eec13c469d
Author: spqchan <spqchan@chromium.org>
Date: Tue Apr 10 09:58:15 2018

Revert "[MacViews] Fix for incorrect extension padding"

This reverts commit 2b9a46a98f6334cf6701e79acfe239796dc018a2.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> [MacViews] Fix for incorrect extension padding
>
> Move the extensions 2pt to the right on Mac to match the native Mac
> placements.
>
> Bug:  792593 
> Change-Id: I579520a37043667dfa289b110287c034447c3812
> Reviewed-on: https://chromium-review.googlesource.com/982353
> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> Commit-Queue: Sarah Chan <spqchan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#546536}

TBR=ellyjones@chromium.org,spqchan@chromium.org

Bug:  792593 
Change-Id: I517f941cd1b950e45d2879636e1ad0c07c2f5a49
Reviewed-on: https://chromium-review.googlesource.com/989535
Commit-Queue: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Sarah Chan <spqchan@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549472}
[modify] https://crrev.com/6472927282796d57f26365da62ca10eec13c469d/chrome/browser/ui/layout_constants.cc
[modify] https://crrev.com/6472927282796d57f26365da62ca10eec13c469d/chrome/browser/ui/layout_constants.h
[modify] https://crrev.com/6472927282796d57f26365da62ca10eec13c469d/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
[modify] https://crrev.com/6472927282796d57f26365da62ca10eec13c469d/chrome/browser/ui/toolbar/toolbar_actions_bar.h
[modify] https://crrev.com/6472927282796d57f26365da62ca10eec13c469d/chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc

CL reverted. I'm going to open a new bug regarding the uneven spacing