In those cases, it should default to 300 for width and 150 for height. (HTML5 spec 4.8.11)
Created attachment 56140 [details] Proposed patch
(In reply to comment #0) > In those cases, it should default to 300 for width and 150 for height. (HTML5 spec 4.8.11) link? It is useful with a link as the spec is work in progress and there are thus multiple versions (Editor Draft, etc)
Created attachment 56176 [details] Proposed patch v2 Updated patch with spec link in ChangeLogs: http://www.whatwg.org/specs/web-apps/current-work/#attr-canvas-width
Comment on attachment 56176 [details] Proposed patch v2 WebCore/html/HTMLCanvasElement.cpp:116 + setAttribute(heightAttr, String::number(DefaultHeight)); I would have probably used a ternary here. Something like: int height = (value >= 0) ? value : DefaultHeight); Otherwise the code and test look great.
Created attachment 56199 [details] Proposed patch v3 Updated patch, thanks Eric.
Comment on attachment 56199 [details] Proposed patch v3 I think this patch is not entirely correct or at least not complete. The code in HTMLCanvasElement::reset seems fine, but the changes to HTMLCanvasSetElement::setHeight and setWidth are either wrong or incomplete. > void HTMLCanvasElement::setHeight(int value) > { > - setAttribute(heightAttr, String::number(value)); > + int height = (value >= 0) ? value : DefaultHeight; > + setAttribute(heightAttr, String::number(height)); > } In other similar call sites, I've seen behavior where attempting to set to an illegal value leaves the attribute alone rather than setting it to a default value. To test this we'd want to set the height attribute of a canvas to something other than the default and see what an attempt to change it to a negative number does in that case. It also seems strange and probably wrong that this would add a height attribute of "150" to a canvas element that previously had no height attribute of all. I'd also like to know what happens if we attempt to set the height attribute to a non-numeric string. Both by assigning that value to height and using the setAttribute function. Also, what happens if we set a canvas's height attribute to a negative number using the setAttribute function. I'd also like to see test cases where we set height to NaN and infinity to see how those cases should be handled. And hear what canvas implementations in other browser engines do in all those cases.
Comment on attachment 56199 [details] Proposed patch v3 Removing from commit-queue due to Darin's comments.
Comment on attachment 56199 [details] Proposed patch v3 Good comments, setting r- to removed from pending patches.
Comment on attachment 56176 [details] Proposed patch v2 Cleared Eric Seidel's review+ from obsolete attachment 56176 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 57867 [details] Proposed patch v4 Updated patch with additional test coverage. The spec isn't too explicit on how NaN/etc should be handled. NaN/Infinity/null becomes 0 on Firefox and 300/150 (default value) on Opera. Opera OTOH doesn't allow 0 width/height. Personally, while I think mapping NaN/Infinity/null to the default values makes some sense, it doesn't justify adding custom JS bindings to deal with these cases.
Comment on attachment 57867 [details] Proposed patch v4 Clearing flags on attachment: 57867 Committed r62299: <http://trac.webkit.org/changeset/62299>
All reviewed patches have been landed. Closing bug.