New issue
Advanced search Search tips

Issue 799300 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug
Team-Security-UX

Blocked on:
issue 798927



Sign in to add a comment

Incorrect text wrapping in Cocoa PageInfo

Project Member Reported by elawrence@chromium.org, Jan 4 2018

Issue description

Chrome 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?
 
TextWrapping.png
95.1 KB View Download
Cc: tapted@chromium.org
Components: Internals>Views>Desktop
Labels: Proj-MacViews
Labels: -Pri-3 M-65 Pri-2
Status: Available (was: Untriaged)
Blockedon: 798927
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)
Screen Shot 2018-01-08 at 9.29.27 am.png
53.4 KB View Download
Owner: tapted@chromium.org
Status: Assigned (was: Available)
Project Member

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

Comment 6 by tapted@chromium.org, 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!
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.

Comment 8 by meh...@chromium.org, 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.
Version 65.0.3319.0.mov
2.0 MB Download

Comment 9 by tapted@chromium.org, Jan 15 2018

Cc: wutao@chromium.org msw@chromium.org
Components: UI>GFX
Status: Started (was: Assigned)
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
Project Member

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

Status: Fixed (was: Started)
Project Member

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

Project Member

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