New issue
Advanced search Search tips
Starred by 11 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 7
Components:
EstimatedDays: 10
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 821991
issue 834508



Sign in to add a comment

Basic restyling of the toolbar

Project Member Reported by pbos@chromium.org, Mar 14

Issue description

Supposed to cover resizing, recoloring and relayouting the toolbar per spec.
 
Labels: Proj-MdRefresh
Owner: pbos@chromium.org
Status: Assigned (was: Available)
Components: -UI>Browser>TabStrip UI>Browser>Toolbar
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 23

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

commit d12bc2537e8bcb9be2a70f92fb293c7157f14587
Author: Peter Boström <pbos@chromium.org>
Date: Fri Mar 23 00:01:31 2018

Fix glitchy toolbar drawing in MD refresh.

Incorrect inconsistent in GetLayoutConstant() caused incorrect black
repainting of the location bar and later causing DCHECKs to hit further
in if not straight up crashing.

Also fixing an OOB read for TOOLBAR_ACTION_VIEW where the constant was
missing. These constants now more closely resemble MATERIAL_NORMAL.

Bug:  chromium:822069 
Change-Id: I6add56fc8cce1296164b49d98014a5a4167e756c
Reviewed-on: https://chromium-review.googlesource.com/976877
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545294}
[modify] https://crrev.com/d12bc2537e8bcb9be2a70f92fb293c7157f14587/chrome/browser/ui/layout_constants.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 23

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

commit 305ca5ccec007d5cc178026103698e58af21f62b
Author: Peter Boström <pbos@chromium.org>
Date: Fri Mar 23 18:06:10 2018

Use touch-theme colors for MD refresh.

Colors the toolbar white under Material refresh among others. These
colors are already used by the touch UI. This checks IsNewerMaterialUi()
which is true for both touch-optimized and material refresh.

Bug: chromium:810165,  chromium:822069 
Change-Id: I293537b0b03806ef1205a33a895cffa4d9552e9d
Reviewed-on: https://chromium-review.googlesource.com/976807
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545518}
[modify] https://crrev.com/305ca5ccec007d5cc178026103698e58af21f62b/chrome/browser/themes/theme_properties.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 29

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

commit 2df6c89a410c7e9ac501b9fc9089b5f774faad59
Author: Peter Boström <pbos@chromium.org>
Date: Thu Mar 29 01:44:06 2018

Increase toolbar spacing in Material refresh

Increases toolbar element padding and standard spacing under the
Material refresh flag (to 4dip and 8dip respectively).

Bug:  chromium:822069 
Change-Id: I2305f1f6af6d3b7ac8d9590467793e17690077e7
Reviewed-on: https://chromium-review.googlesource.com/984863
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546696}
[modify] https://crrev.com/2df6c89a410c7e9ac501b9fc9089b5f774faad59/chrome/browser/ui/layout_constants.cc

Labels: -Pri-3 Target-69 Pri-2
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 11

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

commit 3701d7b54f3096e5bb6b22f652baa56790bb8396
Author: Peter Boström <pbos@chromium.org>
Date: Wed Apr 11 05:13:48 2018

Separate browser actions from trusted area

Adds a separator to the browser-actions container that shows under the
Material refresh. This ensures that browser actions are distint from
trusted built-in toolbar buttons (like the avatar button for switching
between users).

This separator only shows when browser actions are visible and animates
in and out along with them.

Bug:  chromium:822069 
Change-Id: Iad7a0121088932884c4f63e8a8340e4919023957
Reviewed-on: https://chromium-review.googlesource.com/987299
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549770}
[modify] https://crrev.com/3701d7b54f3096e5bb6b22f652baa56790bb8396/chrome/browser/ui/views/toolbar/browser_actions_container.cc
[modify] https://crrev.com/3701d7b54f3096e5bb6b22f652baa56790bb8396/chrome/browser/ui/views/toolbar/browser_actions_container.h
[modify] https://crrev.com/3701d7b54f3096e5bb6b22f652baa56790bb8396/chrome/browser/ui/views/toolbar/toolbar_view.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 14

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

commit ddbaa793e05a528867a36f60985873d6bfe8555b
Author: Peter Boström <pbos@chromium.org>
Date: Sat Apr 14 00:43:29 2018

Update toolbar inkdrop shapes for MD refresh

This shape is an 8dp rounded corner. This fix does not address colors.

Bug:  chromium:822069 
Change-Id: Ie6afa55b0ebe3b0f5c23eff7d54e9e60deb757ba
Reviewed-on: https://chromium-review.googlesource.com/999113
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Mohsen Izadi <mohsen@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550834}
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ash/shelf/shelf_button.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/chrome/browser/ui/views/download/download_item_view.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ui/views/animation/ink_drop_host_view.h
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ui/views/controls/button/md_text_button.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 17

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ddbaa793e05a528867a36f60985873d6bfe8555b

commit ddbaa793e05a528867a36f60985873d6bfe8555b
Author: Peter Boström <pbos@chromium.org>
Date: Sat Apr 14 00:43:29 2018

Update toolbar inkdrop shapes for MD refresh

This shape is an 8dp rounded corner. This fix does not address colors.

Bug:  chromium:822069 
Change-Id: Ie6afa55b0ebe3b0f5c23eff7d54e9e60deb757ba
Reviewed-on: https://chromium-review.googlesource.com/999113
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Mohsen Izadi <mohsen@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550834}
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ash/shelf/shelf_button.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/chrome/browser/ui/views/download/download_item_view.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ui/views/animation/ink_drop_host_view.h
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/ddbaa793e05a528867a36f60985873d6bfe8555b/ui/views/controls/button/md_text_button.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 17

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

commit 3ea9a75160dfa00d72aaa1e40c25c697797886c0
Author: Peter Boström <pbos@chromium.org>
Date: Tue Apr 17 23:43:30 2018

Use 8dp corners for browser actions in MD refresh

Extends 8dp corners to ToolbarActionView which was missed in a previous
change. This makes them consistent with other toolbar buttons.

Bug:  chromium:822069 
Change-Id: Id894664ec698504e3ff997c1b12691821ab1062e
Reviewed-on: https://chromium-review.googlesource.com/1015286
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551523}
[modify] https://crrev.com/3ea9a75160dfa00d72aaa1e40c25c697797886c0/chrome/browser/ui/views/toolbar/toolbar_action_view.cc

Blocking: 834508
Blocking: 834511
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 30

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

commit bbcce752dbe0afe0913000e7726267564bde7abf
Author: Peter Boström <pbos@chromium.org>
Date: Mon Apr 30 21:33:10 2018

Fix toolbar button insets for Touch and Refresh

This change accomplishes a couple of things:

* Unifies Touchable and Refresh layout inkdrop behavior.
* Gets 8dp inkdrop corners for Touchable
* Removes hard-coded inkdrop sizes and insets.
* Makes the flood-fill inkdrop work with extended edges (toolbar edges).
  This was not a problem for touchable as they removed extended edges.
* Changes Refresh inkdrop size to 28dps instead of inkdrop-default 24dp.
* Uses flood-fill inkdrop for Refresh.
* Increases toolbar horizontal padding to 8dp per specs.

Bug:  chromium:822069 ,  chromium:834511 
Change-Id: I458fecfb14ca2d21dcf9513756182b2bf8e9320e
Reviewed-on: https://chromium-review.googlesource.com/1033962
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554884}
[modify] https://crrev.com/bbcce752dbe0afe0913000e7726267564bde7abf/chrome/browser/ui/layout_constants.cc
[modify] https://crrev.com/bbcce752dbe0afe0913000e7726267564bde7abf/chrome/browser/ui/layout_constants.h
[modify] https://crrev.com/bbcce752dbe0afe0913000e7726267564bde7abf/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/bbcce752dbe0afe0913000e7726267564bde7abf/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/bbcce752dbe0afe0913000e7726267564bde7abf/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/bbcce752dbe0afe0913000e7726267564bde7abf/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.h
[modify] https://crrev.com/bbcce752dbe0afe0913000e7726267564bde7abf/chrome/browser/ui/views/toolbar/toolbar_view.cc

Blocking: -834511
Project Member

Comment 17 by bugdroid1@chromium.org, May 8

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

commit 23fca9f451a75fbf1fdce94dd8fcdd7b1fdaf093
Author: Peter Boström <pbos@chromium.org>
Date: Tue May 08 02:41:02 2018

Use toolbar separator color for trusted area

Uses COLOR_TOOLBAR_VERTICAL_SEPARATOR instead of the toolbar button
color. This matches the separators used in the bookmarks and download
bars.

Bug:  chromium:822069 
Change-Id: Iea05d91bebf432f90e58d09104118cb322b00e23
Reviewed-on: https://chromium-review.googlesource.com/1048842
Commit-Queue: Peter Boström <pbos@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556654}
[modify] https://crrev.com/23fca9f451a75fbf1fdce94dd8fcdd7b1fdaf093/chrome/browser/ui/views/toolbar/toolbar_view.cc

Project Member

Comment 18 by bugdroid1@chromium.org, May 11

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

commit 404e6fccc348d8781d25888d0f3aa587a8806739
Author: Peter Boström <pbos@chromium.org>
Date: Fri May 11 00:46:34 2018

Use circular ink-drops for toolbar and bookmarks

Adds circular inkdrops for the toolbar and pill-shaped ones for entries
in the bookmarks bar for Refresh. This removes the recent move to 8dp
squircles for Refresh and Touch and instead unifies them as rounded.

Also tones down EMPHASIS_HIGH for non-touchable and non-refresh as they
are not ready for rounded corners.

Bug:  chromium:822069 ,  chromium:822072 
Change-Id: Ie619f40834127ebfde59621a86fae42e7d8c4f23
Reviewed-on: https://chromium-review.googlesource.com/1053379
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557743}
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/chrome/browser/ui/views/harmony/chrome_layout_provider.h
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/chrome/browser/ui/views/harmony/material_refresh_layout_provider.cc
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/chrome/browser/ui/views/harmony/material_refresh_layout_provider.h
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/404e6fccc348d8781d25888d0f3aa587a8806739/ui/views/layout/layout_provider.cc

It's important to me that the spacing between extension icons is 4dp, just as they are with the rest of the toolbar controls. Not sure if this is the right bug to track that.
I assume you're referring to Mac and spacing between focus rings. Is this correct? We're kind of cheating outside Mac so I can imagine this being busted on Mac for this reason (there's technically no padding between items it just happens to add up to 4dp padding with the fixed-size inkdrops.

Mac doesn't have inkdrops so I believe the focus rings fill the entire view which probably is why you're not seeing the same spacing and size. Is this the case or are you observing something different? Can you post a screenshot?
I think the hover buttons of the extensions are directly next to each other without the 4dp spacing between each other and to the Omnibox. You can compare it with the profile and the Wrench icon hover buttons. Attached a screencast. I hope it helps :-)
extension_buttons_hover_no_spacing.mov
315 KB View Download
Project Member

Comment 22 by bugdroid1@chromium.org, May 23

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

commit 820f59a752e13841e4a599b06fade3928abd0890
Author: Peter Boström <pbos@chromium.org>
Date: Wed May 23 11:39:12 2018

Use views-sized browser actions for MacViews

Fixes bug where the larger MD Refresh 28dp inkdrops overflow into
browser-action insets. The prior code returned 24dp icon areas as it
assumed that MacOS was always using the Cocoa implementation. This now
effectively matches ToolbarButton sizes in views.

Bug:  chromium:822069 
Change-Id: I27a8b2c36fe0bba57ee698aae940b1b0c472f29f
Reviewed-on: https://chromium-review.googlesource.com/1068179
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561037}
[modify] https://crrev.com/820f59a752e13841e4a599b06fade3928abd0890/chrome/browser/ui/toolbar/toolbar_actions_bar.cc

EstimatedDays: 10
pbos: please update estimated days...
ping back if there is a lot of remaining work.
Project Member

Comment 25 by bugdroid1@chromium.org, May 31

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

commit 7fcfc72c61f88a2dab963b45c336f6d535614dfc
Author: Peter Boström <pbos@chromium.org>
Date: Thu May 31 22:21:47 2018

Unify toolbar-related ink-drop colors

* Makes bookmarks-bar buttons use the toolbar ink-drop base color.
* Use the previously-touch opacities for hovered and selected ink-drop
  states unconditionally for toolbar buttons, extension icons, bookmark
  buttons and the 3-dot menu.
* Renames kTouchToolbar constants to reflect that they're no longer
  touch-only constants. Also does corresponding comment cleanup.

Bug: chromium:821996,  chromium:822069 ,  chromium:846416 
Change-Id: Ia28642afb2dd7381bff59428405fa895559be8bd
Reviewed-on: https://chromium-review.googlesource.com/1080552
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563398}
[modify] https://crrev.com/7fcfc72c61f88a2dab963b45c336f6d535614dfc/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/7fcfc72c61f88a2dab963b45c336f6d535614dfc/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/7fcfc72c61f88a2dab963b45c336f6d535614dfc/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/7fcfc72c61f88a2dab963b45c336f6d535614dfc/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/7fcfc72c61f88a2dab963b45c336f6d535614dfc/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.h

Project Member

Comment 26 by bugdroid1@chromium.org, Jun 1

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

commit 1526d82653413c5c812f1b6d869e7b29389f5398
Author: Peter Boström <pbos@chromium.org>
Date: Fri Jun 01 12:12:38 2018

Add spacing for browser actions' resize area

This makes the toolbar area look less cramped, especially for Material
refresh but even so in the current layout. Before this the resize area
would overlap existing elements and there would be no spacing between
this area and the omnibox.

Bug:  chromium:822069 ,  chromium:846353 
Change-Id: I9eb566fe4369c1aae2ac275b6312effa67a0255a
Reviewed-on: https://chromium-review.googlesource.com/1015620
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563597}
[modify] https://crrev.com/1526d82653413c5c812f1b6d869e7b29389f5398/chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc
[modify] https://crrev.com/1526d82653413c5c812f1b6d869e7b29389f5398/chrome/browser/ui/views/toolbar/browser_actions_container.cc

Status: Fixed (was: Assigned)
Triage: closing this, please open new bugs for todos.

Sign in to add a comment