YUKI "Piro" Hiroshi Reporter
	Description • 17 years ago

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.

	YUKI "Piro" Hiroshi Reporter
	Comment 1 • 17 years ago

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.

	YUKI "Piro" Hiroshi Reporter
	Comment 2 • 17 years ago

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

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 4 • 17 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 5 • 15 years ago

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.

	:Benjamin Peterson Assignee
	Comment 6 • 13 years ago

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.

	:Benjamin Peterson Assignee
	Comment 7 • 13 years ago

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

	Joe Drew (not getting mail)
	Comment 8 • 13 years ago

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?

	:Benjamin Peterson Assignee
	Comment 9 • 13 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 13 years ago

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

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

	Mounir Lamouri (:mounir)
	Comment 12 • 13 years ago

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

	Eric Shepherd [:sheppy]
	Comment 13 • 13 years ago

Documentation updated:

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

	AndreiD[QA]
	Comment 14 • 13 years ago

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)

	:Benjamin Peterson Assignee
	Comment 15 • 13 years ago

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

	AndreiD[QA]
	Comment 16 • 13 years ago

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

	AndreiD[QA]
	Comment 17 • 13 years ago

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

	:Benjamin Peterson Assignee
	Comment 18 • 13 years ago

Okay. Sorry for the noise.