Bug 63933 – HTML canvas strokes with dash and dashOffset

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Dirk Schulze]

Mozilla implemented canvas.mozDash and .mozDashOffset[1] (first on gets float, second one float[]). I think that we should do the same. I can imagine that a lot of people are just waiting for this API, just search for 'canvas stroke dash'. DashArray support is already in GraphicsContext, shouldn't be to hard to implement it in CanvasRenderingContext2D. Any thoughts?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=662038

[Andreas Røsdal]

I would really like to see support for dashed lined in Webkit. I'm already using canvas.mozDash in http://www.freeciv.net and would like to see support for this in Webkit also.  Thanks!

[Young Han Lee]

I just went over the classes which are related to this bug. As you said, it seems not to be hard so much. I will try to upload a patch for this in a few days :)

[Young Han Lee]

Created attachment 108026 [details]
Patch

[Young Han Lee]

Comment on attachment 108026 [details]
Patch

patch for discussion (can be applied).

[WebKit Review Bot]

Comment on attachment 108026 [details]
Patch

Attachment 108026 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9774332

[Young Han Lee]

Created attachment 108028 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108028 [details]
Patch

Attachment 108028 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9763347

[Young Han Lee]

Created attachment 108167 [details]
Patch

[Young Han Lee]

Comment on attachment 108167 [details]
Patch

This patch adds dash-attributes for JSC only.

After this patch is landed, I will implement these attributes for V8 as a follow-up patch.

[Young Han Lee]

I already uploaded a patch for this, but it seems to be a matter that needs a consensus to add APIs like this.

Should I open up a discussion through webkit-dev?

[Dirk Schulze]

Comment on attachment 108167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108167&action=review

Great start!

> LayoutTests/ChangeLog:12
> +        webkitLineDash is float[] that represents a dash-pattern of stroke.
> +        (e.g. [3, 2, 1, 4] means "on 3 units, off 2 units, on 1 units and off 4 units")

Ok, you are missing a lot of different tests. First I'd like a test with numbers > max of float. I'd also like to have a test with negative values (in different orders) and of course none-weel formed arguments '[0, 0b]' '[a, b]' and so on. A lot more stress testing.

> Source/WebCore/ChangeLog:14
> +        Add webkitLineDash and webkitLineDashOffset attributes to CanvasRenderingContext2D for JSC.
> +        These attributes can be used to determine the dash-style of stroke.
> +
> +        webkitLineDash is float[] that represents a dash-pattern of stroke.
> +        (e.g. [3, 2, 1, 4] means "on 3 units, off 2 units, on 1 units and off 4 units")
> +
> +        webkitLineDashOffset is float that is offset of the dash-pattern at which stroking would start.

Hm. It is not specified yet (as far as I know). So you have to explain why you use webkitLineDashOffset, webkitLineDash where the values come from and how it was designed. I would be extremely helpful if you can mention the discussion on the mozilla bug report and also link to this report in the change log.

It might be a good idea to explain your plans on the webkit-dev mailing list. Especially because it is not specified yet.

[Young Han Lee]

Created attachment 108635 [details]
Patch

[Young Han Lee]

> Ok, you are missing a lot of different tests. First I'd like a test with numbers > max of float. I'd also like to have a test with negative values (in different orders) and of course none-weel formed arguments '[0, 0b]' '[a, b]' and so on. A lot more stress testing.

I added another test, canvas-webkitLineDash-invalid.html, for the invalid arguments.


> Hm. It is not specified yet (as far as I know). So you have to explain why you use webkitLineDashOffset, webkitLineDash where the values come from and how it was designed. I would be extremely helpful if you can mention the discussion on the mozilla bug report and also link to this report in the change log.
> 
> It might be a good idea to explain your plans on the webkit-dev mailing list. Especially because it is not specified yet.

Yep, I rewrote the changeLogs. Should it be more detailed?

[Dirk Schulze]

Comment on attachment 108635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108635&action=review

I'd like to have simon or another reviewer to check the code as well. Just some notes from me first.

The Changelog looks ok, haven't looked at the tests so far.

> Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:182
> +        if (!(elem > 0 && elem <= FLT_MAX))

is the if (elem <= 0) return; check not enough? Don't you want to raise a DOM exception on false values? setDOMException(exec, ec);

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:526
> +    if (!(offset > 0 && offset <= FLT_MAX))

First, why is a negative offset not allowed? SVG spec allows that. 2nd: Please use std::numeric_limits<float>::max() instead of FLT_MAX. IIRC you don't need to check for FLT_MAX, but you need to check for infinite. Just use the same code like for setLineWidth here.

[Young Han Lee]

> is the if (elem <= 0) return; check not enough?

No. Infinite and NaN should be checked here. "if (elem <= 0 || !isfinite(elem)) return;" would be enough.


> Don't you want to raise a DOM exception on false values? setDOMException(exec, ec);

I believe we don't have to raise an exception here for the following reasons.

1. There seems to be no attribute raising an exception for false value. All DOM exception is raised only by methods, not attributes.

2. Setting lineWidth with false value isn't raising an exception. I think webkitLineDashOffset shouldn't also raise an exception for consistency. If so, it seems weird that webkitLineDash with false value raises an exception, but webkitLineDashOffset doesn't.

[Young Han Lee]

Created attachment 108825 [details]
Patch

[WebKit Review Bot]

Attachment 108825 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu>
ERROR: Line:3792 Path does not exist. ast/canvas/webgl/premultiplyalpha-test.html
LayoutTests/platform/chromium/test_expectations.txt:3792:  Path does not exist. ast/canvas/webgl/premultiplyalpha-test.html  [test/expectations] [2]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Young Han Lee]

This style-error is caused by the typo in test_expectations.txt[1], not by this patch.

[1] http://trac.webkit.org/changeset/96090/trunk/LayoutTests/platform/chromium/test_expectations.txt

[Simon Fraser (smfr)]

Comment on attachment 108825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108825&action=review

Do you plan to do rendering tests in another pass?

> Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:181
> +        float elem = valueArray->getIndex(i).toFloat(exec);

What about array elements that are not floats? Your testcase should test ctx.webkitLineDash['foo', {}] or something.

[Young Han Lee]

> Do you plan to do rendering tests in another pass?

Yes, I will add pixel tests in the follow-up patch. So my plan is as follows:

1. patch for JS with basic tests.
2. patch for V8.
3. pixel tests.


> > Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:181
> > +        float elem = valueArray->getIndex(i).toFloat(exec);
> 
> What about array elements that are not floats? Your testcase should test ctx.webkitLineDash['foo', {}] or something.

Non-floats can't pass the "!isfinite(elem)" check, and my testcase already has the test statement for a string argument;

shouldBe("trySettingLineDash([1, 'string'])", "[1.5, 2.5]");

[Young Han Lee]

Can someone cq+ this patch?

I can't upload follow-up patches until the patch is landed.

[WebKit Review Bot]

Comment on attachment 108825 [details]
Patch

Clearing flags on attachment: 108825

Committed r96626: <http://trac.webkit.org/changeset/96626>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ian 'Hixie' Hickson]

*** Bug 26331 has been marked as a duplicate of this bug. ***

[noel gordon]

Chrome would be a simple matter of bindings; make those use the GraphicsContext line stroker used for SVG dashed paths.  For complex paths, the interaction of line-joins and path shapes lead to interesting compat issues.  knots.svg attached.

[noel gordon]

Created attachment 130963 [details]
complex paths and line joins svg

[noel gordon]

Created attachment 130964 [details]
knots.svg results on 10.6 mac-osx -- chrome 19 dev, firefox 10.0.2, safari 5.1.2

[Simon Fraser (smfr)]

Noel: if you think there's an issue, please file a new bug.

[noel gordon]

Done, bug 80674.