RenderTextMac can likely be removed now |
|||||
Issue descriptionRenderTextMac 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.
,
Sep 27
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.
,
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
,
Oct 1
,
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
,
Nov 16
** 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.
,
Nov 16
The issue isn't fixed yet, the CL above was a partial step towards it.
,
Nov 16
Adding appropriate label for UI mass triage. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by meh...@chromium.org
, Sep 26