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

If the canvas has a horizontal dimension of zero, then toDataURL() must return the string "data:,".

VERIFIED FIXED in mozilla6

Status

()

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

People

(Reporter: khuey, Assigned: Benjamin)

Tracking

(Blocks: 1 bug)

Trunk
mozilla6
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ietestcenter, URL)

Attachments

(1 attachment, 2 obsolete attachments)

1.11 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
At least one problem here: when canvas.width=0 is called we don't actually set the width to 0 ... see http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLCanvasElement.cpp#121
Version: unspecified → Trunk
If I change the clamp from <= to < we crash in Cairo while trying to get an inputstream from the context.
The check was added by bug 291216.
(Assignee)

Comment 4

6 years ago
Created attachment 527714 [details] [diff] [review]
patch to fix the issue

Here is a patch and test.
Attachment #527714 - Flags: review?
Comment on attachment 527714 [details] [diff] [review]
patch to fix the issue

Thanks for the patch Benjamin.

Joe, can you review this?  Not sure who reviews canvas stuff these days ...
Attachment #527714 - Flags: review? → review?(joe)
Comment on attachment 527714 [details] [diff] [review]
patch to fix the issue

Review of attachment 527714 [details] [diff] [review]:

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +124,3 @@
     size.width = DEFAULT_CANVAS_WIDTH;
+  if (size.height < 0)
+    size.height = DEFAULT_CANVAS_HEIGHT;

I think I would rather GetWidthHeightInternal didn't do any frobbing of the returned data; if users want to get the raw data, they should be ready to handle silly results.

(This would also imply changing GetWidthHeight to contain <= rather than ==.)

@@ +360,5 @@
+    nsIntSize size = GetWidthHeightInternal();
+    if (size.height == 0 || size.width == 0) {
+      aDataURL = NS_LITERAL_STRING("data:,");
+      return NS_OK;
+    }

Similarly, here we'd change this to <= 0.
Attachment #527714 - Flags: review?(joe) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 530456 [details] [diff] [review]
fix the issue

This addresses review comments. However, it does not change "width == 0" to "width <= 0" because I don't think a negative value can get past ParseNonNegativeIntValue in ParseAttribute(). Correct me if I'm wrong.
Attachment #527714 - Attachment is obsolete: true
Attachment #530456 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #530456 - Flags: review? → review?(joe)
Comment on attachment 530456 [details] [diff] [review]
fix the issue

's fine with me! :)
Attachment #530456 - Flags: review?(joe) → review+
I don't think we should do this until we fix bug 421865.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
http://www.w3c-test.org/html/tests/approved/canvas/toDataURL.zerosize.html
Assignee: benjamin → nobody
Blocks: 622842
Status: ASSIGNED → NEW
OS: Windows 7 → All
Hardware: x86 → All

Updated

6 years ago
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
(Assignee)

Comment 11

6 years ago
Created attachment 531507 [details] [diff] [review]
fix based on 421865

This simple patch (requiring the the patch to 421865) fixes the issue.
Attachment #530456 - Attachment is obsolete: true
Attachment #531507 - Flags: review?(joe)
Comment on attachment 531507 [details] [diff] [review]
fix based on 421865

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

Love it.
Attachment #531507 - Flags: review?(joe) → review+
(Assignee)

Updated

6 years ago
Depends on: 421865
(Assignee)

Updated

6 years ago
No longer depends on: 421865
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: ietestcenter → ietestcenter [fixed in cedar]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/951de6cc1fdb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: ietestcenter [fixed in cedar] → ietestcenter
Target Milestone: --- → mozilla6
Thanks for fixing this Benjamin.

Comment 15

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

The test toDataURL on w3c-test.org (Comment 10) is PASS, as well as the test at the link:
http://samples.msdn.microsoft.com/ietestcenter/html5/canvas_harness.htm?url=canvas-canvasElement-001
Also the depending bug 421865 has been fixed
Status: RESOLVED → VERIFIED
(Assignee)

Updated

6 years ago
Status: VERIFIED → RESOLVED
Last Resolved: 6 years ago6 years ago
Thanks for the verification, Andrei.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.