Bug 39149 – Canvas element cannot have negative width or height

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andreas Kling]

In those cases, it should default to 300 for width and 150 for height. (HTML5 spec 4.8.11)

[Andreas Kling]

Created attachment 56140 [details]
Proposed patch

[Kenneth Rohde Christiansen]

(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)

[Andreas Kling]

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

[Eric Seidel (no email)]

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.

[Andreas Kling]

Created attachment 56199 [details]
Proposed patch v3

Updated patch, thanks Eric.

[Darin Adler]

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.

[Andreas Kling]

Comment on attachment 56199 [details]
Proposed patch v3

Removing from commit-queue due to Darin's comments.

[Kenneth Rohde Christiansen]

Comment on attachment 56199 [details]
Proposed patch v3

Good comments, setting r- to removed from pending patches.

[Eric Seidel (no email)]

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.

[Andreas Kling]

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.

[WebKit Commit Bot]

Comment on attachment 57867 [details]
Proposed patch v4

Clearing flags on attachment: 57867

Committed r62299: <http://trac.webkit.org/changeset/62299>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.