Zero width <canvas> becomes 300pixel width wrongly

VERIFIED FIXED in mozilla6

Status

()

defect
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: yuki, Assigned: Benjamin)

Tracking

(Depends on 1 bug, Blocks 1 bug, {dev-doc-complete, testcase})

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Posted file 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.
Version: unspecified → Trunk
Component: General → Layout: Canvas
Keywords: testcase
Product: Firefox → Core
QA Contact: general → layout.canvas
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).
Posted patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → taken.spc
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #308543 - Flags: review?(vladimir)
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 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.
Attachment #308543 - Flags: review?(vladimir)
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.
Attachment #531505 - Flags: review?
Attachment #531505 - Flags: review? → review?(joe)
(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.
Assignee: taken.spc → benjamin
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?
Attachment #531505 - Flags: superreview?(roc)
Attachment #531505 - Flags: review?(joe)
Attachment #531505 - Flags: review+
Blocks: 649618
(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.
No longer blocks: 649618
Attachment #531505 - Attachment is obsolete: true
Attachment #533362 - Flags: superreview?(roc)
Attachment #531505 - Flags: superreview?(roc)
Comment on attachment 533362 [details] [diff] [review]
more tests and remove extraneous changes

Review of attachment 533362 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #533362 - Flags: superreview?(roc) → superreview+
Keywords: checkin-needed
Attachment #308543 - Attachment is obsolete: true
OS: Windows XP → All
Hardware: x86 → All
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/41f93610ff0c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla6
Depends on: 668627
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)
Status: RESOLVED → VERIFIED
Status: VERIFIED → RESOLVED
Closed: 8 years ago8 years ago
Status: RESOLVED → VERIFIED
Thank you for checking. Please do not change the bug status; the correct status for fixed bugs is "Resolved Fixed".
Status: VERIFIED → RESOLVED
Closed: 8 years ago8 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.
(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!
Status: RESOLVED → VERIFIED
Okay. Sorry for the noise.
You need to log in before you can comment on or make changes to this bug.