Bug 13559

Summary: REGRESSION: Canvas aspect ratio is incorrect
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: DOMAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz
Priority: P1 Keywords: Regression
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
URL: http://hixie.ch/tests/adhoc/perf/video/002.html
Attachments:
Description Flags
Layout test
none
patch -- passes all layout tests
darin: review-
patch -- passes all layout tests
none
patch incorporating Mitz's feedback, passes layout tests hyatt: review+

Alexey Proskuryakov
Reported 2007-05-01 03:06:34 PDT
The animation in this test is rendered correctly by Firefox and shipping Safari, but is horizontally stretched in TOT. Seems to work correctly when cached, but reproducible again after a reload.
Attachments
Layout test (366 bytes, text/html)
2007-05-01 10:16 PDT, mitz 		no flags
Details
patch -- passes all layout tests (34.29 KB, patch)
2007-05-01 10:38 PDT, Darin Adler 		darin: review-
DetailsFormatted DiffDiff
patch -- passes all layout tests (33.81 KB, patch)
2007-05-01 13:50 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
patch incorporating Mitz's feedback, passes layout tests (33.29 KB, patch)
2007-05-01 13:54 PDT, Darin Adler 		hyatt: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2007-05-01 07:58:32 PDT
For some reason the width and height attributes of the canvas element are not changing the width and height -- the canvas dimensions instead match the default canvas width and height.
Darin Adler
Comment 2 2007-05-01 08:01:47 PDT
There's a race condition here. When I set a breakpoint at HTMLCanvasElement::reset, the page worked fine. I think the issue is that the reset function needs to do something to trigger layout.
Darin Adler
Comment 3 2007-05-01 08:05:24 PDT
I think that HTMLCanvasElement::reset needs code that's more like what's in RenderImage::imageChanged. It needs to call setNeedsLayout(true) and setPrefWidthsDirty(true) at least in some cases. And maybe calcWidth and calcHeight too.
Darin Adler
Comment 4 2007-05-01 09:59:30 PDT
I've got a fix, but I could use a reduced test case to check in as a layout test.
mitz
Comment 5 2007-05-01 10:16:00 PDT
Created attachment 14296 [details] Layout test (In reply to comment #4) > I've got a fix, but I could use a reduced test case to check in as a layout > test. > This should work.
Darin Adler
Comment 6 2007-05-01 10:38:53 PDT
Created attachment 14297 [details] patch -- passes all layout tests
mitz
Comment 7 2007-05-01 11:03:59 PDT
(In reply to comment #3) > I think that HTMLCanvasElement::reset needs code that's more like what's in > RenderImage::imageChanged. It needs to call setNeedsLayout(true) and > setPrefWidthsDirty(true) at least in some cases. And maybe calcWidth and > calcHeight too. > The RenderImage code is a bad example. See <http://bugs.webkit.org/show_bug.cgi?id=9276#c3>. Looks like with your patch, RenderHTMLCanvas would still have the bad behavior RenderImage currently has.
Darin Adler
Comment 8 2007-05-01 11:21:28 PDT
Comment on attachment 14297 [details] patch -- passes all layout tests Mitz says review-. I'll make a new version addressing his comment.
Darin Adler
Comment 9 2007-05-01 13:50:30 PDT
Created attachment 14299 [details] patch -- passes all layout tests
Darin Adler
Comment 10 2007-05-01 13:54:08 PDT
Created attachment 14300 [details] patch incorporating Mitz's feedback, passes layout tests
Dave Hyatt
Comment 11 2007-05-01 15:12:52 PDT
Comment on attachment 14300 [details] patch incorporating Mitz's feedback, passes layout tests r=me
Darin Adler
Comment 12 2007-05-01 15:32:56 PDT
checked in r21211
Note You need to log in before you can comment on or make changes to this bug.