Closed Bug 1283932 Opened 5 years ago Closed 4 years ago

COLR table with uncolored layers is rejected

Categories

(Core :: Graphics: Text, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: dr.khaled.hosny, Assigned: dr.khaled.hosny)

References

Details

Attachments

(5 files, 3 obsolete files)

The COLR table spec (https://www.microsoft.com/typography/otspec/colr.htm) indicates that layers with the special palette index 0xFFFF take the text foreground color, however Firefox rejects the COLR table if this value is used and displays the text with the base glyphs.
Attached file Test font
Attached file Test HTML file
Attached patch WIP patch for fixing the issue (obsolete) — Splinter Review
This patch fixes the rejection of the table, and tries to render such layers as regular glyphs. I’m not sure what is the best way to indicate that a color is unset, I used (-1, -1, -1, -1) because (0, 0, 0, 1) is a valid (though useless) color.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jfkthame)
Jonathan, could you have a look at the patch, and review it and/or advise Khaled on how to proceed?
Yes, this is clearly a bug we should fix; thanks for the report & WiP patch, Khaled.

I'm not sure this is quite the best approach to the rendering, though. AIUI, by putting layers with "unset" (foreground) color into the main glyph buffer instead of emitting them individually from RenderColorGlyph, this will have the effect of re-ordering the layers of the colored glyph. If there were foreground-color layers that were overlapped by later specified-color layers, this will alter the rendering.

To avoid this, I'd suggest passing the current text-fill color in to RenderColorGlyph, so that it can simply use it instead of a color from the CPAL table when necessary, and still paint the layers in the proper order. The necessary color should be available from runParams.context->GetDeviceColor() in DrawOneGlyph, I think.

Another possible modification would be to avoid the need for a magic "unset" color value by instead passing the foreground color through to GetColorLayersInfo(), and then when it sees an index of 0xffff it would simply assign that color to the layer. This seems perhaps cleaner/simpler to me, though it's purely an implementation detail that shouldn't affect the result.
Flags: needinfo?(jfkthame)
Attached patch Patch, v2 (obsolete) — Splinter Review
Thanks Jonathan for the review, I updated the patch as suggested.
Attachment #8767342 - Attachment is obsolete: true
Attachment #8767410 - Flags: review?(jfkthame)
Comment on attachment 8767410 [details] [diff] [review]
Patch, v2

Review of attachment 8767410 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks! Just a couple of minor Gecko coding style nits, and there's one small precautionary change I think we should make; see below.

::: gfx/thebes/gfxFont.cpp
@@ +2170,4 @@
>  
>  bool
>  gfxFont::RenderColorGlyph(DrawTarget* aDrawTarget,
> +                          gfxContext *aContext,

Style nit: please attach the * to the type rather than the variable.

@@ +2180,5 @@
>      AutoTArray<uint16_t, 8> layerGlyphs;
>      AutoTArray<mozilla::gfx::Color, 8> layerColors;
>  
> +    mozilla::gfx::Color defaultColor;
> +    aContext->GetDeviceColor(defaultColor);

GetDeviceColor() returns a bool indicating whether it succeeded, so I think we should do something like:

    if (!aContext->GetDeviceColor(defaultColor)) {
        defaultColor = mozilla::gfx::Color(0, 0, 0);
    }

here to provide a safe fallback.

::: gfx/thebes/gfxFont.h
@@ +2155,4 @@
>                          bool& aEmittedGlyphs) const;
>  
>      bool RenderColorGlyph(DrawTarget* aDrawTarget,
> +                          gfxContext *aContext,

Move the * to end of gfxContext.

::: gfx/thebes/gfxFontEntry.cpp
@@ +1062,4 @@
>  
>  bool
>  gfxFontEntry::GetColorLayersInfo(uint32_t aGlyphId,
> +                            mozilla::gfx::Color& defaultColor,

Please declare the defaultColor param as const.

Also, gecko style[1] is to prefix argument names with 'a', so it should be aDefaultColor.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes

::: gfx/thebes/gfxFontEntry.h
@@ +265,4 @@
>  
>      bool     TryGetColorGlyphs();
>      bool     GetColorLayersInfo(uint32_t aGlyphId,
> +                                mozilla::gfx::Color& defaultColor,

const mozilla::gfx::Color& aDefaultColor

::: gfx/thebes/gfxFontUtils.cpp
@@ +1714,4 @@
>  gfxFontUtils::GetColorGlyphLayers(hb_blob_t* aCOLR,
>                                    hb_blob_t* aCPAL,
>                                    uint32_t aGlyphId,
> +                                  mozilla::gfx::Color& defaultColor,

const mozilla::gfx::Color& aDefaultColor

@@ +1757,5 @@
> +            aColors.AppendElement(mozilla::gfx::Color(color->red / 255.0,
> +                                                      color->green / 255.0,
> +                                                      color->blue / 255.0,
> +                                                      color->alpha / 255.0));
> +            }

This is indented one step too far.

::: gfx/thebes/gfxFontUtils.h
@@ +979,4 @@
>      static bool GetColorGlyphLayers(hb_blob_t* aCOLR,
>                                      hb_blob_t* aCPAL,
>                                      uint32_t aGlyphId,
> +                                    mozilla::gfx::Color& defaultColor,

const mozilla::gfx::Color& aDefaultColor
Attached patch Patch, v3Splinter Review
A new patch addressing review comments.
Attachment #8767410 - Attachment is obsolete: true
Attachment #8767410 - Flags: review?(jfkthame)
Attachment #8767420 - Flags: review?(jfkthame)
Comment on attachment 8767420 [details] [diff] [review]
Patch, v3

Review of attachment 8767420 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, thanks!

Could you also create a reftest for this? It could be similar to your original testcase, but ideally the test glyph should be modified so that it involves overlapping layers with specified and unset colors. I'm envisioning a font with, say, three glyphs A, B, and C, all with the same set of layered shapes, but in A, some of the layers have unset color; B and C have all their colors specified, and A will match one or the other of them depending on the CSS foreground color that's applied.
Attachment #8767420 - Flags: review?(jfkthame) → review+
Attached patch Reftests (obsolete) — Splinter Review
Reftests attached. I created a new font with 4 glyphs; A: with a red layer overlapping a layer with unset color, B: with the other layer colored green, C: with it colored blue, and D: with no colors at all.
Attachment #8767533 - Flags: review?(jfkthame)
Comment on attachment 8767533 [details] [diff] [review]
Reftests

Review of attachment 8767533 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/font-face/color-2a.html
@@ +14,5 @@
> +}
> +</style>
> +</head>
> +<body>
> +<span>A</span><span style="color: #00FF00FF">B</span><span style="color: #0000FFFF">C</span>

Did you perhaps intend to use glyph A in all three spans here, with the different foreground colors applied? ISTM that would be the better test.
(In reply to Jonathan Kew (:jfkthame) from comment #12)
> Comment on attachment 8767533 [details] [diff] [review]
> Reftests
> 
> Review of attachment 8767533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/font-face/color-2a.html
> @@ +14,5 @@
> > +}
> > +</style>
> > +</head>
> > +<body>
> > +<span>A</span><span style="color: #00FF00FF">B</span><span style="color: #0000FFFF">C</span>
> 
> Did you perhaps intend to use glyph A in all three spans here, with the
> different foreground colors applied? ISTM that would be the better test.

That is right of course.
Attached patch Reftests, v2Splinter Review
Attachment #8767533 - Attachment is obsolete: true
Attachment #8767533 - Flags: review?(jfkthame)
Attachment #8767653 - Flags: review?(jfkthame)
Attachment #8767653 - Flags: review?(jfkthame) → review+
Assignee: nobody → khaledhosny
I'm not sure if Khaled is familiar with how to get code into the tree.  If not these might be of interest:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
https://wiki.mozilla.org/Build:TryServer

Jonathan, could you help with making an appropriate push to try, at least?
Flags: needinfo?(jfkthame)
Thanks fir the links. I don’t have try server access, so help here is much appreciated.
Yes, I'll take care of getting this checked in.
Pushed a try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cc994ee52bd.
Flags: needinfo?(jfkthame)
https://hg.mozilla.org/mozilla-central/rev/e666f640f643
https://hg.mozilla.org/mozilla-central/rev/d2c74a585698
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1318539
You need to log in before you can comment on or make changes to this bug.