Bug 4884 - Canvas element breaks when RenderObject creation is deferred by external CSS
Summary: Canvas element breaks when RenderObject creation is deferred by external CSS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 412
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: HasReduction
: 6291 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-09-08 05:27 PDT by Martin Dittus
Modified: 2006-04-09 16:34 PDT (History)
2 users (show)

See Also:


Attachments
Three simple test cases (1.23 KB, application/octet-stream)
2005-09-08 05:29 PDT, Martin Dittus
no flags Details
patch, including detailed change log and layout test (40.72 KB, patch)
2006-04-08 20:04 PDT, Darin Adler
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Dittus 2005-09-08 05:27:59 PDT
Pages with a canvas element stop working in Safari as soon as you add an external CSS file to the page. 
The canvas is still correctly added to the page, but all drawing seems to be ignored.

This is reproducable in both 412 and 412+ builds.

I posted this to webkit-dev, here is Dave Hyatt's response: "Yeah, clearly a bug in our code because we 
defer renderobject creation until stylesheets are loaded. Firefox doesn't exhibit the bug because it blocks 
its HTML parser while waiting for stylesheets to load.  We don't."

Test case attachment follows.
Comment 1 Martin Dittus 2005-09-08 05:29:29 PDT
Created attachment 3814 [details]
Three simple test cases

Attaching three simple test cases:

test 1 - simple page with external CSS, and all canvas drawing done inline
test 2 - simple page without external CSS, and all canvas drawing done inline
test 3 - simple page with external CSS, and all canvas drawing done via
onload()

Both Safari 2.0.1 (412.5) and the most recent WebKit CVS version show
the same behavior:

test 1 doesn't work (blank canvas) 
test 2 works (canvas filled black)
test 3 works (canvas filled black)

Just to be sure I checked with a recent Firefox release, and all three
test cases worked fine (canvas filled black).
Comment 2 Joost de Valk (AlthA) 2005-12-29 07:25:10 PST
Confirmed, good testcases. Reassigning to webkit-unassigned
Comment 3 David Carson 2006-03-08 17:10:00 PST
I believe the root cause of this problem is the same as:
http://bugzilla.opendarwin.org/show_bug.cgi?id=6291
ie the render object has not been constructed yet, and as such, there is bitmap to paint onto.
Comment 4 David Carson 2006-03-08 18:27:37 PST
*** Bug 6291 has been marked as a duplicate of this bug. ***
Comment 5 David Carson 2006-03-08 18:29:29 PST
Details copied from 6291:
The problem seems to be that the Canvas render object (RenderCanvasImage) has
not been created yet, although there is a valid DOM element. By memory, render
objects are not created until they are attached to the DOM tree. I believe this
is because the engine does not know how to provide the style information needed
to create a render object until it knows it's location within the tree.

So, although the getContext function will return with a valid canvas context,
no methods on that object will succeed until it is attached to the DOM.
I am not sure what can be done about this.
Comment 6 Dave Hyatt 2006-03-08 21:51:25 PST
We basically need the capability to stall script when it ends up needing style information.  Assuming improvements do not occur in this area (i.e., the JS engine), I'll probably be changing the engine to simply block on scripts until stylesheets have fully loaded.
Comment 7 Darin Adler 2006-03-13 16:51:05 PST
Hyatt and I talked about this recently and we realized that the right way to fix this is to move the canvas bitmap buffer from the render object into the DOM object. I'll probably be doing that after I land the fix for bug 7749, although the fix for that will not resolve this.
Comment 8 Darin Adler 2006-04-08 20:04:21 PDT
Created attachment 7592 [details]
patch, including detailed change log and layout test
Comment 9 Anders Carlsson 2006-04-09 11:50:22 PDT
Comment on attachment 7592 [details]
patch, including detailed change log and layout test

While it does look like this change breaks backwards compatibility for <canvas> elements without any specified width or height, a) I don't think anyone is relying on that and b) following the standard is way better.

r=me