Closed Bug 1318539 Opened 7 years ago Closed 7 years ago

RTL text using colored font is invisible with non-opaque text color

Categories

(Core :: Graphics: Text, defect)

50 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox50 --- wontfix
firefox51 + wontfix
firefox52 + wontfix
firefox53 + fixed
firefox54 --- fixed

People

(Reporter: corbett.dav, Assigned: jfkthame)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161115182233

Steps to reproduce:

1. I downloaded Amiri Quran Colored from https://github.com/khaledhosny/amiri-font/blob/eb309222e715615c872d625943a74c741646d1c7/amiri-quran-colored.ttf. This font uses COLR and CPAL tables to color the dots of Arabic letters red while leaving their skeletons in the text foreground color.
2. I rendered this document: data:text/html;charset=utf-8,<span style="font: 200px Amiri Quran Colored; color:%238888">(%D9%82)</span>


Actual results:

Between two #8888 parentheses was an invisible Arabic letter qaf.


Expected results:

Between two #8888 parentheses should be a #8888 Arabic letter qaf with opaque red dots.

For comparison, if you set the color to #888 or any other opaque color, it is rendered as expected.
Component: Untriaged → Graphics: Text
Product: Firefox → Core
Your thoughts, Jonathan?
Flags: needinfo?(jfkthame)
I can't reproduce this with FF 48 but got problem after FF 50 on mac.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [gfx-noted]
Quite possibly, but I’ve no idea what is going on. If I try to print the layer colors, I get:

r: 0.533333, g: 0.533333, b: 0.533333, a: 1.00000
r: 0.800000, g: 0.200000, b: 0.200000, a: 1.00000

So the first layer is getting the text color (since it has no assigned color in the font) and the second layer is getting the reddish color from the font. Not sure about the alpha values.
Khaled, could you check this issue after your patch in bug 1283932.
Flags: needinfo?(khaledhosny)
Track 51+/52+/53+ as regression.
Hmm, after considerable puzzling about this, I ran across an interesting quirk: the problem seems to be related to the colored Arabic font being RTL. If we override the character properties so that the entire testcase has LTR directionality, suddenly the Qaf appears:

data:text/html;charset=utf-8,<div style="unicode-bidi:bidi-override;"><span style="color:rgba(0,255,0,.5); font: 200px Amiri Quran Colored;">(%D9%82)</span>

I haven't yet identified exactly where things are going wrong, but this may provide a clue...
Flags: needinfo?(jfkthame)
Version: 51 Branch → 50 Branch
Jonathan, any recent insights on this?
Flags: needinfo?(jfkthame)
Some additional testcases that may help narrow down the failure...

Similar failure with a color-emoji character (reproduced on macOS, but I expect it would fail similarly on other platforms provided there's a color emoji font):
  data:text/html;charset=utf-8,<div dir=rtl style="opacity:0.5; font:100px serif;">(%F0%9F%98%80)

Removing the dir=rtl attribute makes all the text appear as expected:
  data:text/html;charset=utf-8,<div style="opacity:0.5; font:100px serif;">(%F0%9F%98%80)

Removing the opacity:0.5 property makes all the text appear as expected:
  data:text/html;charset=utf-8,<div dir=rtl style="font:100px serif;">(%F0%9F%98%80)

Adding a strong-LTR character to the content makes all the text appear as expected:
  data:text/html;charset=utf-8,<div dir=rtl style="opacity:0.5; font:100px serif;">(%F0%9F%98%80x)

One key observation here is that it's not merely color-font + partial-opacity that leads to the failure; it is also necessary for the text involved to be RTL (either inherently, as in the Arabic example, or through the use of neutral-direction characters in an RTL context).

Even that isn't quite the whole story; in this example, the emoji (neutral) appear RTL because of the surrounding Arabic letters, yet the text still renders fine, because the surrounding parens are LTR:
  data:text/html;charset=utf-8,<div style="opacity:0.5; font:100px serif;">(%D9%81%F0%9F%98%80%F0%9F%98%81%D9%82)

Remove the parens, and it fails:
  data:text/html;charset=utf-8,<div style="opacity:0.5; font:100px serif;">%D9%81%F0%9F%98%80%F0%9F%98%81%D9%82
Flags: needinfo?(jfkthame)
Another note: with the testcases in comment 10, selecting the invisible text (e.g. with Cmd-A) makes it suddenly become visible. However, after deselecting it again, it may vanish as soon as a repaint is triggered (e.g. by resizing the window).
Summary: Colored font is invisible with non-opaque text color → RTL text using colored font is invisible with non-opaque text color
I have a patch that I think fixes this, just testing a bit before I post it for review.
Flags: needinfo?(khaledhosny)
Assignee: nobody → jfkthame
Attachment #8839404 - Flags: review?(jmuizelaar) → review+
Attachment #8839402 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b98ddcdbfdae76e5cd45decbc2249e5b7df837a
Bug 1318539 - Reftests for RTL rendering of color glyph with partial opacity (currently failing). r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c364b17c93ffb5745d283596bdc788bc028f675
Bug 1318539 - Correct the bounds of the buffer used for drawing colored text with opacity when the run direction is RTL. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/9b98ddcdbfda
https://hg.mozilla.org/mozilla-central/rev/1c364b17c93f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Looks like a pretty small patch with tests included. Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(jfkthame)
Flags: in-testsuite+
Comment on attachment 8839404 [details] [diff] [review]
Correct the bounds of the buffer used for drawing colored text with opacity when the run direction is RTL

AFAICS this is not actually a regression from bug 1283932, as suggested earlier;  it has probably existed since color font support was initially implemented. The fix in bug 1283932 made the issue appear with the Amiri Colored font because previously, we didn't handle its color table at all, but this bug existed prior to that and could have been demonstrated with other fonts such as emoji in RTL content.

I don't think this is critical enough to justify a late-beta uplift; it's an extremely specific case that isn't likely to be common in the wild. But it would be good to take on Aurora, given that it is such a simple and small fix.

Approval Request Comment
[Feature/Bug causing the regression]: color font support

[User impact if declined]: RTL text using colored fonts (e.g. emoji) with partial opacity fails to render (is invisible)

[Is this code covered by automated tests?]: yes

[Has the fix been verified in Nightly?]: yes

[Needs manual test from QE? If yes, steps to reproduce]: no

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: no

[Why is the change risky/not risky?]: localized change affecting only this particular code path (RTL + color-font or synthetic bold + transparency), so cannot regress any other case

[String changes made/needed]: none
Flags: needinfo?(jfkthame)
Attachment #8839404 - Flags: approval-mozilla-aurora?
wontfix for 52 per comment 19.  Thanks Jonathan.
Doesn't apply to aurora
warning: conflicts while merging layout/reftests/text/reftest.list!
Flags: needinfo?(jfkthame)
(In reply to Sylvestre Ledru [:sylvestre] from comment #21)
> Doesn't apply to aurora
> warning: conflicts while merging layout/reftests/text/reftest.list!

The reftest patch (with tests annotated as "fails") lands first, then the actual bug-fix lands (and removes the annotations).
Flags: needinfo?(jfkthame)
Comment on attachment 8839404 [details] [diff] [review]
Correct the bounds of the buffer used for drawing colored text with opacity when the run direction is RTL

Fix an RTL text issue. Aurora53+.
Attachment #8839404 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.