1402413 - Link underlines are positioned inside the link text

Status: RESOLVED FIXED

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

Description

[berwyn.codeweaver]

Attachment: Screen Shot 2017-09-22 at 13.49.20.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170917031738

Steps to reproduce:

1. Navigate to https://www.freedommobile.ca/plans-and-devices/mobile-devices/device-details/lg-v20

Note that:
 - `gfx.webrender.enabled` is set to true
 - `gfx.webrendest.enabled` is set to true
 - `gfx.webrender.layers-free` is set to true
 - `gfx.webrender.blob-images` is set to true
 - `layers.async-pan-zoom.enabled` is set to false


Actual results:

Link underlines are rendered like strikethroughs

Note the "LTE", "Terms & Conditions", and "MyTab" links near the top of the page


Expected results:

The underlines appear under the word, rather than through it

Comment 1

[Aria Beingessner [:Gankra]]

Woah neat! Thanks for reporting. I'll look into this when we get the build back into a state that renders text at all. :/

Comment 2

[Aria Beingessner [:Gankra]]

It seems the issue is the GothamBook font. Not sure what's interesting about it yet.

Comment 3

[Aria Beingessner [:Gankra]]

Huh! This is actually visible in vanilla firefox on macos. Bug doesn't reproduce on linux or windows. 

Something in our mac font code is messing up the font metrics, I guess?

Comment 4

[Jonathan Kew [:jfkthame]]

This is the result of a font bug. Dumping the 'hhea' table from that .otf file:

  <hhea>
    <tableVersion value="0x00010000"/>
    <ascent value="960"/>
    <descent value="240"/>
    <lineGap value="0"/>
    <advanceWidthMax value="1273"/>
    <minLeftSideBearing value="-222"/>
    <minRightSideBearing value="-222"/>
    <xMaxExtent value="1216"/>
    <caretSlopeRise value="1"/>
    <caretSlopeRun value="0"/>
    <caretOffset value="0"/>
    <reserved0 value="0"/>
    <reserved1 value="0"/>
    <reserved2 value="0"/>
    <reserved3 value="0"/>
    <metricDataFormat value="0"/>
    <numberOfHMetrics value="502"/>
  </hhea>

Note that the 'descent' value is positive. This is... unusual, shall we say. Compare a standard font like Arial, for instance:

  <hhea>
    <tableVersion value="0x00010000"/>
    <ascent value="1854"/>
    <descent value="-434"/>
    <lineGap value="67"/>
    <advanceWidthMax value="4096"/>
    <minLeftSideBearing value="-1361"/>
    <minRightSideBearing value="-1414"/>
    <xMaxExtent value="4096"/>
    <caretSlopeRise value="1"/>
    <caretSlopeRun value="0"/>
    <caretOffset value="0"/>
    <reserved0 value="0"/>
    <reserved1 value="0"/>
    <reserved2 value="0"/>
    <reserved3 value="0"/>
    <metricDataFormat value="0"/>
    <numberOfHMetrics value="3381"/>
  </hhea>

(The values in GothamSSm Book are based on an em-square of 1000 units, while Arial uses 2048 units, so its values are around twice as large. But the important point is the sign of the descent value.)

This only shows up on macOS because the 'hhea' table metrics are traditionally used by Apple, while other platforms rely more on metrics from the OS/2 table, and Gotham's OS/2 has a negative sTypoDescender field, as expected.

Comment 5

[Aria Beingessner [:Gankra]]

Note that this renders correctly in other browsers (safari/chrome). Apparently everyone else has some mechanism that reliably avoids this problem?

Comment 6

[Jonathan Kew [:jfkthame]]

Most probably they use the same (OS/2-table-based) metrics across all platforms, for better consistency.

We might want to try that (issues in this area come up from time to time), but changes to how we compute font metrics (on any platform) carry quite a bit of potential risk -- e.g. it's liable to affect lots of details of line-heights in the UI, so that the front-end people would have to do a whole bunch of revisions to carefully-tuned spacing parameters.

Comment 7

[Jonathan Kew [:jfkthame]]

A possible mitigation here -- with less risk than a complete reimplementation of font metrics -- would be to check for a descent value with the opposite sign to what's expected (i.e. indicating a descender line that's above the baseline), and assume this is an error (so we'd either flip the sign, or try falling back to a different set of metrics from the font). It seems unlikely there are any real-world cases where a font would expect us to use such a value for the descent.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: If the TrueType 'hhea' table metrics resulted in negative maxDescent, assume they're probably broken and prefer metrics from the 'OS/2' table

This is a conservative fix for the kind of brokenness seen here, switching to the OS/2 metrics if the descender value seems to have the wrong sign, without affecting how we compute metrics in the general case. (I think a more extensive reworking of font metrics lies somewhere in our future, but this isn't the bug to address it.)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fffd2b6a6c7
If the TrueType 'hhea' table metrics resulted in negative maxDescent, assume they're probably broken and prefer metrics from the 'OS/2' table. r=jrmuizel

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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