Closed Bug 1573249 Opened 2 years ago Closed 2 years ago

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

Categories

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

70 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: myakura.web, Assigned: jfkthame)

References

Details

Attachments

(5 files)

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.

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

Blocks: 1572800
Component: Untriaged → Layout: Text and Fonts
Product: Firefox → Core

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.

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.

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true

(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

Duplicate of this bug: 1574811

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.

Priority: -- → P3
Duplicate of this bug: 1575262
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #9086430 - Attachment description: Bug 1573249 - patch 1 - (preparatory patch, no functional change) Clean up some management of the GlyphRun array in gfxTextRun. r=dholbert → Bug 1573249 - patch 1 - Clean up some management of the GlyphRun array in gfxTextRun. r=dholbert
Pushed by jkew@mozilla.com:
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

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

Flags: needinfo?(jfkthame)
Flags: needinfo?(jfkthame)
Pushed by jkew@mozilla.com:
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

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

Flags: needinfo?(jfkthame)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b03a028f23a
followup, clang-format nits. r=dholbert
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.