1599841 - Combination of unicode-bidi: bidi-override, Arabic subtending mark, and a CSS counter value makes single digits disappear

Status: RESOLVED FIXED

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

Description

[Saadat Mateen]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

Write the following code:

HTML:
<ul>
<li></li>
<li></li>
...
</ul>

CSS:
ul {
font-family: "Noto Nastaliq Urdu", sans-serif;
counter-reset: c;
list-style: none;
}

ul li::before {
content: "؂"counter(c, persian);
counter-increment: c;
unicode-bidi: bidi-override;
}

Actual results:

When the counter value had a single digit, only the subtending mark (؂) was rendered.

When the counter value had more than one digits, they were (correctly) rendered on top of the subtending mark.

Expected results:

Just like multiple digits, a single digit should also have rendered on top of the subtending mark.

Comment 1

[Saadat Mateen]

Attachment: footnote-mark-with-digits.png

Comment 2

[Saadat Mateen]

Attachment: subtending-mark-css-counter-noto-nastaliq.zip

Comment 3

[Saadat Mateen]

Some background about this issue:

In the Arabic Unicode block, there are some signs or marks that span numbers (sometimes called Arabic subtending marks). They include:

U+0600 ARABIC NUMBER SIGN
U+0601 ARABIC SIGN SANAH
U+0602 ARABIC FOOTNOTE MARKER
U+0603 ARABIC SIGN SAFHA
U+0604 ARABIC SIGN SAMVAT

In text representation, these marks precede the sequence of digits that they span. For example:

U+0602 U+06F1 U+06F2

which will be ؂۱۲. Visually, it should look like as shown in the attachement footnote-mark-with-digits.png.

The font Noto Nastaliq Urdu supports these marks, but in Firefox (and Chrome too), the sequence <Arabic subtending mark + digits> set in Noto Nastaliq Urdu require to be surrounded by U+202D/U+202E and U+202C to render correctly, which can be done in CSS via unicode-bidi: bidi-override.

Now, the issue:

A subtending mark and its digit(s), set in Noto Nastaliq Urdu and with unicode-bidi: bidi-override, render correctly in plain HTML elements.

But: in the content CSS property, a subtending mark and its single digit (coming from a CSS counter value), results in the rendering of just the mark and not the digit. If the CSS counter value has more than one digits, then the mark and digits do render correctly.

A test page showing the comparison between HTML element and CSS counter can be found here. In that page, both columns should look the same, with digits being rendered on top of the subtending mark, but the column titled “In CSS counter” does not show single digits.

A reduced test case is also attached (subtending-mark-css-counter-noto-nastaliq.zip). Screenshot of this test case in both Firefox and Chrome is attached, too (where Chrome is showing the correct rendering).

(In both test pages, please ignore the incorrect rendering of ۶ (U+06F6), which is due to a font bug.)

This used to render correctly in previous versions of Firefox. (As far as I can tell, the last Firefox version where it worked was 69.0.3). Also works correctly in Chrome.

Comment 4

[Saadat Mateen]

Attachment: ff-chrome-comparison.png

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 6

[Cameron McCormack (:heycam)]

mozregression tells me this was caused by bug 1513423. Jonathan, could you take a look?

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix in 70/71 since the 71 release is tomorrow.

Comment 8

[Jonathan Kew [:jfkthame]]

Ah, I think I see the problem here; when we're measuring RTL text as if it were LTR, just mirroring bounding boxes etc., we need to invert the sign of any DetailedGlyph mOffset.x value that's being applied to the glyph rect. I've pushed a try run to check that fixing this doesn't unexpectedly break anything else.

Assuming all is well, we should also add a testcase that specifically catches this issue.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1599841 - Invert the direction of details->mOffset.x when doing "mirrored" measurement of RTL glyph runs. r=heycam

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1599841 - Reftest based on the testcase here. r=heycam

Comment 11

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on Windows 10 x64, updating affected FX73 flag.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f03a144b7b0
Invert the direction of details->mOffset.x when doing "mirrored" measurement of RTL glyph runs. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a7a57d54b008
Reftest based on the testcase here. r=heycam

Comment 13

[Daniel Varga [:dvarga]]

Backed out 2 changesets (Bug 1599841) for causing reftest failures at tests/reftest/tests/layout/reftests/bugs/1599841-1.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=a7a57d54b0080ba764b94464cd38a6557b4fa414

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280291969&repo=autoland&lineNumber=11186

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=7e76637fcbe05ee59a969bfda4bb68a15ccb6cc1

Reftest analyzer: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/D4hxRoAzRhSFJfRrWcWnmw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0f23fdfc04b
Invert the direction of details->mOffset.x when doing "mirrored" measurement of RTL glyph runs. r=heycam
https://hg.mozilla.org/integration/autoland/rev/eb4efd9664c0
Reftest based on the testcase here. r=heycam

Comment 15

[Jonathan Kew [:jfkthame]]

Ah, drat -- I had annotated this appropriately in my local tree, and pushed a try job that confirmed it was all green, but failed to update the revision in phabricator. :-(

Comment 16

[Daniel Varga [:dvarga]]

Backed out 1 changesets (Bug 1599841) for causing reftest failures at tests/layout/reftests/bugs/1562733-rotated-nastaliq-1.html

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=eb4efd9664c005f3fbffe9f4ec19e7c0d13723cd

Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=280308210&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=c12330a620c4fe34b5c55d9d225554dbaa69640b&searchStr=android-em-7-0-x86_64-qr%2Copt%2Creftests%2Ctest-android-em-7.0-x86_64-qr%2Fopt-geckoview-reftest-e10s-1%2Cr%28r1%29

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=9dc0369e19ac424eba4992ced84d50dc12216789

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0166a597e7d
Invert the direction of details->mOffset.x when doing "mirrored" measurement of RTL glyph runs. r=heycam

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f0166a597e7d

Comment 19

[Julien Cristau [:jcristau]]

We've shipped this before, so fix-optional for 72. Would take an uplift though if you think it's worth it.

Comment 20

[Jonathan Kew [:jfkthame]]

It's a pretty obscure case, but OTOH the failure when it happens is bad (characters simply missing from the rendering). The patch carries minimal risk as it doesn't affect anything except this obscure case, so I think it'd be worth uplifting.

Comment 21

[Jonathan Kew [:jfkthame]]

Comment on attachment 9113285 [details]
Bug 1599841 - Invert the direction of details->mOffset.x when doing "mirrored" measurement of RTL glyph runs. r=heycam

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect rendering (possibility of "disappearing" glyphs) in RTL text where complex shaping is involved
• 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): This is a small, minimal-risk patch - the only thing affected is the bounding-box returned for glyphs in RTL text when shaping has applied a positional offset to the glyph. There is no effect outside of RTL text, and no wider effect on layout, only on the bounding box used in painting.
• String changes made/needed: none

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 9113285 [details]
Bug 1599841 - Invert the direction of details->mOffset.x when doing "mirrored" measurement of RTL glyph runs. r=heycam

bidi fix, approved for 72.0b5

Comment 23

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b6a76dc240a7
https://hg.mozilla.org/releases/mozilla-beta/rev/f390141e3f22

Comment 24

[Saadat Mateen]

Just confirmed the fix on Firefox 72.0b5. Thanks. :-)