Bug 97333 – Implement Canvas Path object

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

[Dirk Schulze]

The HTML Canvas specification and the WHAT WG HTML specification define the Path object[1][2]. The Path and the CanvasRenderingContext2D objects share some graphics operations like: closePath, lineTo, moveTo, arc, arcTo, ellipse (not implemented yet), quadraticCurveTo, cubicCurveTo, rect [3]. But instead of an Canvas, the operations get applied to a Path object. Furthermore the object allows adding other Paths with transform. A Path object can then be drawn on a Canvas.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#path-objects
[2] http://dev.w3.org/html5/2dcontext/#path-objects
[3] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#2dcontext

[Dirk Schulze]

Created attachment 165130 [details]
Patch

Test if it builds on all systems.

[Build Bot]

Comment on attachment 165130 [details]
Patch

Attachment 165130 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13916636

[Build Bot]

Comment on attachment 165130 [details]
Patch

Attachment 165130 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13951714

[Dirk Schulze]

Created attachment 165134 [details]
Patch

Test if it builds on all systems.

[Build Bot]

Comment on attachment 165134 [details]
Patch

Attachment 165134 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13954638

[Build Bot]

Comment on attachment 165134 [details]
Patch

Attachment 165134 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13954647

New failing tests:
fast/dom/constructed-objects-prototypes.html

[Simon Fraser (smfr)]

I think we might want to wait for this API to settle down before implementing it. I don't really like the new API.

[Dirk Schulze]

(In reply to comment #7)
> I think we might want to wait for this API to settle down before implementing it. I don't really like the new API.

As a compromise, can we put the new API changes behind a flag? SVG WG decided to reuse the concept of Path for their own use. It would be extremely helpful to experiment with it. The flag could be called CANVAS_NEW or CANVAS_NEXT :) (Hope that someone has better suggestions).

[Ian 'Hixie' Hickson]

smfr: The API is entirely "settled" (there's no outstanding feedback on it, last I checked). If you would like to unsettle it, please send e-mail to the WHATWG list with your concerns! (In particular, knowing that you don't want to implement it is very valuable feedback, but knowing why not would make it much easier for us to fix it.)

[Dirk Schulze]

To the build problems: I tried a build from scratch on Mac and it works for me. Maybe the bots need to do the same.

[Mike Reed]

Can you add a comment above transformIsInvertible()? What is it here? Who would override it?

[Dirk Schulze]

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

[Dirk Schulze]

(In reply to comment #11)
> Can you add a comment above transformIsInvertible()? What is it here? Who would override it?

Not sure what you mean with "Who would override it?". The problem that some libraries are focused with is the following:

ctx.scale(0)
ctx.lineTo(100,100);

freezes the library. This was in particular the case for Cairo Graphics. For this reason it is necessary to check before applying the function and stop there. This problem might be fixed in Cairo Graphics already and can finally be removed. This is in the scope of a different patch and a different investigation.

[Dirk Schulze]

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

[Mike Reed]

What is the status of this. Are you waiting on more reviews?

[Dirk Schulze]

(In reply to comment #15)
> What is the status of this. Are you waiting on more reviews?

We don't have a consensus on how we proceed with the proposal, see comment 7.

[Mike Reed]

#7 smfr

Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?

[Dean Jackson]

(In reply to comment #17)

> Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?

Might as well go ahead, but please put this behind an ENABLE flag. It's a Web-facing feature so implementations might want to hold off on releasing it if they plan to request significant changes to the specification. I'm not aware of any, but there are other reasons such as content compatibility (e.g. the new API has a name clash with an important site) that means people might want to live on this before shipping.

[Rik Cabanier]

(In reply to comment #18)
> (In reply to comment #17)
> 
> > Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?
> 
> Might as well go ahead, but please put this behind an ENABLE flag. It's a Web-facing feature so implementations might want to hold off on releasing it if they plan to request significant changes to the specification. I'm not aware of any, but there are other reasons such as content compatibility (e.g. the new API has a name clash with an important site) that means people might want to live on this before shipping.

I think that the API should change. The current API is not all that useful and will not accomplish what people want (for instance, you can't use it to aggregate paths)

[Ian 'Hixie' Hickson]

(In reply to comment #7 and comment #19)

As always, if you want the API to change, send use cases and design constraints to the WHATWG list. I'm happy to update the API to address new use cases. (Please focus on problems that authors try to address that the current spec doesn't handle, not alternative proposed solutions, in initial e-mails on any topic.)

[Justin Novosad]

How about landing an implementation that reflects the current state of the draft specification as "webkitPath"?

[Dirk Schulze]

(In reply to comment #21)
> How about landing an implementation that reflects the current state of the draft specification as "webkitPath"?

I'll upload a new patch with the flag for canvas path first. Actually, Rik's request is not that far from the WHAT WG version, it just goes beyond it (with very good use cases). It should not be seen as counter proposal.

I am not so sure about prefixing this new object. I would rather not compile it by default on the bots until we are more certain about the API. That gives us a lot more freedom to change our mind.

[Ian 'Hixie' Hickson]

No need to prefix. I will make sure the spec doesn't conflict with shipped implementations. If changes are desired to the spec, make sure the use cases are mailed to the WHATWG list or a bug is filed.

[Dirk Schulze]

Making this to a master bug report.

[Dirk Schulze]

Created attachment 184013 [details]
Patch

[WebKit Review Bot]

Comment on attachment 184013 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16040699

[Early Warning System Bot]

Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16041740

[Build Bot]

Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16033762

[Peter Beverloo (cr-android ews)]

Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16065018

[Build Bot]

Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16037735

[EFL EWS Bot]

Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16045725

[Dirk Schulze]

Created attachment 184032 [details]
Patch

Patch. Should build on Mac now.

[Early Warning System Bot]

Comment on attachment 184032 [details]
Patch

Attachment 184032 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16033805

[Early Warning System Bot]

Comment on attachment 184032 [details]
Patch

Attachment 184032 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16063158

[Dirk Schulze]

Created attachment 184036 [details]
Patch

Patch. Should fix more builds.

[WebKit Review Bot]

Comment on attachment 184036 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html
fast/canvas/canvas-path-object.html

[WebKit Review Bot]

Comment on attachment 184036 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html
fast/canvas/canvas-path-object.html

[Dirk Schulze]

Created attachment 184070 [details]
Patch

Patch. Update build files after changes from other patches.

[WebKit Review Bot]

Comment on attachment 184070 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html

[Dirk Schulze]

Created attachment 184104 [details]
Patch

Patch.

[Dean Jackson]

Comment on attachment 184104 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE -> ADOBE ?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:39
> +void CanvasPathMethods::closePath()
> +{
> +    if (m_path.isEmpty())
> +        return;
> +

Should you check the transform here as well?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:47
> +    if (!isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:56
> +    if (!isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:65
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(p1);
> +    else if (p1 != m_path.currentPoint())
> +        m_path.addLineTo(FloatPoint(x, y));

Use p1 in addLineTo since you have it?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:70
> +    if (!isfinite(cpx) | !isfinite(cpy) | !isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:79
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(FloatPoint(cpx, cpy));
> +
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (p1 != m_path.currentPoint())
> +        m_path.addQuadCurveTo(FloatPoint(cpx, cpy), p1);

Is it defined behaviour that if (cpx, cpy) == (x, y) and you didn't have a control point then you still do the moveTo? In fact, it seems a little weird to move to the control point at all.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:84
> +    if (!isfinite(cp1x) | !isfinite(cp1y) | !isfinite(cp2x) | !isfinite(cp2y) | !isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:93
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(FloatPoint(cp1x, cp1y));
> +
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (p1 != m_path.currentPoint())
> +        m_path.addBezierCurveTo(FloatPoint(cp1x, cp1y), FloatPoint(cp2x, cp2y), p1);

Same question as for quad above.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:99
> +    if (!isfinite(x1) | !isfinite(y1) | !isfinite(x2) | !isfinite(y2) | !isfinite(r))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:114
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(p1);

Again, is this defined? In this case you're moving to the start of the arc, then not adding the arc, so subsequent calls would not draw from the end. I have no idea what is correct, but that seems weird.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:124
> +    if (!isfinite(x) | !isfinite(y) | !isfinite(r) | !isfinite(sa) | !isfinite(ea))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:133
> +        // The arc is empty but we still need to draw the connecting line

Nit: trailing .

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:141
> +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle starting/ending at 'sa'

Nit: trailing .

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:151
> +    if (anticlockwise && sa - ea >= 2 * piFloat) {
> +        m_path.addArc(FloatPoint(x, y), r, sa, sa - 2 * piFloat, anticlockwise);
> +        return;
> +    }
> +    if (!anticlockwise && ea - sa >= 2 * piFloat) {
> +        m_path.addArc(FloatPoint(x, y), r, sa, sa + 2 * piFloat, anticlockwise);
> +        return;
> +    }
> +
> +    m_path.addArc(FloatPoint(x, y), r, sa, ea, anticlockwise);

Might as well create a variable FloatPoint(x, y)

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:159
> +    if (!isfinite(x) || !isfinite(y) || !isfinite(width) || !isfinite(height))

This one is ok!! :)

> Source/WebCore/html/canvas/CanvasPathMethods.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/CanvasPathMethods.h:49
> +    void arc(float x, float y, float r, float sa, float ea, bool clockwise, ExceptionCode&);

Watch out - you name this clockwise here and anticlockwise in the implementation!

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:133
>               in [Optional=DefaultIsUndefined] boolean anticlockwise)
>          raises (DOMException);
> +
>      void fill(in [Optional] DOMString winding);

Intentional?

> Source/WebCore/html/canvas/DOMPath.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/DOMPath.idl:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/DOMPath.idl:63
> +             in [Optional=DefaultIsUndefined] boolean anticlockwise)

And anticlockwise here.

[Dirk Schulze]

Comment on attachment 184104 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:39
>> +
> 
> Should you check the transform here as well?

Very good question. I checked the spec, but couldn't find any wording what happens on a non-invertible CTM. Implementations are fairly different.

Here are two examples:
http://jsfiddle.net/Dghuh/1/
http://jsfiddle.net/Dghuh/2/

The first one does scale(0), which should make the CTM non-invertibe, we still apply lineTo and closePath for some reason. IE and FF refuse to draw anything.

The second does setTransform(0,0,0,0,0), which should reset the CTM to a zero matrix (again, not invertible). IE, Opera and FF draw a line to 0,0 and close the path afterwards (which kind of makes sense, since the universe is convoluted to one point). We refuse the lineTo command and close the path as expected.

There is clearly a need to specify this case properly. For now, I preserve our behavior.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:79
>> +        m_path.addQuadCurveTo(FloatPoint(cpx, cpy), p1);
> 
> Is it defined behaviour that if (cpx, cpy) == (x, y) and you didn't have a control point then you still do the moveTo? In fact, it seems a little weird to move to the control point at all.

"The quadraticCurveTo(cpx, cpy, x, y) method must ensure there is a subpath for (cpx, cpy)"

and later:

"When the user agent is to ensure there is a subpath for a coordinate (x, y) on a path, the user agent must check to see if the path has any subpaths, and if it does not, then the user agent must create a new subpath with the point (x, y) as its first (and only) point, as if the moveTo() method had been called."

We are following the spec, or better, the spec is following us ;)

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:93
>> +        m_path.addBezierCurveTo(FloatPoint(cp1x, cp1y), FloatPoint(cp2x, cp2y), p1);
> 
> Same question as for quad above.

Ditto: "method must ensure there is a subpath for (cp1x, cp1y)".

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:114
>> +        m_path.moveTo(p1);
> 
> Again, is this defined? In this case you're moving to the start of the arc, then not adding the arc, so subsequent calls would not draw from the end. I have no idea what is correct, but that seems weird.

Yes: " method must first ensure there is a subpath for (x1, y1)"

Definition of "ensure there is a subpath for" is above.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:151
>> +    m_path.addArc(FloatPoint(x, y), r, sa, ea, anticlockwise);
> 
> Might as well create a variable FloatPoint(x, y)

The point is never used twice. I wouldn't do it right now.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:159
>> +    if (!isfinite(x) || !isfinite(y) || !isfinite(width) || !isfinite(height))
> 
> This one is ok!! :)

Hehe, true.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:133
>>      void fill(in [Optional] DOMString winding);
> 
> Intentional?

Yes, to separate the functions that can be moved from the functions that can't.

[Dirk Schulze]

Created attachment 184279 [details]
Patch

Fixed requested changes.

[Dirk Schulze]

Committed r140604: <http://trac.webkit.org/changeset/140604>

[Dirk Schulze]

Reopen this bug report as meta bug.

[Dirk Schulze]

All pending patches landed.

[Tim Nguyen (:ntim)]

rdar://11159162