Bug 13559 - REGRESSION: Canvas aspect ratio is incorrect
Summary: REGRESSION: Canvas aspect ratio is incorrect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL: http://hixie.ch/tests/adhoc/perf/vide...
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2007-05-01 03:06 PDT by Alexey Proskuryakov
Modified: 2007-05-01 15:32 PDT (History)
1 user (show)

See Also:


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-
Details | Formatted Diff | Diff
patch -- passes all layout tests (33.81 KB, patch)
2007-05-01 13:50 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch incorporating Mitz's feedback, passes layout tests (33.29 KB, patch)
2007-05-01 13:54 PDT, Darin Adler
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 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.
Comment 1 Darin Adler 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.
Comment 2 Darin Adler 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.
Comment 3 Darin Adler 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.
Comment 4 Darin Adler 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.
Comment 5 mitz 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.
Comment 6 Darin Adler 2007-05-01 10:38:53 PDT
Created attachment 14297 [details]
patch -- passes all layout tests
Comment 7 mitz 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.
Comment 8 Darin Adler 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.
Comment 9 Darin Adler 2007-05-01 13:50:30 PDT
Created attachment 14299 [details]
patch -- passes all layout tests
Comment 10 Darin Adler 2007-05-01 13:54:08 PDT
Created attachment 14300 [details]
patch incorporating Mitz's feedback, passes layout tests
Comment 11 Dave Hyatt 2007-05-01 15:12:52 PDT
Comment on attachment 14300 [details]
patch incorporating Mitz's feedback, passes layout tests

r=me
Comment 12 Darin Adler 2007-05-01 15:32:56 PDT
checked in r21211