[MdRefresh] The items on the "Detached" Bookmarks Bar could be 1px lower to be vertically centered. |
|||||||||
Issue descriptionChrome Version: Canary 69.0.3491.0 OS: macOS 10.13.6 but probably OS=All What steps will reproduce the problem? (1) Start Canary (2) Detach the Bookmarks Bar (3) Be sure that you are on the NTP, so that the detached Bookmarks Bar is visible (4) Mouse over a bookmark item, so that the hover button appears (5) Compare the white space above and below the hover button What is the expected result? All items on the "Detached" Bookmarks Bar could be 1px lower to be vertically centered. What happens instead? You see above 2px and below 4px space. Screenshots are attached. Thanks for looking into this issue :) Mehmet
,
Jul 16
This might be a good first bug. :)
,
Jul 16
+bettes@ FYI, let us know if you disagree but I think this makes sense.
,
Jul 17
Triage: Unassigning for now.
,
Jul 17
She's already started, assigning back.
,
Jul 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8025aa7e1614a59fb70f1f9589bb1e0d933f656e commit 8025aa7e1614a59fb70f1f9589bb1e0d933f656e Author: Charlene Yan <cyan@chromium.org> Date: Tue Jul 17 19:10:05 2018 Changed kBottomMargin of the detached bookmark bar from 4 to 2 pixels. This makes the detached bookmarks items vertically centered. Bug: 863697 Change-Id: Icf6529ec0b33255dc8072d5bfacc2f32b0495f93 Reviewed-on: https://chromium-review.googlesource.com/1139191 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Charlene Yan <cyan@chromium.org> Cr-Commit-Position: refs/heads/master@{#575746} [modify] https://crrev.com/8025aa7e1614a59fb70f1f9589bb1e0d933f656e/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
,
Jul 18
Hi cyan@, thanks for fixing it for the detached BMB. I checked latest Canary and it is looking fine centered (4px top/4px bottom) in the detached Bookmarks Bar. But your change has regressed it on the attached Bookmarks bar. The button is no longer vertically centered. It is now 4px top / 1px bottom :( Can you give the bottom again +3px so that it nicely centered 4px top / 4px bottom? Thanks in advance.
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/49ebcfa3d6fd8eda69b9f85ab69c30bd68a277e7 commit 49ebcfa3d6fd8eda69b9f85ab69c30bd68a277e7 Author: Peter Boström <pbos@chromium.org> Date: Wed Jul 18 17:49:52 2018 Revert "Changed kBottomMargin of the detached bookmark bar from 4 to 2 pixels." This reverts commit 8025aa7e1614a59fb70f1f9589bb1e0d933f656e. Reason for revert: Inadvertently affects the attached bookmarks bar. Original change's description: > Changed kBottomMargin of the detached bookmark bar from 4 to 2 pixels. > > This makes the detached bookmarks items vertically centered. > > Bug: 863697 > Change-Id: Icf6529ec0b33255dc8072d5bfacc2f32b0495f93 > Reviewed-on: https://chromium-review.googlesource.com/1139191 > Reviewed-by: Bret Sepulveda <bsep@chromium.org> > Commit-Queue: Charlene Yan <cyan@chromium.org> > Cr-Commit-Position: refs/heads/master@{#575746} TBR=bsep@chromium.org,cyan@chromium.org Change-Id: I9381f0796b85cd658d2101981e3a1ef7071fb4ab No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 863697 Reviewed-on: https://chromium-review.googlesource.com/1142168 Reviewed-by: Peter Boström <pbos@chromium.org> Commit-Queue: Peter Boström <pbos@chromium.org> Cr-Commit-Position: refs/heads/master@{#576127} [modify] https://crrev.com/49ebcfa3d6fd8eda69b9f85ab69c30bd68a277e7/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
,
Jul 18
Thanks mehmet@, good catch. The code is actually trying to vertically center the bookmarks but failing (the bookmarks are pushed upwards), cyan@'ll look into why that is instead. I think fixing that might make the margins 3px on both sides instead which would also center them.
,
Jul 18
Okay, thanks for looking into this again :)
,
Jul 19
,
Jul 25
Issue 864197 has been merged into this issue.
,
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 20
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by meh...@chromium.org
, Jul 14