Last Comment Bug 421865 - Zero width <canvas> becomes 300pixel width wrongly
: Zero width <canvas> becomes 300pixel width wrongly
Status: VERIFIED FIXED
: dev-doc-complete, testcase
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: :Benjamin Peterson
:
:
Mentors:
Depends on: 668627 670442
Blocks: 622842
  Show dependency treegraph
 
Reported: 2008-03-10 00:03 PDT by YUKI "Piro" Hiroshi
Modified: 2011-07-20 05:21 PDT (History)
11 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (860 bytes, application/vnd.mozilla.xul+xml)
2008-03-10 00:07 PDT, YUKI "Piro" Hiroshi
no flags Details
Patch v1 (1.50 KB, patch)
2008-03-10 19:19 PDT, Takeshi Kurosawa
no flags Details | Diff | Splinter Review
stop lying in GetWidthHeight() (5.88 KB, patch)
2011-05-10 17:15 PDT, :Benjamin Peterson
joe: review+
Details | Diff | Splinter Review
more tests and remove extraneous changes (12.33 KB, patch)
2011-05-18 12:08 PDT, :Benjamin Peterson
roc: superreview+
Details | Diff | Splinter Review

Description YUKI "Piro" Hiroshi 2008-03-10 00:03:52 PDT
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 2008-03-10 00:07:39 PDT
Created attachment 308383 [details]
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 2008-03-10 19:07:40 PDT
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 2008-03-10 19:19:41 PDT
Created attachment 308543 [details] [diff] [review]
Patch v1
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2008-03-10 19:34:53 PDT
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] 2010-03-23 14:51:42 PDT
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 2011-05-10 17:15:30 PDT
Created attachment 531505 [details] [diff] [review]
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 2011-05-10 17:21:12 PDT
(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) 2011-05-18 10:15:38 PDT
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 2011-05-18 12:05:52 PDT
(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 2011-05-18 12:08:00 PDT
Created attachment 533362 [details] [diff] [review]
more tests and remove extraneous changes
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 14:35:45 PDT
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) 2011-05-19 06:20:52 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/41f93610ff0c
Comment 13 Eric Shepherd [:sheppy] 2011-05-19 12:05:45 PDT
Documentation updated:

https://developer.mozilla.org/en/HTML/Element/canvas#Browser-specific_notes
Comment 14 AndreiD[QA] 2011-07-20 02:54:15 PDT
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 2011-07-20 04:50:53 PDT
Thank you for checking. Please do not change the bug status; the correct status for fixed bugs is "Resolved Fixed".
Comment 16 AndreiD[QA] 2011-07-20 05:15:08 PDT
(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] 2011-07-20 05:20:48 PDT
(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 2011-07-20 05:21:48 PDT
Okay. Sorry for the noise.

Note You need to log in before you can comment on or make changes to this bug.