New issue
Advanced search Search tips

Issue 338148 link

Starred by 12 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Feature



Sign in to add a comment

Chrome should use font metrics to calculate underline thickness

Reported by h.jo...@samsung.com, Jan 26 2014

Issue description

UserAgent: Mozilla/5.0 (iPhone; CPU iPhone OS 7_0_4 like Mac OS X) AppleWebKit/537.51.1 (KHTML, like Gecko) CriOS/31.0.1650.18 Mobile/11B554a Safari/8536.25 (5616E139-056E-4A75-83F0-9352805BD2B0)

Example URL:

Steps to reproduce the problem:
1. Open any site with underline features
2. Observed chrome apply font size based underline thickness
3. 

What is the expected behavior?
Chrome should use font data to calculate correct underline thickness in case of faulty font metrics it should fallback on font size dependent calculation.

What went wrong?
To draw underline currently Chrome is depending on Font Size taking 10% 
Font Size as underline thickness which is not correct.

Does it occur on multiple sites: Yes

Is it a problem with a plugin? N/A 

Did this work before? No 

Does this work in other browsers? N/A 

Chrome version: <Copy from: 'about:version'>  Channel: n/a
OS Version: 
Flash Version:
 

Comment 1 by h.jo...@samsung.com, Jan 27 2014

I am working on this bug and code for review is submitted
https://codereview.chromium.org/147703002/

 Patches include change in Blink and Skia

Comment 2 by h.jo...@samsung.com, Jan 28 2014

Blink and Skia git repo are different so added two more patches related to Skia changes only.
https://codereview.chromium.org/148453005/
https://codereview.chromium.org/137543008/
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 11 2014

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=166890

------------------------------------------------------------------------
r166890 | h.joshi@samsung.com | 2014-02-11T03:54:48.681999Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/css/line-thickness-underline-strikethrough-overline-expected.png?r1=166890&r2=166889&pathrev=166890
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/fonts/FontMetrics.h?r1=166890&r2=166889&pathrev=166890
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/fonts/mac/SimpleFontDataMac.mm?r1=166890&r2=166889&pathrev=166890
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/InlineTextBox.cpp?r1=166890&r2=166889&pathrev=166890
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/TestExpectations?r1=166890&r2=166889&pathrev=166890
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/fonts/skia/SimpleFontDataSkia.cpp?r1=166890&r2=166889&pathrev=166890

To draw underline currently Chrome is depending on Font Size taking 10%
Font Size as underline thickness which is not correct. Font properties
should be taken into account to calculate underline thickness. Following change does the same, some Skia files are also updated for this change.

Blink and Skia git repo are different so added two more patches related to Skia changes only.
https://codereview.chromium.org/148453005/
https://codereview.chromium.org/137543008/
 and made this patch Skia independent.

BUG= 338148 

Review URL: https://codereview.chromium.org/147703002
------------------------------------------------------------------------
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 12 2014

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=167037

------------------------------------------------------------------------
r167037 | ojan@chromium.org | 2014-02-12T20:35:58.382539Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/repaint/shadow-multiple-vertical-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/inserting/return-key-with-selection-001-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/repaint/shadow-multiple-vertical-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/tables/mozilla/bugs/bug128229-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/deleting/delete-4083333-fix-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/fast/dom/clone-node-dynamic-style-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/block/positioning/auto/vertical-lr/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/block/positioning/auto/vertical-rl/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-001-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/text/international/rtl-white-space-pre-wrap-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/css/clip-zooming-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/http/tests/misc/acid2-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-005-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/text/trailing-white-space-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/repaint/shadow-multiple-horizontal-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-009-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/block/positioning/auto/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/css/line-thickness-underline-strikethrough-overline-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/css/first-line-text-decoration-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/repaint/shadow-multiple-strict-horizontal-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/inserting/insert-div-022-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/inserting/insert-div-026-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/fast/block/float/centered-float-avoidance-complexity-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/inserting/return-key-with-selection-002-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/editing/inserting/insert-div-023-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/fast/block/positioning/auto/vertical-lr/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/svg/custom/svg-fonts-without-missing-glyph-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/text/stroking-decorations-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-010-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-002-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-006-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/text/decorations-with-text-combine-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-4038267-fix-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/fast/css/line-thickness-underline-strikethrough-overline-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/fast/block/positioning/auto/vertical-rl/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/css/acid2-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/block/float/centered-float-avoidance-complexity-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/inserting/insert-div-023-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/block/float/float-in-float-painting-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/fast/block/positioning/auto/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/style/style-3998892-fix-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/inserting/return-key-with-selection-003-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/text/stroking-decorations-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/repaint/shadow-multiple-strict-vertical-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/block/positioning/auto/vertical-rl/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/block/positioning/auto/vertical-lr/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-003-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/style/block-styles-007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/repaint/shadow-multiple-horizontal-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/block/float/float-in-float-hit-testing-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/block/positioning/auto/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/block/float/centered-float-avoidance-complexity-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/editing/inserting/insert-div-023-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/inserting/insert-div-024-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/text/line-breaks-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/dom/clone-node-dynamic-style-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/dom/clone-node-dynamic-style-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-decoration-style-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/fast/css/line-thickness-underline-strikethrough-overline-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/ietestcenter/css3/text/textshadow-002-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/fast/block/positioning/auto/vertical-rl/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/ietestcenter/css3/text/textshadow-002-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/repaint/shadow-multiple-strict-vertical-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-004-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/text/decorations-with-text-combine-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/TestExpectations?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/text/trailing-white-space-2-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/fast/block/positioning/auto/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-line-endings-008-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/fast/block/float/centered-float-avoidance-complexity-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/fast/block/positioning/auto/vertical-lr/007-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/editing/inserting/insert-div-023-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-lion/fast/repaint/shadow-multiple-strict-horizontal-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/fast/css/first-line-text-decoration-inherited-from-parent-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/fast/dom/clone-node-dynamic-style-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-match-style-002-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/style/smoosh-styles-003-expected.png?r1=167037&r2=167036&pathrev=167037
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/editing/pasteboard/paste-text-019-expected.png?r1=167037&r2=167036&pathrev=167037

Auto-rebaseline for r166890

http://src.chromium.org/viewvc/blink?view=revision&revision=166890

BUG= 338148 
TBR=h.joshi@samsung.com

Review URL: https://codereview.chromium.org/161163002
------------------------------------------------------------------------

Comment 5 by js...@chromium.org, Feb 27 2014

Does this issue also deal with using the underline position specified in a font?  CFF/PS fonts have the underline position as well as the underline thickness specified in post and CFF table. 

Comment 6 by h.jo...@samsung.com, Feb 27 2014

In this patch only Underline Thickness is added, need to work on Underline position. Changes for Underline Thickness and Position are added to Skia "https://codereview.chromium.org/152073003/". 
 
Will be making patch for other platforms and submit for review. 

Comment 7 by jia...@opera.com, Apr 21 2014

h.joshi, was this bug fixed already? Should the status be Unconfirmed?

Comment 8 by h.jo...@samsung.com, Apr 22 2014

Issue is fixed and code is committed. Let me check how to change bug status, I have no idea on how to make change to bug status.

Comment 9 by rsesek@chromium.org, Apr 28 2014

 Issue 313206  has been merged into this issue.

Comment 10 by kea...@keavon.com, Oct 27 2014

This still appears to be an issue with the current release of Chrome. What's the status?

Comment 11 by drott@chromium.org, Jun 25 2015

Cc: e...@chromium.org drott@chromium.org
Labels: Cr-Blink-Fonts
Owner: h.jo...@samsung.com
Status: Assigned
Summary: Chrome should use font metrics to calculate underline thickness (was: Chrome using font metrics to calculate underline thickness)
This issue is not fixed and as far as I can tell your patches h.joshi@ are somewhat in limbo - see eae's revert of your underline thickness changes in https://codereview.chromium.org/555883002. This means, currently setUnderlineThickness of FontMetrics.h is dead code at the moment, and we always fall back to the heuristic of calculating a thickness based on the font size. I would strongly suggest to find a sound and complete solution for this issue first, before looking at issue 386703. You brought this change in based on what you found in FF - which sounded promising to improve Blink's rendering.  Perhaps you can get a solution for the non-desirable cases/fonts as well, which were the reason eae@ reverted the change. Thanks.

Comment 12 by tkent@chromium.org, Jul 10 2015

Labels: -Cr-Content Cr-Blink
Labels: Hotlist-Recharge
This issue likely requires triage.  The current issue owner maybe inactive (i.e. hasn't fixed an issue in the last 30 days).  Thanks for helping out!

-Anthony
Labels: -Cr-Blink
Removing Cr-Blink from issues that already have Cr-Blink sub-label set.
Project Member

Comment 15 by sheriffbot@chromium.org, Jul 6 2016

Labels: Hotlist-OpenBugWithCL
A change has landed for this issue, but it's been open for over 6 months. Please review and close it if applicable. If this issue should remain open, remove the "Hotlist-OpenBugWithCL" label. If no action is taken, it will be archived in 30 days.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-OpenBugWithCL
Owner: ----
Status: Available (was: Assigned)
h.joshi@samsung.com does not seem to work on the project any longer.
Project Member

Comment 17 by sheriffbot@chromium.org, Jul 6 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 18 by e...@chromium.org, Jul 6 2017

Cc: kojii@chromium.org
Labels: -Type-Bug Type-Feature
Status: Available (was: Untriaged)
Owner: kojii@chromium.org
Status: Fixed (was: Available)
I believe we already do this.

ComputeDecorationThickness
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/TextPainterBase.cpp?type=cs&q=ComputeDecorationThickness&l=230

If anyone sees this not working properly yet, please leave comments here.

Sign in to add a comment