Can we stop eliding bookmark titles in the Mac mainMenu bar? (aka Mac mainMenu bookmarks doesn't elide subfolders consistently) |
|||||
Issue description
Chrome Version : 69.0.3497.12
See attached.
I suspect this is a non-regression issue, and the fix is easy. Basically
void BookmarkMenuBridge::AddNodeAsSubmenu(NSMenu* menu,
const BookmarkNode* node,
NSImage* image) {
NSString* title = base::SysUTF16ToNSString(node->GetTitle());
base::scoped_nsobject<NSMenuItem> items(
[[NSMenuItem alloc] initWithTitle:title action:nil keyEquivalent:@""]);
becomes
..
NSString* title = [BookmarkMenuCocoaController menuTitleForNode:node];
..
Also this is now the only place we use the Cocoa bookmarks menu bridging goop. Perhaps we just want to leave eliding up to Cocoa anyway (which is what currently happens for subfolders).
i.e. the fix is one of
A. make items match subfolders, or
B. make subfolders match items
My preference is for A. i.e. don't limit the menu width to be narrower than the native Cocoa limit.
Prior to MacViews doing A., would have affected the bookmarks menu in the Chrome app/hotdog menu as well. But that's a different codepath now.
A. allows us to purge the use of gfx::ElideText() from the Cocoa menu code which (a) requires RenderTextMac which we want to die, and (b) is slow.
,
Aug 14
#c1 fixes the inconsistency. But it would be nicer to not elide to a smaller width than what native menus do, IMO. Note that the native eliding is "different" and also also "nicer". It - is much wider - elides the middle, not the tail - applies kerning, so elided labels are fully justified
,
Aug 16
Tried to reproduce the issue on Mac OS 10.13.3 on the reported version 69.0.3497.12 and the latest M-70 build 70.0.3524.0. Could scroll through the bookmarks from Mac menu and couldn't observe any difference between the behaviors in the build without fix and latest M-70. Attached is the screen cast for reference. bettes@ Request you to check and help us in verifying the fix on the M-70 build. Thanks..
,
Aug 17
bettes: question still for you is whether we can just use AppKit's eliding behavior for the mainMenu bar. (see #c2 for rationale) #c3: to repro the issue, you need a _folder_ with a really really long name. (e.g. Rename a folder via the bookmarks toolbar, then open the mainMenu->Bookmarks menu again.) I compared 70.0.3521.0 and 70.0.3524.3 and the fix seems to be working correctly.
,
Oct 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/726cfc3b2338027e1f9b8be2dfa9c6b40281a35f commit 726cfc3b2338027e1f9b8be2dfa9c6b40281a35f Author: Trent Apted <tapted@chromium.org> Date: Thu Oct 25 02:42:52 2018 Elide items in the macOS mainMenu according to platform conventions. Currently we elide page titles in the middle for history at 400px, at the tail for bookmarks at 300px. Menus should not elide tail because an ellipsis there is an affordance for "this will show a dialog". History in the app menu wants to elide at 320px (according to RecentTabsSubMenuModel:: GetMaxWidthForItemAtIndex()), but actually elides at 800px because AppMenu::GetMaxWidthForMenu doesn't call that. But also History in the app menu is a very different list to the one in the mainMenu bar. And none of these elide page titles the way the macOS `Window` menu does. We have no control over that unless we change the window title in other places as well. Anyway, it's a mess. Eliding strings ourselves pulls in lots of code and we better match platform expectations if we just let NSMenu do eliding for us. NSMenu elides dynamically based on the screen space available when the [sub]menu is shown. Bug: 889152, 869270 Change-Id: I76d1e996f86b3f3f09c9e5ca68d117b4f4686089 Reviewed-on: https://chromium-review.googlesource.com/c/1250311 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Peter Boström <pbos@chromium.org> Cr-Commit-Position: refs/heads/master@{#602588} [modify] https://crrev.com/726cfc3b2338027e1f9b8be2dfa9c6b40281a35f/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm [modify] https://crrev.com/726cfc3b2338027e1f9b8be2dfa9c6b40281a35f/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge_unittest.mm [modify] https://crrev.com/726cfc3b2338027e1f9b8be2dfa9c6b40281a35f/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.h [modify] https://crrev.com/726cfc3b2338027e1f9b8be2dfa9c6b40281a35f/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.mm [modify] https://crrev.com/726cfc3b2338027e1f9b8be2dfa9c6b40281a35f/chrome/browser/ui/cocoa/history_menu_bridge.mm [modify] https://crrev.com/726cfc3b2338027e1f9b8be2dfa9c6b40281a35f/ui/base/cocoa/menu_controller.h [modify] https://crrev.com/726cfc3b2338027e1f9b8be2dfa9c6b40281a35f/ui/base/cocoa/menu_controller.mm
,
Oct 25
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Aug 14