1572302 - fix text-decoration-skip-ink issue with Awami-Nastaliq character

Status: RESOLVED FIXED

(Core :: Layout, defect, P5)

Description

[Charlie Marlow [:cmarlow]]

text-decoration-skip-ink has an issue with drawing the underline with arabic text using the Awami-Nastaliq font when there is a soft line-wrap. The issue can be seen in the third and fourth case here: https://charliemarlow.github.io/SkipInkTests/skipInkBugs.html

On that page the only difference between the working case and the failing case is the s on the second line. From some initial testing, it seems that adding the s changes the frame point (location on the page of the text) of the first character on the first line, even though it doesn't move at all when it is rendered.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Somehow, the lz4 1.9.2 update in bug 1575293 made this test start passing. Per IRC discussion with dholbert, we're going to update the annotation and get on with life as the screenshots confirm that we're rendering the testcase identically to the reference expectation (not just breaking both equally).

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1572302 - Update the expected result for skip-ink-multiline-position. r=dholbert

Somehow, updating lz4 from version 1.9.1 to 1.9.2 makes this test reliably pass.

Comment 3

[Daniel Holbert [:dholbert]]

jfkthame, does this make any sense at all? (Do we use lz4 to decompress/decode fonts?)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8e037a5fb87
Update the expected result for skip-ink-multiline-position. r=dholbert

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #3)

jfkthame, does this make any sense at all? (Do we use lz4 to decompress/decode fonts?)

Not in most cases.... but graphite does use it internally to decompress its special layout tables, and as it happens, Awami is a (very complex) graphite font. So in this particular case, yes, there is a connection to lz4, and so the update could have changed something.

Having said that, it still seems a bit surprising, so I want to look at how Awami renders after the lz4 update... it could well be broken (equally in both testcase and reference), resulting in a spurious pass here. Leaving needinfo for now until I've had a chance to look at it.

Comment 6

[Daniel Holbert [:dholbert]]

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

Having said that, it still seems a bit surprising, so I want to look at how Awami renders after the lz4 update... it could well be broken (equally in both testcase and reference), resulting in a spurious pass here. Leaving needinfo for now until I've had a chance to look at it.

I'm pretty sure it's fixed in both testcase and reference case (rather than broken in both). At least: the reftest-screenshot for the "unexpected pass"[1] renders the same way that the reference case renders right now in Nightly (with a decoration-line before & after the "S"-like character, and just above the cross of the "f"-like character, skipping over the main vertical part of the "f".)

[1]
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=262690695&repo=autoland&lineNumber=10471
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/fROukQ-GTqCiVcJsFIRKvw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 7

[Jonathan Kew [:jfkthame]]

Unfortunately, the screenshot there isn't enough to tell whether Awami shaping is working correctly, as it just includes an isolated Arabic-script character that doesn't depend on Graphite rules.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Per bug 1575293 comment 9, it sounds like lz4 caused a real regression which made this test start passing. Backed out alongside the lz4 update (though I did leave the typo fixes in place).

https://hg.mozilla.org/integration/autoland/rev/395f2aa0a4b57e16de37e7e64e2b1218971e7e51

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Looks like the failing reftest annotation was removed in bug 1655364. Did that fix this bug, Jonathan?

Comment 10

[Jonathan Kew [:jfkthame]]

Oops, I should've resolved it when that landed.

--> FIXED as an unplanned (but welcome) result of the code cleanup in bug 1655364.