1617515 - Gaps in the underline don't align right when an emoji is placed in the text

Status: RESOLVED FIXED

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

Description

[Karl Dubost💡 :karlcow]

Attachment: rendering in firefox, chrome and safari

As said in https://codepen.io/remco_dekker/pen/abOZyQK

Gaps in the underline don't align right when an emoji is placed in the text.
This seems to only be a problem in Firefox on macOS.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1617515 - Properly advance current position for glyph runs where we don't implement ink-skipping. r=dholbert

Comment 2

[Jonathan Kew [:jfkthame]]

This is a sufficiently bad visual glitch that I think we may want to consider uplifting it to 74.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43da03986d1d
Properly advance current position for glyph runs where we don't implement ink-skipping. r=dholbert

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 9128453 [details]
Bug 1617515 - Properly advance current position for glyph runs where we don't implement ink-skipping. r=dholbert

Beta/Release Uplift Approval Request

• User impact if declined: Potential for bad underline rendering when emoji present
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: See codepen in comment 0.
  (Reftest is included with patch, just pushed to autoland.)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial patch, just updates position when we step past a glyph run
• String changes made/needed:

Comment 5

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 9128453 [details]
Bug 1617515 - Properly advance current position for glyph runs where we don't implement ink-skipping. r=dholbert

Low risk patch, covered by tests, uplift approved for 74.0b8, thanks.

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/43da03986d1d

Comment 7

[Natalia Csoregi [:nataliaCs]]

Unable to uplift - conflicts while merging layout/painting/nsCSSRendering.cpp.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1617515 - [mozilla-beta] Properly advance current position for glyph runs where we don't implement ink-skipping. r=dholbert a=pascalc

Comment 9

[Jonathan Kew [:jfkthame]]

I posted a rebased patch (https://phabricator.services.mozilla.com/D64326) for mozilla-beta, but don't know how to make phabricator land it there. (Shouldn't need re-review or anything; the only merge conflict was in comments around one of the changes, not affecting the actual code change.)

Comment 10

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

https://hg.mozilla.org/releases/mozilla-beta/rev/ff7d83d22698

Comment 11

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

Thanks. Phabricator doesn't handle uplifts yet.

Comment 12

[Oana Pop-Rus]

Backed out changeset ff7d83d22698 (bug 1617515) for multiple reftest failures on layout/reftests/bugs/1617515-1.html

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=ff7d83d2269814adad427c91dab3e8620d1a959e&selectedJob=290538605

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290538605&repo=mozilla-beta&lineNumber=11480

Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/002bb48bab1c3319e3723cddec6b92eb8cec6c1b

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Oana Pop-Rus from comment #12)

Backed out changeset ff7d83d22698 (bug 1617515) for multiple reftest failures on layout/reftests/bugs/1617515-1.html

Argh - sorry, my bad - the beta patch didn't actually include the reftest files. Mercurial user error!

I've updated the revision on phabricator, should be OK now. At least, it shows the added files when I view it in phab.

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/e801d73575fe