1283932 - COLR table with uncolored layers is rejected

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Khaled Hosny]

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.

Comment 1

[Khaled Hosny]

Attachment: Test font

Comment 2

[Khaled Hosny]

Attachment: Test HTML file

Comment 3

[Khaled Hosny]

Attachment: WIP patch for fixing the issue

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.

Comment 4

[Khaled Hosny]

Attachment: The expected rendering (after applying the patch)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Jonathan, could you have a look at the patch, and review it and/or advise Khaled on how to proceed?

Comment 6

[Jonathan Kew [:jfkthame]]

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.

Comment 7

[Khaled Hosny]

Attachment: Patch, v2

Thanks Jonathan for the review, I updated the patch as suggested.

Comment 8

[Jonathan Kew [: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

Comment 9

[Khaled Hosny]

Attachment: Patch, v3

A new patch addressing review comments.

Comment 10

[Jonathan Kew [: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.

Comment 11

[Khaled Hosny]

Attachment: Reftests

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.

Comment 12

[Jonathan Kew [: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.

Comment 13

[Khaled Hosny]

(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.

Comment 14

[Khaled Hosny]

Attachment: Reftests, v2

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 16

[Khaled Hosny]

Thanks fir the links. I don’t have try server access, so help here is much appreciated.

Comment 17

[Jonathan Kew [:jfkthame]]

Yes, I'll take care of getting this checked in.
Pushed a try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cc994ee52bd.

Comment 18

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e666f640f6434593a6077b4dc24e96bfdf868166
Bug 1283932 - support COLR table layers with unset color, r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c74a5856983742da3d22209637ff483ee5e83c
Bug 1283932 - reftests, r=jfkthame

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e666f640f643
https://hg.mozilla.org/mozilla-central/rev/d2c74a585698