Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(111)

Issue 2643413002: Fix 'text-underline-position: under' to use em height ascent/descent (Closed)

Created:
3 years, 11 months ago by kojii
Modified:
3 years, 7 months ago
CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, f(malita), jbroman, jchaffraix+rendering, Justin Novosad, kinuko+watch, leviw+renderwatch, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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} Committed: https://chromium.googlesource.com/chromium/src/+/103954da11bdf022c300c4620d39b3b38ef15baa

Patch Set 1 #

Patch Set 2 : WIP #

Patch Set 3 : #

Patch Set 4 : WIP #

Patch Set 5 : WIP, change to em height #

Patch Set 6 : WIP, more on em height #

Patch Set 7 : WIP, with PaintFontMetrics #

Patch Set 8 : WIP, adjust gap #

Patch Set 9 : WIP #

Patch Set 10 : WIP #

Patch Set 11 : WIP #

Patch Set 12 : WIP #

Patch Set 13 : WIP #

Patch Set 14 : WIP #

Patch Set 15 : Cleanup #

Patch Set 16 : Rebaseline #

Total comments: 9

Patch Set 17 : eae review #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+267 lines, -99 lines) Patch
M third_party/WebKit/LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/android/fast/css3-text/css3-text-decoration/text-decoration-skip-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/fast/block/positioning/auto/vertical-lr/007-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/fast/block/positioning/auto/vertical-rl/007-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
D third_party/WebKit/LayoutTests/platform/linux/fast/css3-text/css3-text-decoration/text-decoration-skip-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/platform/linux/virtual/mojo-loading/fast/table/border-collapsing/003-vertical-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/text/decorations-with-text-combine-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/block/positioning/auto/vertical-lr/007-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/block/positioning/auto/vertical-rl/007-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/text/decorations-with-text-combine-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/block/positioning/auto/vertical-lr/007-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/block/positioning/auto/vertical-rl/007-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/platform/win/virtual/mojo-loading/fast/table/border-collapsing/003-vertical-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win7/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Binary file 0 comments Download
M third_party/WebKit/Source/core/layout/line/InlineBox.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +21 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/line/InlineFlowBox.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +16 lines, -36 lines 0 comments Download
M third_party/WebKit/Source/core/layout/line/InlineTextBox.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +38 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 12 chunks +89 lines, -59 lines 0 comments Download
M third_party/WebKit/Source/platform/fonts/SimpleFontData.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +11 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +85 lines, -0 lines 5 comments Download

Messages

Total messages: 38 (28 generated)
kojii
PTAL.
3 years, 7 months ago (2017-05-01 13:04:20 UTC) #9
eae
Thank you for working on this! Overall this looks great but I do have a ...
3 years, 7 months ago (2017-05-01 16:21:48 UTC) #10
kojii
Thank you, it was greatly helpful. All done, PTAL. https://codereview.chromium.org/2643413002/diff/320001/third_party/WebKit/Source/core/layout/line/InlineBox.h File third_party/WebKit/Source/core/layout/line/InlineBox.h (right): https://codereview.chromium.org/2643413002/diff/320001/third_party/WebKit/Source/core/layout/line/InlineBox.h#newcode55 third_party/WebKit/Source/core/layout/line/InlineBox.h:55: ...
3 years, 7 months ago (2017-05-02 22:56:53 UTC) #27
eae
LGTM
3 years, 7 months ago (2017-05-02 22:58:35 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2643413002/440001
3 years, 7 months ago (2017-05-02 23:34:32 UTC) #30
commit-bot: I haz the power
Committed patchset #17 (id:440001) as https://chromium.googlesource.com/chromium/src/+/103954da11bdf022c300c4620d39b3b38ef15baa
3 years, 7 months ago (2017-05-02 23:40:11 UTC) #33
bungeman-chromium
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right): https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp#newcode417 third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:417: // TODO(kojii): This should move to Skia once finalized. ...
3 years, 7 months ago (2017-05-03 13:56:37 UTC) #35
kojii
On 2017/05/03 at 13:56:37, bungeman wrote: > https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp > File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right): > > https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp#newcode417 ...
3 years, 7 months ago (2017-05-03 18:07:31 UTC) #36
drott
Thanks, Koji, LGTM as discussed in Tokyo, thanks for addressing this issue. Sorry I missed ...
3 years, 7 months ago (2017-05-08 08:04:00 UTC) #37
kojii
3 years, 7 months ago (2017-05-09 04:37:16 UTC) #38
Message was sent while issue was closed.
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:420: size_t size =
typeface->getTableData(SkSetFourByteTag('O', 'S', '/', '2'), 68,
On 2017/05/08 at 08:04:00, drott wrote:
> What's 68 here? Could you make a constant for this?

It's the offset. Skia has access to os2table so they don't need it but we need
to hard code until this is moved to Skia. Should have commented, thank you
pointing out.

https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:458: // This matches
to how Gecko computes "em height":
On 2017/05/08 at 08:04:00, drott wrote:
> Would be great if you had a Mozilla DXR link to where it happens in Gecko.

Yeah, not great at reading Mozilla code yet...

Powered by Google App Engine
This is Rietveld 408576698