Bug 103392 – Canvas does not draw any text if the font is not fully loaded yet

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

As per the latest Canvas Element specification (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html):
"If a font is used before it is fully loaded, or if the font style source object does not have that font in scope at the time the font is to be used, then it must be treated as if it was an unknown font, falling back to another as described by the relevant CSS specifications."

However, WebKit currently does not draw the text at all if the custom font is not fully loaded yet. We should handle this case better and follow the specification so that we use a fallback font for this case and draw the text.

[Chris Dumez]

Created attachment 176232 [details]
Patch

[Chris Dumez]

Created attachment 176234 [details]
Patch

Fix typo in bug title.

[Mikhail Pozdnyakov]

Comment on attachment 176234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176234&action=review

> Source/WebCore/platform/graphics/Font.cpp:159
> +void Font::drawText(GraphicsContext* context, const TextRun& run, const FloatPoint& point, int from, int to, bool force) const

isn't enum better than bool?

> Source/WebCore/platform/graphics/Font.cpp:162
> +    // except if the 'force' argument is set to true.

so what will it draw if fonts are not yet loaded?

[Chris Dumez]

Created attachment 176237 [details]
Patch

Converted test into a ref test as advised by Dominik offline.

[Kenneth Rohde Christiansen]

Comment on attachment 176232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176232&action=review

> LayoutTests/http/tests/canvas/canvas-slow-font-loading-expected.txt:3
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

You actually see more

> LayoutTests/http/tests/canvas/canvas-slow-font-loading-expected.txt:10
> +The 2 canvas should be identical (2 green 'X' on a red background).

this should be moved up

[Kenneth Rohde Christiansen]

Comment on attachment 176237 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176237&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2306
> +            maskImageContext->drawBidiText(font, textRun, FloatPoint(0, 0), true /*force*/);

I don't think force is easy to understand here. Force what?

[Chris Dumez]

(In reply to comment #5)
> (From update of attachment 176232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176232&action=review
> 
> > LayoutTests/http/tests/canvas/canvas-slow-font-loading-expected.txt:3
> > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> You actually see more
> 
> > LayoutTests/http/tests/canvas/canvas-slow-font-loading-expected.txt:10
> > +The 2 canvas should be identical (2 green 'X' on a red background).
> 
> this should be moved up

It's a ref test now.

[Chris Dumez]

(In reply to comment #6)
> (From update of attachment 176237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176237&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2306
> > +            maskImageContext->drawBidiText(font, textRun, FloatPoint(0, 0), true /*force*/);
> 
> I don't think force is easy to understand here. Force what?

Agreed. Mikhail's suggestion to use an enum instead is good. I plan to do that to make it more understandable. I'm currently think about a good enum name :)

[Chris Dumez]

Created attachment 176255 [details]
Patch

[Chris Dumez]

Sadly, I'm getting a build error due to a conflict between "Complex" typedef from WTF::Complex and Complex enum value in Font.h.

I'll reupload a patch renaming "Complex" enum value to "ComplexPath" to avoid conflict with WTF::Complex :(

[Chris Dumez]

Created attachment 176262 [details]
Patch

[Chris Dumez]

Comment on attachment 176262 [details]
Patch

Based on Kenneth's offline feedback, I'll split the "Complex" enum value renaming to a separate (dependent) patch. Clearing flags for now.

[Chris Dumez]

(In reply to comment #10)
> Sadly, I'm getting a build error due to a conflict between "Complex" typedef from WTF::Complex and Complex enum value in Font.h.
> 
> I'll reupload a patch renaming "Complex" enum value to "ComplexPath" to avoid conflict with WTF::Complex :(

My initial thinking was wrong. It is not conflicting with WTF::Complex. However, the "Complex" enum value in Font.h is conflicting with something on EFL port: I just don't know what yet.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #10)
> > Sadly, I'm getting a build error due to a conflict between "Complex" typedef from WTF::Complex and Complex enum value in Font.h.
> > 
> > I'll reupload a patch renaming "Complex" enum value to "ComplexPath" to avoid conflict with WTF::Complex :(
> 
> My initial thinking was wrong. It is not conflicting with WTF::Complex. However, the "Complex" enum value in Font.h is conflicting with something on EFL port: I just don't know what yet.

Ah, I believe I finally found it:
/usr/include/X11/X.h:#define Complex			0	/* paths may intersect */

Any proposal on how to fix this? Is renaming "Complex" to "ComplexPath" an acceptable solution for everyone?

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #10)
> > > Sadly, I'm getting a build error due to a conflict between "Complex" typedef from WTF::Complex and Complex enum value in Font.h.
> > > 
> > > I'll reupload a patch renaming "Complex" enum value to "ComplexPath" to avoid conflict with WTF::Complex :(
> > 
> > My initial thinking was wrong. It is not conflicting with WTF::Complex. However, the "Complex" enum value in Font.h is conflicting with something on EFL port: I just don't know what yet.
> 
> Ah, I believe I finally found it:
> /usr/include/X11/X.h:#define Complex            0    /* paths may intersect */
> 
> Any proposal on how to fix this? Is renaming "Complex" to "ComplexPath" an acceptable solution for everyone?

Alternatively, I could add the following to Font.h:
#ifdef Complex  // "X11/X.h" defines Complex to 0.
    #undef Complex
#endif

Any preference? I would personally prefer the #undef as it requires less change.

[Chris Dumez]

Created attachment 176303 [details]
Patch

Proposal with the #undef to fix EFL port build break and minimize the amount of changes.

[WebKit Review Bot]

Comment on attachment 176303 [details]
Patch

Clearing flags on attachment: 176303

Committed r135888: <http://trac.webkit.org/changeset/135888>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Beth Dakin]

The new test is failing on Mac. I filed https://bugs.webkit.org/show_bug.cgi?id=104954