Closed Bug 1402413 Opened 3 years ago Closed 3 years ago

Link underlines are positioned inside the link text

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: berwyn.codeweaver, Assigned: jfkthame)

Details

Attachments

(2 files)

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
Component: Untriaged → Graphics: WebRender
Product: Firefox → Core
Woah neat! Thanks for reporting. I'll look into this when we get the build back into a state that renders text at all. :/
Assignee: nobody → a.beingessner
Blocks: 1392723
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [wr-mvp] [triage]
It seems the issue is the GothamBook font. Not sure what's interesting about it yet.
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?
Assignee: a.beingessner → nobody
Status: ASSIGNED → NEW
Component: Graphics: WebRender → Layout: Text
Whiteboard: [wr-mvp] [triage]
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.
No longer blocks: 1392723
Note that this renders correctly in other browsers (safari/chrome). Apparently everyone else has some mechanism that reliably avoids this problem?
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.
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.
Priority: -- → P3
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.)
Attachment #8917420 - Flags: review?(jmuizelaar)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8917420 - Flags: review?(jmuizelaar) → review+
Pushed by jkew@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/5fffd2b6a6c7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.