Project: chromium ▼ Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 338148 Chrome should use font metrics to calculate underline thickness
Starred by 11 users Project Member Reported by h.jo...@samsung.com, Jan 26, 2014 Back to list
Status: Assigned
Owner: h.jo...@samsung.com
Cc: e...@chromium.org, drott@chromium.org
Components:
OS: Mac
Pri: 2
Type: Bug


Sign in to add a comment
 
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 bugdro...@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 bugdro...@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
Status: Assigned
Summary: Chrome should use font metrics to calculate underline thickness (was: Chrome using font metrics to calculate underline thickness)
Owner: h.jo...@samsung.com
Cc: e...@chromium.org drott@chromium.org
Labels: Cr-Blink-Fonts
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
Comment 13 by lafo...@chromium.org, Sep 23, 2015
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
Comment 14 by dtapu...@chromium.org, Dec 10, 2015
Labels: -Cr-Blink
Removing Cr-Blink from issues that already have Cr-Blink sub-label set.
► Sign in to add a comment