Bug 103392

Summary: Canvas does not draw any text if the font is not fully loaded yet
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bdakin, darin, d-r, eric, fmalita, haraken, hausmann, kenneth, mitz, ojan, pdr, schenney, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html
Bug Depends on:    
Bug Blocks: 87355    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2012-11-27 04:23:23 PST
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.
Comment 1 Chris Dumez 2012-11-27 04:38:45 PST
Created attachment 176232 [details]
Patch
Comment 2 Chris Dumez 2012-11-27 04:40:10 PST
Created attachment 176234 [details]
Patch

Fix typo in bug title.
Comment 3 Mikhail Pozdnyakov 2012-11-27 04:45:25 PST
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?
Comment 4 Chris Dumez 2012-11-27 04:57:09 PST
Created attachment 176237 [details]
Patch

Converted test into a ref test as advised by Dominik offline.
Comment 5 Kenneth Rohde Christiansen 2012-11-27 04:59:07 PST
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
Comment 6 Kenneth Rohde Christiansen 2012-11-27 05:00:45 PST
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?
Comment 7 Chris Dumez 2012-11-27 05:02:38 PST
(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.
Comment 8 Chris Dumez 2012-11-27 05:03:23 PST
(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 :)
Comment 9 Chris Dumez 2012-11-27 06:38:12 PST
Created attachment 176255 [details]
Patch
Comment 10 Chris Dumez 2012-11-27 07:28:04 PST
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 :(
Comment 11 Chris Dumez 2012-11-27 07:28:32 PST
Created attachment 176262 [details]
Patch
Comment 12 Chris Dumez 2012-11-27 08:32:56 PST
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.
Comment 13 Chris Dumez 2012-11-27 10:05:49 PST
(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.
Comment 14 Chris Dumez 2012-11-27 10:12:05 PST
(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?
Comment 15 Chris Dumez 2012-11-27 10:14:52 PST
(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.
Comment 16 Chris Dumez 2012-11-27 10:54:29 PST
Created attachment 176303 [details]
Patch

Proposal with the #undef to fix EFL port build break and minimize the amount of changes.
Comment 17 WebKit Review Bot 2012-11-27 11:26:42 PST
Comment on attachment 176303 [details]
Patch

Clearing flags on attachment: 176303

Committed r135888: <http://trac.webkit.org/changeset/135888>
Comment 18 WebKit Review Bot 2012-11-27 11:26:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Beth Dakin 2012-12-13 14:32:27 PST
The new test is failing on Mac. I filed https://bugs.webkit.org/show_bug.cgi?id=104954