New issue
Advanced search Search tips

Issue 681857 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 569159



Sign in to add a comment

underline-position: under is too low when the primary font is Meiryo

Project Member Reported by kojii@chromium.org, Jan 17 2017

Issue description

http://output.jsbin.com/qajilo

The 3rd line has "font-family: Meiryo"
 
underline-position-under-meiryo-blink.png
2.2 KB View Download
underline-position-under-meiryo-gecko.png
2.4 KB View Download
underline-position-under-meiryo-edge.png
2.1 KB View Download

Comment 1 by kojii@chromium.org, Jan 21 2017

Cc: -kojii@chromium.org bunge...@chromium.org
Owner: kojii@chromium.org
Status: Assigned (was: Available)
It turns out that we need cap-height to fix this, but it looks like Skia returns cap-height only on Mac, or internal to PDF.

This one line change seems to be working:
https://skia-review.googlesource.com/c/7380/
but not familiar with changes to Skia. drott@, bungeman@, could you advise?

Note, Noto may be in the same situation, in that case, we need Linux/Android, but I need more investigation to determine if that's the case.
Why do we need cap-height for this? It can be added, but note that in many fonts it is wrong. This was the primary reason we also don't use the underline position and thickness specified by the font. We did for a short bit, but it turned out that most fonts have poor values or it causes difficult to resolve issues when fonts are mixed on a single line.
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 23 2017

The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/a6725fcb14f63734d7668bb0550cd9c128e841b0

commit a6725fcb14f63734d7668bb0550cd9c128e841b0
Author: Koji Ishii <kojii@chromium.org>
Date: Sat Jan 21 17:30:51 2017

Support fCapHeight in SkScalerContext_DW

BUG= chromium:681857 

Change-Id: Ia97354d7798eb685351a98ff559bcf176a67af18
Reviewed-on: https://skia-review.googlesource.com/7380
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>

[modify] https://crrev.com/a6725fcb14f63734d7668bb0550cd9c128e841b0/src/ports/SkScalerContext_win_dw.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39f70de82dc0904c8a40f910feaa0b53409e7b87

commit 39f70de82dc0904c8a40f910feaa0b53409e7b87
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Mon Jan 23 19:55:22 2017

Roll src/third_party/skia/ 5b92e4ab8..a6725fcb1 (3 commits).

https://skia.googlesource.com/skia.git/+log/5b92e4ab8e1d..a6725fcb14f6

$ git log 5b92e4ab8..a6725fcb1 --date=short --no-merges --format='%ad %ae %s'
2017-01-22 kojii Support fCapHeight in SkScalerContext_DW
2017-01-23 reed change clip-bounds getters to always return the rect (actually fixes undefined result in getClipBounds)
2017-01-20 herb Remove public APIs that use SkDataTable.

BUG= 681857 

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=ethannicholas@google.com

Review-Url: https://codereview.chromium.org/2647373002
Cr-Commit-Position: refs/heads/master@{#445445}

[modify] https://crrev.com/39f70de82dc0904c8a40f910feaa0b53409e7b87/DEPS

Comment 5 by kojii@chromium.org, Jan 24 2017

Ben, thank you for landing the change in Skia!

> Why do we need cap-height for this? It can be added, but note that in many fonts it is wrong.

Yeah, I checked some fonts and found the value is incorrect in many cases. I'm experimenting a WIP, it looks like it's usable for CJK underline purposes if I add some heuristics. See "SimpleFontData.cpp" in
https://codereview.chromium.org/2643413002
Project Member

Comment 6 by bugdroid1@chromium.org, May 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/103954da11bdf022c300c4620d39b3b38ef15baa

commit 103954da11bdf022c300c4620d39b3b38ef15baa
Author: kojii <kojii@chromium.org>
Date: Tue May 02 23:39:47 2017

Fix 'text-underline-position: under' to use em height ascent/descent

This patch changes underline position when 'text-underline-position:
under' is set to the "em height ascent/descent".

Due to large internal leadings in many modern East Asian fonts,
underlines are often drawn too far. This patch changes it to match to
Gecko.

The spec[1] defines to draw under the text content area, which CSS2
leaves explicitly undefined[2].

[1] https://drafts.csswg.org/css-text-decor-3/#underline-under
[2] https://drafts.csswg.org/css2/visudet.html#leading

BUG= 681857 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2643413002
Cr-Commit-Position: refs/heads/master@{#468821}

[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/android/fast/css3-text/css3-text-decoration/text-decoration-skip-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/linux/fast/block/positioning/auto/vertical-lr/007-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/linux/fast/block/positioning/auto/vertical-rl/007-expected.png
[delete] https://crrev.com/ba2e08f19f4a5967a1eb9c06edc7938c89144063/third_party/WebKit/LayoutTests/platform/linux/fast/css3-text/css3-text-decoration/text-decoration-skip-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/linux/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png
[add] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/linux/virtual/mojo-loading/fast/table/border-collapsing/003-vertical-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/text/decorations-with-text-combine-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/mac/fast/block/positioning/auto/vertical-lr/007-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/mac/fast/block/positioning/auto/vertical-rl/007-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/mac/fast/text/decorations-with-text-combine-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/win/fast/block/positioning/auto/vertical-lr/007-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/win/fast/block/positioning/auto/vertical-rl/007-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/win/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png
[add] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/win/virtual/mojo-loading/fast/table/border-collapsing/003-vertical-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/LayoutTests/platform/win7/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/Source/core/layout/line/InlineBox.h
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/Source/core/layout/line/InlineFlowBox.h
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/Source/core/layout/line/InlineTextBox.h
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp
[modify] https://crrev.com/103954da11bdf022c300c4620d39b3b38ef15baa/third_party/WebKit/Source/platform/fonts/SimpleFontData.h

Comment 7 by kojii@chromium.org, May 15 2017

Status: Fixed (was: Assigned)

Sign in to add a comment