1582016 - "text-decoration-skip-ink" interacts terribly with Guardian TextEgyp font (used on e.g. Engadget)

Status: VERIFIED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot of article with barely any underlining visible under purple links (zoomed in 300%)

STR:

1. Load https://www.engadget.com/amp/2019/09/17/googles-fi-unlimited-plan/
2. (optional) Zoom in with Ctrl+
3. Look at underlined links in the article text.

ACTUAL RESULTS:
The underlines skip a lot, skipping over characters that don't have descenders. It's especially bad (i.e. it skips more) if you zoom in with Ctrl+.

EXPECTED RESULTS:
Only descenders should be skipped.

Chrome gives EXPECTED RESULTS. They're using the same webfont, "Guardian TextEgyp". This might be a difference in the choice of underline offset, or perhaps a different tolerance for closeness of decoration + text?

Calling this "P2" since we're about to ship this feature and Engadget is a pretty major site...

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (standalone)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screencast of testcase 1, zooming in, in Firefox vs. Chrome

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: with text-decoration-skip-ink:none

Here's essentially the same testcase, with text-decoration-skip-ink:none, just so you can look at the placement of the underline in the absence of ink-skipping.

If you load this testcase in Firefox vs. Chrome, you can tell that the underline is much closer to the text in Firefox (particularly at high full-page-zoom levels).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Screenshot of testcase 2 at 300% full-page-zoom, in Firefox NIghtly (top) vs. Chrome (bottom)

Here's a screenshot showing testcase 2 in Firefox (no space between text+underline) vs. Chrome (nice clearly-defined space between text+underline).

Comment 5

[Jonathan Kew [:jfkthame]]

This is the fault of the font. Dumping the 'post' table with TTX to check, we find:

  <post>
    <formatType value="2.0"/>
    <italicAngle value="0.0"/>
    <underlinePosition value="0"/>
    <underlineThickness value="102"/>
    ....etc

Note in particular underlinePosition value="0". This calls for the underline to be placed exactly at the baseline of the text. So no wonder it's liable to clash with most glyphs, and get broken up badly when skip-ink is used.

A simple fix for the site -- assuming it may take some time to actually get the font fixed -- would be to use text-underline-offset: 1px to shift the underline slightly below the baseline of the text.

(Chrome doesn't show the issue because it doesn't respect the underline position and thickness parameters provided by the font.)

Comment 6

[Jonathan Kew [:jfkthame]]

So what we should probably do to mitigate this issue is to clamp the offset to a reasonable minimum gap when text-underline-offset:auto is in effect, rather than entirely trusting the font's value.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1582016 - When text-underline-offset is `auto`, clamp the minimum used underline-offset to 1/16em. r?dholbert

This is to avoid skip-ink problems with fonts that leave the OpenType field at zero,
which is unlikely to ever be what is really wanted. Authors can still avoid the clamp by
explicitly using text-underline-offset:from-font.

Comment 8

[Jonathan Kew [:jfkthame]]

Not sure if this will offend the reftest gods... I've pushed a try job to see how it looks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead75486cad6c4f62887f30d3241c26acc921dd3

Comment 9

[Daniel Holbert [:dholbert]]

We should uplift this to beta70 (where these features are enabled by default via bug 1573631), perhaps after this has gotten a day or so of testing on Nightly.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f97bd10d6da
When text-underline-offset is `auto`, clamp the minimum used underline-offset to 1/16em. r=dholbert

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2f97bd10d6da

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 9093544 [details]
Bug 1582016 - When text-underline-offset is auto, clamp the minimum used underline-offset to 1/16em. r?dholbert

Beta/Release Uplift Approval Request

• User impact if declined: text-decoration-skip-ink (new feature shipping in 70) leads to poor rendering of underlined text (e.g. links) on sites such as Engadget due to use of a webfont that does not have a proper underline-offset value.
  (Rendering was already somewhat poor with such fonts, due to the underline clashing with the baseline of glyphs, but with skip-ink enabled - which is the default setting - it becomes much worse; the underlining breaks up and virtually disappears.)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small patch to apply a minimum-offset clamp to the underline-offset value found in the font, so as to avoid clashing with glyphs in the case where the font doesn't provide a real value; no effect other than on underline positioning.
• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9093544 [details]
Bug 1582016 - When text-underline-offset is auto, clamp the minimum used underline-offset to 1/16em. r?dholbert

Fix for text-underline-offset shipping in Fx70. Approved for 70.0b8.

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/64c1b75ba5a7

Comment 15

[Alexandru Trif, Desktop QA [:atrif]]

Hello!
Reproduced the issue using Firefox 71.0a1 (20190917155453) on Windows 10x64 using the test cases and the website mentioned.
The issue is verified fixed with Firefox 71.0a1 (20190922213341) and Firefox 70.0b8 (20190919164641) on Windows 10x64, macOS 10.14, Ubuntu 18.04.