Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Zero width <canvas> becomes 300pixel width wrongly

VERIFIED FIXED in mozilla6

Status

()

Core
Canvas: 2D
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: YUKI "Piro" Hiroshi, Assigned: Benjamin)

Tracking

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

Trunk
mozilla6
dev-doc-complete, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 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.
(Reporter)

Comment 1

10 years ago
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.
(Reporter)

Updated

10 years ago
Version: unspecified → Trunk

Updated

10 years ago
Component: General → Layout: Canvas
Keywords: testcase
Product: Firefox → Core
QA Contact: general → layout.canvas
(Reporter)

Comment 2

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

Comment 3

10 years ago
Created attachment 308543 [details] [diff] [review]
Patch v1
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)

Updated

7 years ago
Blocks: 622842
Keywords: dev-doc-needed
(Assignee)

Comment 6

6 years ago
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.
Attachment #531505 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #531505 - Flags: review? → review?(joe)
(Assignee)

Comment 7

6 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.
(Assignee)

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
Blocks: 649618
(Assignee)

Comment 9

6 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.
No longer blocks: 649618
(Assignee)

Comment 10

6 years ago
Created attachment 533362 [details] [diff] [review]
more tests and remove extraneous changes
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+
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla6
Documentation updated:

https://developer.mozilla.org/en/HTML/Element/canvas#Browser-specific_notes
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 668627
Depends on: 670442

Comment 14

6 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)
Status: RESOLVED → VERIFIED
(Assignee)

Updated

6 years ago
Status: VERIFIED → RESOLVED
Last Resolved: 6 years ago6 years ago

Updated

6 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Comment 15

6 years ago
Thank you for checking. Please do not change the bug status; the correct status for fixed bugs is "Resolved Fixed".
Status: VERIFIED → RESOLVED
Last Resolved: 6 years ago6 years ago

Comment 16

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

Comment 17

6 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!
Status: RESOLVED → VERIFIED
(Assignee)

Comment 18

6 years ago
Okay. Sorry for the noise.
You need to log in before you can comment on or make changes to this bug.