Issue metadata
Sign in to add a comment
|
Incorrect text wrapping in Cocoa PageInfo |
||||||||||||||||||||||||
Issue descriptionChrome Version: 65.3310 OS: OS X What steps will reproduce the problem? (1) Visit https://testsafebrowsing.appspot.com/s/phishing.html (2) Click Page Info button Expect: Word wrapping Actual: Character-level wrapping on Learn More link Explicitly disabling chrome://flags#secondary-ui-md causes this to go away, so maybe this is a MacViews thing?
,
Jan 7 2018
,
Jan 7 2018
What version of MacOS are you on? This looks OK for me on 10.12.6. I suspect this is related to Issue 793184 and Issue 798927 . I'm working on a fix. It's complicated (first WIP is http://crrev.com/c/848695)
,
Jan 7 2018
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aa6cafdb584215e0158560cadec23b56a1751925 commit aa6cafdb584215e0158560cadec23b56a1751925 Author: Trent Apted <tapted@chromium.org> Date: Wed Jan 10 23:42:01 2018 Make gfx::ElideRectangleText() and url_formatter::ElideHost() typesetter-aware. gfx::ElideRectangleText() is used on Mac both for native (system- driven) UI and for Chrome secondary UI. The former is used for system notifications and will be typeset by CoreText. The latter is typeset by Harfbuzz. This CL introduces gfx::ElideRectangleTextForNativeUi(), whose only consumer at this stage is the Mac message center, which pops native system notifications. Nothing else but MacViews was relying on gfx::ElideRectangleText() on Mac, so switch it over to Harfbuzz. There *is* one other caller: ExtensionIconPlaceholder::Draw(), which draws a single letter to a canvas with DrawStringRectWithFlags(). gfx::Canvas::DrawStringRectWithFlags() may call ElideRectangleText() in some codepaths, so switch DrawStringRectWithFlags() over to Harfbuzz as well so it's consistent. (In practice, the single letter would could never elide, so this doesn't change much). Ensure the rest of cocoa/notification_controller.mm is using the correct typesetter. This requires url_formatter::ElideHost() to also be typesetter-aware. TBR=dtrainor@chromium.org Bug: 793184 , 798927 , 799300 Change-Id: Ibccb740f97a833c9701b25037d69b0369f732028 Reviewed-on: https://chromium-review.googlesource.com/856016 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#528483} [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/chrome/browser/download/download_item_model.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/chrome/browser/ui/cocoa/download/download_item_cell.mm [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/chrome/browser/ui/cocoa/download/md_download_item_view.mm [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/components/url_formatter/elide_url.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/components/url_formatter/elide_url.h [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/canvas_skia.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/render_text.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/render_text.h [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/text_constants.h [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/text_elider.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/text_elider.h [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/message_center/cocoa/notification_controller.mm [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/views/controls/styled_label_unittest.cc
,
Jan 12 2018
elawrence: is this fixed for you in Canary 65.0.3318.0 or later? I could never repro it (maybe OS version, language, or some other setting) but I'm guessing r528483 will fix. Thanks!
,
Jan 12 2018
Re #6: This looks fine now in 65.3318, although I was forced to update to MacOS High Sierra in the interim, so I no longer have the original 10.12 repro environment.
,
Jan 12 2018
Hi, I am still seeing this on macOS 10.12.6 - Chrome Canary Version 65.0.3319.0. A screencast is attached.
,
Jan 15 2018
OK - I managed to repro. I had to switch my language from UK-English to US-English. The words are the same, but there are some extra commas: UK-English has "(for example passwords, phone numbers or credit cards)." US-English has "(for example, passwords, phone numbers, or credit cards)." I think the problem is that the x-offset of the "Learn" must be rounded up to the nearest integer, but the typesetter doesn't know this when it splits the text. Issue 799213 documents some related things, but this seems to be a special corner case we should actually deal with. I have a fix -> https://chromium-review.googlesource.com/866277
,
Jan 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/929a7aaea3d2d340ec56053cb0dc6e7eabe8ab8a commit 929a7aaea3d2d340ec56053cb0dc6e7eabe8ab8a Author: Trent Apted <tapted@chromium.org> Date: Wed Jan 17 06:01:04 2018 Fix a corner case of text wrapping in views::StyledLabel Views are aligned to integer coordinates, but typesetting is not. If part of a line has a stylerange, it may need to shift up to a pixel to the right, compared to an earlier typesetting attempt. Normally this is fine - more text will just wrap to the next line. However, if this occurs for the *last* word on a line, gfx::ElideRectangleText may split that word. Instead, that word should just wrap on to the next line. To fix, allow ElideRectangleText to signal when it splits the first word in an elide attempt. StyledLabel just needs to start a new line in this case rather than putting the split word at the end of the current line. Bug: 799300 , 799213 Change-Id: Ibcc6134872b06c56d19bad5f7336534189fa316a Reviewed-on: https://chromium-review.googlesource.com/866277 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#529646} [modify] https://crrev.com/929a7aaea3d2d340ec56053cb0dc6e7eabe8ab8a/ui/gfx/text_elider.cc [modify] https://crrev.com/929a7aaea3d2d340ec56053cb0dc6e7eabe8ab8a/ui/gfx/text_elider.h [modify] https://crrev.com/929a7aaea3d2d340ec56053cb0dc6e7eabe8ab8a/ui/gfx/text_elider_unittest.cc [modify] https://crrev.com/929a7aaea3d2d340ec56053cb0dc6e7eabe8ab8a/ui/views/controls/styled_label.cc
,
Jan 17 2018
,
Jan 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db87bb2a48e652bf8db4a6c14b523e9ee055e1c4 commit db87bb2a48e652bf8db4a6c14b523e9ee055e1c4 Author: Stephen McGruer <smcgruer@chromium.org> Date: Wed Jan 17 14:32:29 2018 Revert "Fix a corner case of text wrapping in views::StyledLabel" This reverts commit 929a7aaea3d2d340ec56053cb0dc6e7eabe8ab8a. Reason for revert: TextEliderTest.ElideRectangleTextFirstWordTruncated fails on chromium.mac/Mac10.10 Tests: https://ci.chromium.org/buildbot/chromium.mac/Mac10.10%20Tests/28182 Original change's description: > Fix a corner case of text wrapping in views::StyledLabel > > Views are aligned to integer coordinates, but typesetting is not. If > part of a line has a stylerange, it may need to shift up to a pixel to > the right, compared to an earlier typesetting attempt. > > Normally this is fine - more text will just wrap to the next line. > However, if this occurs for the *last* word on a line, > gfx::ElideRectangleText may split that word. Instead, that word should > just wrap on to the next line. > > To fix, allow ElideRectangleText to signal when it splits the first > word in an elide attempt. StyledLabel just needs to start a new line > in this case rather than putting the split word at the end of the > current line. > > Bug: 799300 , 799213 > Change-Id: Ibcc6134872b06c56d19bad5f7336534189fa316a > Reviewed-on: https://chromium-review.googlesource.com/866277 > Reviewed-by: Michael Wasserman <msw@chromium.org> > Commit-Queue: Trent Apted <tapted@chromium.org> > Cr-Commit-Position: refs/heads/master@{#529646} TBR=msw@chromium.org,tapted@chromium.org Change-Id: Ib518babd4bf891418789951186a8bcd7442bbbb2 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 799300 , 799213 Reviewed-on: https://chromium-review.googlesource.com/870611 Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#529736} [modify] https://crrev.com/db87bb2a48e652bf8db4a6c14b523e9ee055e1c4/ui/gfx/text_elider.cc [modify] https://crrev.com/db87bb2a48e652bf8db4a6c14b523e9ee055e1c4/ui/gfx/text_elider.h [modify] https://crrev.com/db87bb2a48e652bf8db4a6c14b523e9ee055e1c4/ui/gfx/text_elider_unittest.cc [modify] https://crrev.com/db87bb2a48e652bf8db4a6c14b523e9ee055e1c4/ui/views/controls/styled_label.cc
,
Jan 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c23f470dd9cb43a5726a28f8ccff9aa252b634d7 commit c23f470dd9cb43a5726a28f8ccff9aa252b634d7 Author: Trent Apted <tapted@chromium.org> Date: Thu Jan 18 04:28:59 2018 Reland "Fix a corner case of text wrapping in views::StyledLabel" This is a reland of 929a7aaea3d2d340ec56053cb0dc6e7eabe8ab8a Issues specific to 10.10 will be fixed by http://crrev.com/c/869630 Original change's description: > Fix a corner case of text wrapping in views::StyledLabel > > Views are aligned to integer coordinates, but typesetting is not. If > part of a line has a stylerange, it may need to shift up to a pixel to > the right, compared to an earlier typesetting attempt. > > Normally this is fine - more text will just wrap to the next line. > However, if this occurs for the *last* word on a line, > gfx::ElideRectangleText may split that word. Instead, that word should > just wrap on to the next line. > > To fix, allow ElideRectangleText to signal when it splits the first > word in an elide attempt. StyledLabel just needs to start a new line > in this case rather than putting the split word at the end of the > current line. > > Bug: 799300 , 799213 > Change-Id: Ibcc6134872b06c56d19bad5f7336534189fa316a > Reviewed-on: https://chromium-review.googlesource.com/866277 > Reviewed-by: Michael Wasserman <msw@chromium.org> > Commit-Queue: Trent Apted <tapted@chromium.org> > Cr-Commit-Position: refs/heads/master@{#529646} TBR=tapted@chromium.org Bug: 799300 , 799213 Change-Id: I751e67c82d4637204b0d489b1b4d4f8e759c0de6 Reviewed-on: https://chromium-review.googlesource.com/872350 Reviewed-by: Trent Apted <tapted@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#530054} [modify] https://crrev.com/c23f470dd9cb43a5726a28f8ccff9aa252b634d7/ui/gfx/text_elider.cc [modify] https://crrev.com/c23f470dd9cb43a5726a28f8ccff9aa252b634d7/ui/gfx/text_elider.h [modify] https://crrev.com/c23f470dd9cb43a5726a28f8ccff9aa252b634d7/ui/gfx/text_elider_unittest.cc [modify] https://crrev.com/c23f470dd9cb43a5726a28f8ccff9aa252b634d7/ui/views/controls/styled_label.cc |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by meh...@chromium.org
, Jan 5 2018Components: Internals>Views>Desktop
Labels: Proj-MacViews