New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 796836 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression



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 description

Chrome 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.
 
Actual_Result.mov
766 KB Download
Expected_Result.mov
989 KB Download

Comment 1 by avsha...@etouch.net, Dec 21 2017

Labels: hasbisect-per-revision
Owner: 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.)
This is a regression issue broken in ‘M-65’ and using the per-revision bisect providing the bisect results,
Good build : 65.0.3299.0 (Revision : 524907)
Bad build : 65.0.3300.0 (Revision : 525555)

You are probably looking for a change made after 525476 (known good), but no later than 525477 (first known bad).

CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/ef62139da55ec66c148556896ca3bf0b23b886a0..c6dd96ffb49cea709f1373d01e7e7e19307a886f

Suspect : https://chromium.googlesource.com/chromium/src/+/c6dd96ffb49cea709f1373d01e7e7e19307a886f

@sdy : Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Note : This is Mac OS specific issue and the same is not observed on Windows(7,8,10) and Linux(14.04 LTS) OS.

Comment 2 by sdy@chromium.org, Dec 27 2017

Cc: sdy@chromium.org
 Issue 797319  has been merged into this issue.

Comment 3 by sdy@chromium.org, Dec 27 2017

Labels: ReleaseBlock-Stable
Status: Started (was: Assigned)
Working on this now. Adding RBS from the issue I just merged.
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by sdy@chromium.org, Jan 2 2018

Status: Fixed (was: Started)
This should be fixed in tomorrow's canary.
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
 
2pt_too_much_padding_favicon_only.png
19.3 KB View Download
2pt_too_much_padding_favicon_with_text.png
19.9 KB View Download
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.


spacing_between_bookmarks.png
46.0 KB View Download

Comment 8 by sdy@chromium.org, Jan 2 2018

Status: Started (was: Fixed)
mehmet@: Thanks for catching this. I'm working on a fix.
Reopening per c#6.
Labels: TE-Verified-M65 TE-Verified-65.0.3310.0
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!
Current_Result.mov
663 KB Download
Project Member

Comment 12 by bugdroid1@chromium.org, 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

Comment 13 by sdy@chromium.org, Jan 3 2018

Status: Fixed (was: Started)
mehmet@, let me know if this change fixed the spacing issue you found.
Labels: TE-Verified-65.0.3311.0
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!
Latest_Results.mov
2.1 MB Download

Comment 15 by sdy@chromium.org, Jan 4 2018

Status: Verified (was: Fixed)
Nice, thank you for checking!
> 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 :-)
+ screencast
elided_bookmarkname_padding_right.mov
1.6 MB Download

Comment 18 by sdy@chromium.org, 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.
--> Done:  issue 799096 
Labels: RegressedIn-65 Target-65 FoundIn-65
Labels: Hotlist-ConOps

Sign in to add a comment