1006656 - SetLineDash with negative values doesn't react as per the spec

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[David Caldwell]

Attachment: firefox-canvas-linedash.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140428174145

Steps to reproduce:

I recently added lineDash support to a canvas implementation for node.js (https://github.com/LearnBoost/node-canvas/pull/373) and while testing I noticed that Firefox and Chrome differed in how they handle bad line values passed to setLineDash().

After reading the w3 specs, I think that Chrome is handling things correctly and Firefox is not.

I've attached a test case that illustrates the issue.


Actual results:

In the test case I see 3 solid red lines.

This means that Firefox reset the line dash setting to the default when bad values were passed to setLineDash().


Expected results:

The red lines should retain the same line dash settings as the green line above them.

The spec says (http://www.w3.org/TR/2dcontext/#line-styles):

  When the setLineDash() method is invoked, it must run the following steps:

    1. Let a be a copy of the array provided as the argument.

    2. If any value in the array is not finite (e.g. an Infinity or a NaN
       value), or if any value is negative (less than zero), then abort
       these steps (without throwing an exception; user agents could show a
       message on a developer console, though, as that would be helpful for
       debugging).

    3. If the number of elements in a is odd, then let a be the
       concatentation of two copies of a.

    4. Let the object's dash list be a.

From my reading, step 2 means that it should abort without proceeding to step 4, and so the line dash should not be changed when bad values are encountered.

Comment 1

[David Caldwell]

Attachment: Proposed Patch

I went ahead and had a go at making a patch for this. The patch is simple--it creates a new array, bailing out if it finds an invalid input. If it makes it to the end, it assigns over top of the old dash array, just like the SetMozDash() setter does it.

In fact, I even stole the float checking expression from JSValToDashArray() in CanvasUtil.h (which is what SetMozDash() uses).

After applying this patch, the test case gives me the dashed red lines that I expect.

Comment 2

[David Caldwell]

Attachment: setlinedash.patch

I've amended my patch to include a testcase.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patch looks great. However, it seems to me that this is actually a WebIDL bug; setLineDash takes a sequence<double>, so non-finite values shouldn't even reach our C++ code here. We would however have to annotate setLineDash with LenientFloat to get the correct behavior.

bz, am I right?

Comment 4

[David Caldwell]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> The patch looks great. However, it seems to me that this is actually a
> WebIDL bug; setLineDash takes a sequence<double>, so non-finite values
> shouldn't even reach our C++ code here.

Ah, I see: my original testcase tested for negative values, which make it through the WebIDL bindings (since they are finite). The subsequent tests failed, but only because the first one had failed (a poor test on my part).

So: the patch doesn't need to test for NaN or Infinity, just negative. I'll update that momentarily.

Comment 5

[David Caldwell]

Attachment: bug1006656-firefox-canvas-linedash-test.html

Attached a more correct visual test showing that only negatives behave incorrectly.

Comment 6

[David Caldwell]

Attachment: set-linedash-r2.patch

This new patch stops checking for finite in the C++ code, but still checks for negative.

I also updated the test to reset the line dash before each test so test failures don't cascade.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, brilliant. I should have known our WebIDL code was flawless :-)

Comment 8

[David Caldwell]

Attachment: set-linedash.patch

This is the same as the last patch, but has the required check-in comment and author name, ready for committing.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, I already checked in with my own checkin comment (but your name of course):

https://hg.mozilla.org/integration/mozilla-inbound/rev/d18ebc644b89

Thanks!!!

Comment 10

[David Caldwell]

No worries. Thanks!

Comment 11

[Carsten Book [:Tomcat]]

clearing the checkin flag since roc did the checkin :) Also David thanks for contributing to Mozilla!

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d18ebc644b89