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
Created attachment 165130 [details] Patch Test if it builds on all systems.
Comment on attachment 165130 [details] Patch Attachment 165130 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13916636
Comment on attachment 165130 [details] Patch Attachment 165130 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13951714
Created attachment 165134 [details] Patch Test if it builds on all systems.
Comment on attachment 165134 [details] Patch Attachment 165134 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13954638
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
I think we might want to wait for this API to settle down before implementing it. I don't really like the new API.
(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).
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.)
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.
Can you add a comment above transformIsInvertible()? What is it here? Who would override it?
*** Bug 82790 has been marked as a duplicate of this bug. ***
(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.
*** Bug 96826 has been marked as a duplicate of this bug. ***
What is the status of this. Are you waiting on more reviews?
(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.
#7 smfr Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?
(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.
(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)
(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.)
How about landing an implementation that reflects the current state of the draft specification as "webkitPath"?
(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.
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.
Making this to a master bug report.
Created attachment 184013 [details] Patch
Comment on attachment 184013 [details] Patch Attachment 184013 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16031730
Comment on attachment 184013 [details] Patch Attachment 184013 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16040699
Comment on attachment 184013 [details] Patch Attachment 184013 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16041740
Comment on attachment 184013 [details] Patch Attachment 184013 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16033762
Comment on attachment 184013 [details] Patch Attachment 184013 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16065018
Comment on attachment 184013 [details] Patch Attachment 184013 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16037735
Comment on attachment 184013 [details] Patch Attachment 184013 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16045725
Created attachment 184032 [details] Patch Patch. Should build on Mac now.
Comment on attachment 184032 [details] Patch Attachment 184032 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16033805
Comment on attachment 184032 [details] Patch Attachment 184032 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16063158
Created attachment 184036 [details] Patch Patch. Should fix more builds.
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
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
Created attachment 184070 [details] Patch Patch. Update build files after changes from other patches.
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
Created attachment 184104 [details] Patch Patch.
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.
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.
Created attachment 184279 [details] Patch Fixed requested changes.
Committed r140604: <http://trac.webkit.org/changeset/140604>
Reopen this bug report as meta bug.
All pending patches landed.
rdar://11159162