421865 - Zero width <canvas> becomes 300pixel width wrongly

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[YUKI "Piro" Hiroshi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030904 Minefield/3.0b5pre

If we set the width of <canvas> element (which is HTML Canvas) to 0, it becomes 300pixel wrongly.

Reproducible: Always

Steps to Reproduce:
1. create <canvas> element.
2. do "canvas.setAttribute('width', 0)", "canvas.setAttribute('style', 'width:0')", "canvas.width = 0" or "canvas.style.width = 0"

Actual Results:  
<canvas> becomes 300pixel width.

Expected Results:  
<canvas> becomes zero width.

Comment 1

[YUKI "Piro" Hiroshi]

Attachment: testcase

1. Click "set width to 0" button.

Actual result:
The red box becomes a horizontal line. It is 300px * 0px box.

Expected result:
The red box becomes a vertical line. It is 0px * 150px box.

Comment 2

[YUKI "Piro" Hiroshi]

In the  implementation, width and height are set to their default value if they are equal or smaller than 0.

http://mxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLCanvasElement.cpp#184
> if (size.width <= 0)
> size.width = DEFAULT_CANVAS_WIDTH;
> if (size.height <= 0)
> size.height = DEFAULT_CANVAS_HEIGHT;

But, in the definition of HTML5 specification, 0 is also a valid value for width and height.

http://www.whatwg.org/specs/web-apps/current-work/multipage/section-the-canvas.html#width0
> The canvas element has two attributes to control the size of the coordinate
> space: width and height. These attributes, when specified, must have values
> that are valid non-negative integers.

http://www.whatwg.org/specs/web-apps/current-work/multipage/section-common1.html#valid
> A string is a valid non-negative integer if it consists of one of more
> characters in the range U+0030 DIGIT ZERO (0) to U+0039 DIGIT NINE (9).

Comment 3

[Takeshi Kurosawa :takenspc]

Attachment: Patch v1

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, this scares me a little, as 0-sized surfaces have been known to cause weird things in cairo.  We'd need to create a bunch of mochitests for this, I think, to make sure that things like get/putImageData and stuff all work correctly.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 308543 [details] [diff] [review]
Patch v1

Clearing this review (sorry that it's so old!) -- this needs a bunch of tests added along with it to make sure things actually work with 0-sized canvases.

Comment 6

[:Benjamin Peterson]

Attachment: stop lying in GetWidthHeight()

This fixes the issue by telling the truth. To avoid problems with zero-width surfaces, a 1 by 1 context is created for zero canvases.

Comment 7

[:Benjamin Peterson]

(In reply to comment #6)
> Created attachment 531505 [details] [diff] [review] [review]
> stop lying in GetWidthHeight()
> 
> This fixes the issue by telling the truth.

Telling the truth in GetWidthHeight() that is.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 531505 [details] [diff] [review]
stop lying in GetWidthHeight()

Review of attachment 531505 [details] [diff] [review]:
-----------------------------------------------------------------

I'm only slightly nervous about this patch, which is good!

I think we need bug 649618 to be fixed along with this, because otherwise toDataURL is going to give us a 1x1 image when the canvas is 0x0. Luckily, you're working on that. :)

Can you also ensure we have a test (or write a test) for:
 - making sure a 0x0 canvas reports its size as 0x0 (a unit test, written in js - make sure that the numbers returned are actually 0)
 - making sure getImageData returns all transparent black for a 0x0 canvas

I'd like roc (or his designate) to look at this too, to make sure I'm not making any web-visible mistakes in the review here.

::: content/canvas/test/test_canvas.html
@@ +24522,1 @@
>    test_toDataURL_zerosize();

Why remove the try/catch here? I presume it's there to ensure an exception doesn't abort the whole canvas test suite.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +229,4 @@
>                                   PRUint32& aSize,
>                                   bool& aFellBackToPNG)
>  {
> +  nsIntSize size = GetWidthHeight();

Why did you move this line?

Comment 9

[:Benjamin Peterson]

(In reply to comment #8)
> Comment on attachment 531505 [details] [diff] [review] [review]
> stop lying in GetWidthHeight()
> 
> Review of attachment 531505 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> I'm only slightly nervous about this patch, which is good!
> 
> I think we need bug 649618 to be fixed along with this, because otherwise
> toDataURL is going to give us a 1x1 image when the canvas is 0x0. Luckily,
> you're working on that. :)
> 
> Can you also ensure we have a test (or write a test) for:
>  - making sure a 0x0 canvas reports its size as 0x0 (a unit test, written in
> js - make sure that the numbers returned are actually 0)
>  - making sure getImageData returns all transparent black for a 0x0 canvas

I'll add these tests in the next patch.

> 
> I'd like roc (or his designate) to look at this too, to make sure I'm not
> making any web-visible mistakes in the review here.
> 
> ::: content/canvas/test/test_canvas.html
> @@ +24522,1 @@
> >    test_toDataURL_zerosize();
> 
> Why remove the try/catch here? I presume it's there to ensure an exception
> doesn't abort the whole canvas test suite.

This was left from some debugging I was doing in test_canvas.html.

> 
> ::: content/html/content/src/nsHTMLCanvasElement.cpp
> @@ +229,4 @@
> >                                   PRUint32& aSize,
> >                                   bool& aFellBackToPNG)
> >  {
> > +  nsIntSize size = GetWidthHeight();
> 
> Why did you move this line?

I'll undo it. I think I had modified that part of the code before moving elsewhere.

Comment 10

[:Benjamin Peterson]

Attachment: more tests and remove extraneous changes

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 533362 [details] [diff] [review]
more tests and remove extraneous changes

Review of attachment 533362 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 12

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/41f93610ff0c

Comment 13

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/HTML/Element/canvas#Browser-specific_notes

Comment 14

[AndreiD[QA]]

Verified fixed on:

-> Linux: Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0
-> Mac: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0
-> Windows: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

If you open test.xul (attached) and click on the "set width to 0" button the red box becomes a vertical line, as expected (comment 1)

Comment 15

[:Benjamin Peterson]

Thank you for checking. Please do not change the bug status; the correct status for fixed bugs is "Resolved Fixed".

Comment 16

[AndreiD[QA]]

(In reply to comment #15)
> Thank you for checking. Please do not change the bug status; the correct
> status for fixed bugs is "Resolved Fixed".

I was going through the fixed bugs for FX6 ("mozilla6" set as target milestone). Link here: http://bit.ly/py2wjd.
These bugs, from the QA point of view, have to be verified ("Verified" status).
I'll leave this as resolved, if this is the correct way for you. Thanks for the notice.

Comment 17

[AndreiD[QA]]

(In reply to comment #15)
> Thank you for checking. Please do not change the bug status; the correct
> status for fixed bugs is "Resolved Fixed".

Also, I'm referring to this https://bugzilla.mozilla.org/page.cgi?id=fields.html#status -> for the Verified status
Thanks!

Comment 18

[:Benjamin Peterson]

Okay. Sorry for the noise.