1573249 - text-decoration-skip-ink doesn't look good on Japanese (Chinese) sentences

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Masataka Yakura]

Attachment: Screen Shot 2019-08-13 at 3.46.59.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

1. enable layout.css.text-decoration-skip-ink.enabled
2. go to https://www.jma.go.jp/jma/menu/menuflash.html
3. look at the links

Actual results:

underlines are chopped by certain Kanji characters

Expected results:

text-decoration-skip-ink should not ink-skip on Han (+Kana) sequences.

Comment 1

[Masataka Yakura]

yet there's no spec-wise resolutions [1], WebKit [2] and Blink [3][4] do not apply the ink-skipping effect on CJK scripts even auto is specified.

[1] https://github.com/w3c/csswg-drafts/issues/707
[2] https://bugs.webkit.org/show_bug.cgi?id=128145
[3] https://crbug.com/677093
[4] https://crbug.com/793762

Comment 2

[Jonathan Kew [:jfkthame]]

In general, it would probably be good for CJK content to use text-underline-offset to explicitly set the underline at an appropriate level so it stays clear of the glyphs. In many fonts, the default position seems to touch the bottom of the Chinese characters, which also doesn't look very good.

We could consider adding rules to the UA stylesheet to set text-decoration-skip-ink:none when lang(zh), lang(ja), etc is present, although that wouldn't help with content that uses CJK characters but doesn't have an appropriate lang attribute.

We could also do something hard-coded based on the Unicode block or script of the characters, though I'm kinda reluctant to do that arbitrarily without any clear spec or WG consensus.

Comment 3

[Cameron McCormack (:heycam)]

Looking at the Blink changesets from the bugs linked above, they are indeed doing it based on Unicode codepoint. They check Character::IsCJKIdeographOrSymbol (not sure exactly what that corresponds to) and Unicode block name. I feel like it would be OK to do the same thing for now until we get WG consensus on how to handle these scripts. Doing something different based on HTML language feels like it would risk compat issues.

Comment 4

[Jonathan Kew [:jfkthame]]

IsCJKIdeographOrSymbol() recognizes the "obvious" CJK ideograph blocks, but also returns true for a rather arbitrary-looking (and large) collection of symbols, many of which have no strong connection with CJK; see codepoints listed in kIsCJKIdeographOrSymbolArray at https://chromium.googlesource.com/chromium/src.git/+/9f802345bd20185c79cbec975e0d19a4bf813411/third_party/WebKit/Source/platform/text/CharacterPropertyDataGenerator.h.

(Why are dagger '†' and double-dagger '‡' in the IsCJKIdeographOrSymbol list? They're widely used in English text, e.g. as footnote markers; why wouldn't skip-ink want to ski them? Why is per-mille sign '‰' there, yet not percent '%' or per-tenthousand '‱'? Etc.)

Indeed, trying an example like

data:text/html;charset=utf-8,<h1><u>Does ink skip dagger '%E2%80%A0' and double-dagger '%E2%80%A1' or not

I see that in Safari, the underline does skip the daggers (as I'd expect), while in Chrome it doesn't.

I don't think Blink's arbitrary list of symbols here makes much sense.

Comment 5

[Masataka Yakura]

(In reply to Jonathan Kew (:jfkthame) from comment #4)

(Why are dagger '†' and double-dagger '‡' in the IsCJKIdeographOrSymbol list? They're widely used in English text, e.g. as footnote markers; why wouldn't skip-ink want to ski them?

The function is not just only be used in the context of ink skipping. Going thorough commit logs U+2020 (dagger) was first added to WebKit so it "rotates properly" in vertical writing mode" [1] (not sure what "properly" means... upright?).

Why is per-mille sign '‰' there, yet not percent '%' or per-tenthousand '‱'? Etc.)

Not sure either. But given those codepoints have been added by Apple and been kept since then, perhaps Blink folks might revisit the list and tweak them to make sense?

[1] https://trac.webkit.org/changeset/138986/webkit

Comment 7

[Jonathan Kew [:jfkthame]]

During textrun construction, we already do script run analysis and shape the text separately per script run (via gfxFontGroup::InitScriptRun), so at that point we know if a given range of the textrun is being shaped as a CJK script. So I think we could add a flag to the GlyphRun struct to record this, and then use that to avoid applying skip-ink to CJK glyph runs.

That should be cheaper than going back to the original text and looking up the script or unicode-block of each character again during skip-ink processing to determine whether it's a CJK character.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1573249 - patch 1 - Clean up some management of the GlyphRun array in gfxTextRun. r=dholbert

Depends on D42527

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1573249 - patch 2 - Don't apply skip-ink to runs of CJK text, because it looks bad with many fonts. r=dholbert

Depends on D42528

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1573249 - patch 3 - Add reftests for ignoring skip-ink behavior on CJK text. r=dholbert

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3576d5b9f2db
patch 1 - Clean up some management of the GlyphRun array in gfxTextRun. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/21306e3f8d31
patch 2 - Don't apply skip-ink to runs of CJK text, because it looks bad with many fonts. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/bdba80dae6e2
patch 3 - Add reftests for ignoring skip-ink behavior on CJK text. r=dholbert

Comment 13

[Cristina Coroiu [:ccoroiu]]

Backed out 3 changesets (bug 1573249) for chrome failures at layout/inspector/tests/chrome/test_fontFaceGeneric.xul

Backout: https://hg.mozilla.org/integration/autoland/rev/d7d41033f5615723ac638929850221c1d683d34c

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bdba80dae6e2e2ae62405f8d9a723d3a2834ef9e

Failure log; https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=262636561&repo=autoland&lineNumber=2276

task 2019-08-21T10:26:06.326Z] 10:26:06 INFO - TEST-PASS | layout/inspector/tests/chrome/test_fontFaceGeneric.xul | one font found
[task 2019-08-21T10:26:06.327Z] 10:26:06 INFO - Buffered messages finished
[task 2019-08-21T10:26:06.327Z] 10:26:06 INFO - TEST-UNEXPECTED-FAIL | layout/inspector/tests/chrome/test_fontFaceGeneric.xul | font DejaVu Serif was specified as CSS generic - got "", expected "serif"
[task 2019-08-21T10:26:06.327Z] 10:26:06 INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:320:16
[task 2019-08-21T10:26:06.327Z] 10:26:06 INFO - RunTest@chrome://mochitests/content/chrome/layout/inspector/tests/chrome/test_fontFaceGeneric.xul:21:5
[task 2019-08-21T10:26:06.327Z] 10:26:06 INFO - onload@chrome://mochitests/content/chrome/layout/inspector/tests/chrome/test_fontFaceGeneric.xul:1:8
[task 2019-08-21T10:26:06.327Z] 10:26:06 INFO - rval@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:146:30
[task 2019-08-21T10:26:06.329Z] 10:26:06 INFO - TEST-PASS | layout/inspector/tests/chrome/test_fontFaceGeneric.xul | one font found
[task 2019-08-21T10:26:06.329Z] 10:26:06 INFO - TEST-PASS | layout/inspector/tests/chrome/test_fontFaceGeneric.xul | used the expected family (DejaVu Serif)
[task 2019-08-21T10:26:06.329Z] 10:26:06 INFO - TEST-PASS | layout/inspector/tests/chrome/test_fontFaceGeneric.xul | font DejaVu Serif was specified by name
[task 2019-08-21T10:26:06.331Z] 10:26:06 INFO - TEST-PASS | layout/inspector/tests/chrome/test_fontFaceGeneric.xul | two fonts found
[task 2019-08-21T10:26:06.331Z] 10:26:06 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-08-21T10:26:06.331Z] 10:26:06 INFO - TEST-UNEXPECTED-FAIL | layout/inspector/tests/chrome/test_fontFaceGeneric.xul | monospace font DejaVu Sans Mono was specified as generic - got "", expected "monospace"
[task 2019-08-21T10:26:06.331Z] 10:26:06 INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:320:16
[task 2019-08-21T10:26:06.332Z] 10:26:06 INFO - RunTest/<@chrome://mochitests/content/chrome/layout/inspector/tests/chrome/test_fontFaceGeneric.xul:43:9
[task 2019-08-21T10:26:06.332Z] 10:26:06 INFO - RunTest@chrome://mochitests/content/chrome/layout/inspector/tests/chrome/test_fontFaceGeneric.xul:38:9
[task 2019-08-21T10:26:06.332Z] 10:26:06 INFO - onload@chrome://mochitests/content/chrome/layout/inspector/tests/chrome/test_fontFaceGeneric.xul:1:8
[task 2019-08-21T10:26:06.333Z] 10:26:06 INFO - rval@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:146:30

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab9106acfa78
patch 1 - Clean up some management of the GlyphRun array in gfxTextRun. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d895db733402
patch 2 - Don't apply skip-ink to runs of CJK text, because it looks bad with many fonts. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/7cb671e5543f
patch 3 - Add reftests for ignoring skip-ink behavior on CJK text. r=dholbert

Comment 15

[Daniel Holbert [:dholbert]]

Please land a followup to address the reviewbot clang-format nits[1] otherwise the next person to touch gfxTextRun.cpp may get them lumped into their patch as part of autoformatting.

[1] https://phabricator.services.mozilla.com/D42529#1295675

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1573249 followup, clang-format nits. r=dholbert

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ab9106acfa78
https://hg.mozilla.org/mozilla-central/rev/d895db733402
https://hg.mozilla.org/mozilla-central/rev/7cb671e5543f

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you want to propose spec changes either in https://github.com/w3c/csswg-drafts/issues/707 or in a separate issue?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b03a028f23a
followup, clang-format nits. r=dholbert

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5b03a028f23a