Issue metadata
Sign in to add a comment
|
Regression : [Mac] Unwanted text continuation is seen for ‘Apps’ shortcut in bookmarks bar.
Reported by
avsha...@etouch.net,
Dec 21 2017
|
||||||||||||||||||||||
Issue descriptionChrome version : 65.0.3300.0 (Official Build) dea3d7abb6ccc703942ae522dd2069d70dbd4b68-refs/heads/master@{#525555} 64 bit OS : Mac(10.12.6) (10.13.3) What steps will reproduce the problem? 1. Launch chrome, open NTP and observe ‘Apps’ shortcut in bookmarks bar. Actual Result : Text continuation is seen for ‘Apps’ shortcut in bookmarks bar. Expected Result : Instead, complete name should be seen for ‘Apps’ shortcut. This is a regression issue broken in ‘M-65’ and will soon provide remaining info.
,
Dec 27 2017
,
Dec 27 2017
Working on this now. Adding RBS from the issue I just merged.
,
Jan 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f57b9e14a1145268314b297e80f38c529f94535a commit f57b9e14a1145268314b297e80f38c529f94535a Author: Sidney San Martín <sdy@chromium.org> Date: Tue Jan 02 19:53:39 2018 Add insetInView when calculating a width for a BookmarkButtonCell. This fixes some short bookmark button titles getting truncated. Bug: 796836 Change-Id: I7bf68b5b5ccc7d48923da231feccb97f9c0caa34 Reviewed-on: https://chromium-review.googlesource.com/845821 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Sidney San Martín <sdy@chromium.org> Cr-Commit-Position: refs/heads/master@{#526520} [modify] https://crrev.com/f57b9e14a1145268314b297e80f38c529f94535a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm [modify] https://crrev.com/f57b9e14a1145268314b297e80f38c529f94535a/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm [modify] https://crrev.com/f57b9e14a1145268314b297e80f38c529f94535a/chrome/browser/ui/cocoa/gradient_button_cell.h [modify] https://crrev.com/f57b9e14a1145268314b297e80f38c529f94535a/chrome/browser/ui/cocoa/gradient_button_cell.mm
,
Jan 2 2018
This should be fixed in tomorrow's canary.
,
Jan 2 2018
Hey sdy@, Happy New Year and thanks for fixing this issue. I checked it in latest Snapshot 526532 and unfortunately the padding on the right side is no longer correct. It has 2pt too much padding now on the right side and the single favicon or favicon_with_text are no longer centered on the button. It should be 4pt too on the right side again. Attached are screenshots. Could you please take a look at this issue again? Thanks! Mehmet
,
Jan 2 2018
Additional notes to my comment 6: - I also noticed, that the spacing between the bookmarks has also been changed and has more padding than before. A screenshot is attached.
,
Jan 2 2018
mehmet@: Thanks for catching this. I'm working on a fix.
,
Jan 2 2018
Reopening per c#6.
,
Jan 2 2018
Change is up for review: https://chromium-review.googlesource.com/c/chromium/src/+/848032
,
Jan 3 2018
Update : Retested the above issue on Mac(10.12.6), (10.13.1) & (10.13.3) OS using latest Canary #65.0.3310.0 and issue is fixed. Now, complete name is seen for 'Apps' instead of continuation. Kindly review an attached screen-cast. Thank you!
,
Jan 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/84077a2b7d10b90f92cc43df232723cbbf46880a commit 84077a2b7d10b90f92cc43df232723cbbf46880a Author: Sidney San Martín <sdy@chromium.org> Date: Wed Jan 03 18:09:45 2018 Fix a regression that made bookmark buttons too wide. r526520 started including a bookmark button's inset when calculating its width. This turns out to change the layout and, because the inset is cr_lineWidth, there's no way to pick padding constants which make the padding and spacing the same on retina and non-retina. The inset (which comes from GradientButton) isn't really used in MD, so overriding insetInView: to return zero gets the right behavior back. This class's layout code could still use cleanup. Bug: 796836 Change-Id: I52741d9fb92db92fa07ba8b27077d8451383a47f Reviewed-on: https://chromium-review.googlesource.com/848032 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Sidney San Martín <sdy@chromium.org> Cr-Commit-Position: refs/heads/master@{#526739} [modify] https://crrev.com/84077a2b7d10b90f92cc43df232723cbbf46880a/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
,
Jan 3 2018
mehmet@, let me know if this change fixed the spacing issue you found.
,
Jan 4 2018
Update : Verified this issue on Mac(10.12.6), (10.13.1) & (10.13.3) OS with latest Canary build #65.0.3311.0 and the issue is no longer reproducible. The fix is working as intended. Kindly review an attached screen-cast. Thank you!
,
Jan 4 2018
Nice, thank you for checking!
,
Jan 4 2018
> mehmet@, let me know if this change fixed the spacing issue you found. Thank you sdy@. Spacing and padding is looking fine again :-) Just only one thing I noticed, but I think we can live with it is, that depending on the length and the letters when the bookmark name is elided, the padding on the right side is sometimes smaller or larger. It should also always be the same padding like on the left side. Please find attached a screencast. Please let me know, if I should file a separate report for this issue or if you think we can leave it as it is. Thanks :-)
,
Jan 4 2018
+ screencast
,
Jan 4 2018
Oh, interesting. Please do file a bug — I'm guessing that it's always been like that, but it would be good to fix either way.
,
Jan 4 2018
--> Done: issue 799096
,
Jan 9 2018
,
Jan 10 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by avsha...@etouch.net
, Dec 21 2017Owner: sdy@chromium.org
Status: Assigned (was: Unconfirmed)
Summary: Regression : [Mac] Unwanted text continuation is seen for ‘Apps’ shortcut in bookmarks bar. (was: Regression : Text continuation is seen for ‘Apps’ shortcut in bookmarks bar.)