New issue
Advanced search Search tips

Issue 889152 link

Starred by 3 users

Issue metadata

Status: Available
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

RenderTextMac can likely be removed now

Project Member Reported by asvitk...@chromium.org, Sep 25

Issue description

RenderTextMac can likely be removed now that MacViews launched.

I believe originally we had a set up where RenderTextMac was used for non-editable text and RenderTextHarfbuzz for editable text. But with MacViews, I think everything can use RenderTextHarfbuzz and so RenderTextMac can be removed.

Filing for Mac triage.
 
Labels: -Pri-3 Proj-MacViews Pri-2
This is sortof a dupe of  Issue 454835  but it got archived.

Note there are still places in the UI where a string rendered is by Cocoa.. i.e. tooltips and native menus (the mainMenu bar). Ooh, also system notifications.


grep for gfx::Typesetter::NATIVE shows legitimate uses:

 - tooltip on downloads bar that elides the filename
   * gfx::ElideFilename, gfx::ElideText
 - cocoa/history_menu_bridge.mm for history in the mainMenu [url titles]
   * gfx::ElideText
 - cocoa/menu_controller.mm which, e.g., cocoa/bookmarks/bookmark_menu_cocoa_controller.mm uses for the mainMenu bookmarks [url titles]
   * gfx::ElideText
 - ui/message_center/cocoa/notification_controller.mm - Cocoa notifications
   * url_formatter::ElideHost(..), gfx::GetStringWidth

And a bug:
 - ui/toolbar/back_forward_menu_model.cc - this should use BROWSER since the back button context menu is not native


So we need implementations of ElideFilename ElideText ElideHost and GetStringWidth that work with NSAttributedString directly.

There's some stuff used for ios in NSString+CrStringDrawing.mm. I'm not sure it will be much help thoguh.

ElideHost and ElideFilename might be particularly annoying :/. The rest... I'm thinking we'd need a NSAttributedString with a NSParagraphStyle with NSLineBreakMode set to NSLineBreakByTruncating{Head,Tail,Middle}. 

Or -[NSString boundingRectWithSize:options:attributes:context:]

But that only gets the dimensions of the string. I don't see an obvious way to get the resulting text (e.g. with elipsis included).

I think instead we need to convince the code relying on gfx::Typesetter::NATIVE to "not care" about the result at all, and just feed through the desired width to Cocoa and let it deal with the full string.
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a5abaaad7eb9693282982a3aadd5bd63357d032

commit 5a5abaaad7eb9693282982a3aadd5bd63357d032
Author: Trent Apted <tapted@chromium.org>
Date: Thu Sep 27 15:34:46 2018

Remove [gfx] switches::kEnableHarfBuzzRenderText.

It's effectively the default. Only a few places using
gfx::Typesetter::NATIVE explicitly should be using RenderTextMac now.

Bug:  798944 , 889152
Change-Id: I3318676b14b938607de9413130627dc1581b2289
Reviewed-on: https://chromium-review.googlesource.com/1248263
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594732}
[modify] https://crrev.com/5a5abaaad7eb9693282982a3aadd5bd63357d032/chrome/browser/about_flags.cc
[modify] https://crrev.com/5a5abaaad7eb9693282982a3aadd5bd63357d032/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/5a5abaaad7eb9693282982a3aadd5bd63357d032/ui/gfx/render_text.cc
[modify] https://crrev.com/5a5abaaad7eb9693282982a3aadd5bd63357d032/ui/gfx/switches.cc
[modify] https://crrev.com/5a5abaaad7eb9693282982a3aadd5bd63357d032/ui/gfx/switches.h
[modify] https://crrev.com/5a5abaaad7eb9693282982a3aadd5bd63357d032/ui/views/controls/label_unittest.cc

Owner: tapted@chromium.org
Status: Assigned (was: Untriaged)
Project Member

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

Labels: Hotlist-DesktopUIValid
Status: WontFix (was: Assigned)
** Mass UI Triage **

Since Fix is landed, Closing this bug as a part of UI mass triage.If you feel this issue should still be addressed, feel free to reopen it or to file a new issue.
Status: Available (was: WontFix)
The issue isn't fixed yet, the CL above was a partial step towards it.
Labels: Hotlist-DesktopUIChecked
Adding appropriate label for UI mass triage.

Sign in to add a comment