[MdRefresh] Attached Bookmarks Bar could have 1px more white space at the bottom |
|||||||||||||
Issue descriptionChrome Version: Canary 69.0.3496.0 OS: OS=All What steps will reproduce the problem? (1) Attach the Bookmarks Bar (2) Hover over a BMB item (3) Compare Space between unfocussed Omnibox and top of the botton with the bottom space between bottom of the button and the bottom border line. What is the expected result? Bottom space could have 1px more to be also 4px like the bottom space which is also 4px. What happens instead? At the bottom the white space is only 3px. This could maybe fixed together with similar issue 863697 which is for the detached BMB. Screenshots are attached. Thanks :) Mehmet
,
Jul 26
,
Jul 26
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c86ba838fcfac0de766c5448a2dc5a35c28ca18 commit 8c86ba838fcfac0de766c5448a2dc5a35c28ca18 Author: Charlene Yan <cyan@chromium.org> Date: Wed Aug 08 15:52:05 2018 Center bookmark bar icons. Simplifying the button padding calculation for the bookmark bar. Checks that it is appropriately centered in attached and detached state as well as with and without an info bar and tab switching. Bug: 865555 , 864595 , 863697 Change-Id: I4550c65af596d51ca41b7ac3be2c8f14825e304f Reviewed-on: https://chromium-review.googlesource.com/1155369 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Charlene Yan <cyan@chromium.org> Cr-Commit-Position: refs/heads/master@{#581574} [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/bookmarks/bookmark_bar_view.h [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/frame/browser_view_layout.cc [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/frame/browser_view_layout.h [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/frame/browser_view_layout_unittest.cc [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/frame/browser_view_unittest.cc [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/frame/contents_layout_manager.cc [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/frame/contents_layout_manager.h [modify] https://crrev.com/8c86ba838fcfac0de766c5448a2dc5a35c28ca18/chrome/browser/ui/views/toolbar/toolbar_view.cc
,
Aug 8
Hey cyan@. Thanks for the CL. I installed latest Chromium Trunk #581596 which includes your fix and I noticed that your CL changes the vertical height of the Bookmarks Buttons from 28px to 32px on the detached BMB on my MacBook Air. This looks very high and inconsistent to the Bookmarks buttons (28px) on the attached BMB. Please find enclosed a screenshot. There you can see that the Button is no longer circular when you only have the favicon on the detached BMB. Can we please change it back to 28px? Thanks :)
,
Aug 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c9953911862ace8cd2c114d1c52bd85cda3306ae commit c9953911862ace8cd2c114d1c52bd85cda3306ae Author: Charlene Yan <cyan@chromium.org> Date: Mon Aug 20 16:36:56 2018 Recalculate positioning and height of bookmark bar icons. The calculations for bookmark bar icons should only impact the start y location of the icons and not the height. Regression mentioned in comment #5 in the bug is due to removing kBookmarkBarBottomMargin in the previous CL for the bug. The underlying reason for this is that GetPreferredHeight() was being used for both the bookmark height as well as the button heights. Bug: 865555 Change-Id: Ief5328a2a3a8c23ebfbaa9a393f1c3af4c7a6920 Reviewed-on: https://chromium-review.googlesource.com/1170226 Commit-Queue: Charlene Yan <cyan@chromium.org> Reviewed-by: Peter Boström <pbos@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#584470} [modify] https://crrev.com/c9953911862ace8cd2c114d1c52bd85cda3306ae/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc [modify] https://crrev.com/c9953911862ace8cd2c114d1c52bd85cda3306ae/chrome/browser/ui/views/bookmarks/bookmark_bar_view.h [modify] https://crrev.com/c9953911862ace8cd2c114d1c52bd85cda3306ae/chrome/browser/ui/views/chrome_layout_provider.cc [modify] https://crrev.com/c9953911862ace8cd2c114d1c52bd85cda3306ae/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/c9953911862ace8cd2c114d1c52bd85cda3306ae/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/c9953911862ace8cd2c114d1c52bd85cda3306ae/chrome/browser/ui/views/material_refresh_layout_provider.cc
,
Aug 20
un-assigning for now since this seems to have a lot more cases than I initially realized The bookmark bar will have to be 1 pixel larger based on if the icon is an even or odd number of pixels high. It also would need to take into account the amount of spacing below the location bar since we decided earlier that the bookmark icons should not technically be on the toolbar https://cs.chromium.org/chromium/src/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc?l=963&rcl=b13951395ae24d044205fe9e830c169c62088b50
,
Aug 20
,
Aug 21
,
Sep 13
,
Sep 20
,
Sep 26
,
Oct 11
,
Oct 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/55fd02ef2c0402988de932830dd65d4083895389 commit 55fd02ef2c0402988de932830dd65d4083895389 Author: Charlene Yan <cyan@chromium.org> Date: Tue Oct 30 00:52:39 2018 Add an additional pixel to the attached bookmarks bar. BOOKMARK_BAR_HEIGHT gives the height of the bookmark when attached so giving an additional pixel here to the vertical margin of the bookmark bar. This makes it so there are the same number of pixels above and below each button on the bookmarks bar when the bookmark is attached. Bug: 865555 Change-Id: I271bfb7f48ef2faa390a8c9ae595c165dbc0bde9 Reviewed-on: https://chromium-review.googlesource.com/c/1280928 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Charlene Yan <cyan@chromium.org> Cr-Commit-Position: refs/heads/master@{#603707} [modify] https://crrev.com/55fd02ef2c0402988de932830dd65d4083895389/chrome/browser/ui/layout_constants.cc
,
Dec 11
,
Dec 13
This is fixed needs verification.
,
Dec 13
Thanks, looks perfect to me (on my non-retina device). Same spacing on top/bottom. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by pbos@chromium.org
, Jul 19Owner: cyan@chromium.org