Rik Cabanier Assignee
	Description • 10 years ago

Spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-path-addpath
This was also implemented in WebKit and Blink.

	Rik Cabanier Assignee
	Comment 1 • 10 years ago

This should be the last addition for a while :-)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 10 years ago

Comment on attachment 8393918 [details] [diff] [review]
Bug 985801 - Add implementation for Path2D::AddPath

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4375,5 @@
> +
> +    if (!transform.IsIdentity()) {
> +      if (!transform.Invert()) {
> +        NS_WARNING("Could not invert transform");
> +        return;

Can you add a comment explaining why this satisfies the specced behavior for singular transforms?

@@ +4377,5 @@
> +      if (!transform.Invert()) {
> +        NS_WARNING("Could not invert transform");
> +        return;
> +      }
> +      

Trailing whitespace

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ +318,5 @@
>   Constructor,
>   Constructor(Path2D other)]
>  interface Path2D
> +{
> +  void addPath(Path2D path, optional SVGMatrix transformation);

I don't understand why this isn't part of CanvasPathMethods. Can you explain why?

	Rik Cabanier Assignee
	Comment 3 • 10 years ago

Comment on attachment 8393918 [details] [diff] [review]
Bug 985801 - Add implementation for Path2D::AddPath

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4375,5 @@
> +
> +    if (!transform.IsIdentity()) {
> +      if (!transform.Invert()) {
> +        NS_WARNING("Could not invert transform");
> +        return;

yes, it doesn't match the spec.
I will hold off on submitting this until there's more clarity. WK and Blink both exit on non-invertible matrices.

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ +318,5 @@
>   Constructor,
>   Constructor(Path2D other)]
>  interface Path2D
> +{
> +  void addPath(Path2D path, optional SVGMatrix transformation);

No, there's no good reason why this is absent from the 2d context.

	Dirk Schulze
	Comment 4 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)

> Can you add a comment explaining why this satisfies the specced behavior for singular transforms?

The spec does not say how to transform a path with a singular matrix. In WebKit we never perform a current path or Path2D action on the context if the CTM or transformation matrix is singular. This basically follows the concept of SVG.

> I don't understand why this isn't part of CanvasPathMethods. Can you explain
> why?

I think this is a valid question and I would really like to have CanvasPathMethods and am willing to do the change in WebKit.

In general, the interface would just move from Path2D.idl to CanvasPathMethods.idl. Path2D would have the same support. Given the concerns from Blink, I think it is ok to implement addPath incrementally. Should we get to an agreement, we could just move addPath to CanvasPathMethods.

	Dirk Schulze
	Comment 5 • 10 years ago

The IDL changed to
 
optional SVGMatrix? transformation = null
 
but is still part of Path2D. IIRC the concerns from Blink stay. Roc, what do you think how we should proceed?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 10 years ago

(In reply to Dirk Schulze from comment #4)
> In general, the interface would just move from Path2D.idl to
> CanvasPathMethods.idl. Path2D would have the same support. Given the
> concerns from Blink, I think it is ok to implement addPath incrementally.
> Should we get to an agreement, we could just move addPath to
> CanvasPathMethods.

What are these "concerns from Blink"?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 10 years ago

(In reply to Dirk Schulze from comment #4)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> 
> > Can you add a comment explaining why this satisfies the specced behavior for singular transforms?
> 
> The spec does not say how to transform a path with a singular matrix. In
> WebKit we never perform a current path or Path2D action on the context if
> the CTM or transformation matrix is singular. This basically follows the
> concept of SVG.

Can we get the spec fixed to specify this properly?

	Rik Cabanier Assignee
	Comment 8 • 10 years ago

There was a thread on WhatWG on this: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/296553.html

There was another one where you were leaning towards not changing the spec.

	Till Schneidereit [:till]
	Comment 9 • 10 years ago

(In reply to Dirk Schulze from comment #4)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> > I don't understand why this isn't part of CanvasPathMethods. Can you explain
> > why?
> 
> I think this is a valid question and I would really like to have
> CanvasPathMethods and am willing to do the change in WebKit.
> 
> In general, the interface would just move from Path2D.idl to
> CanvasPathMethods.idl. Path2D would have the same support. Given the
> concerns from Blink, I think it is ok to implement addPath incrementally.
> Should we get to an agreement, we could just move addPath to
> CanvasPathMethods.

Has anything happened regarding this? Having addPath on the context would make it much easier to reuse paths, as it enables separating path geometry transforms from stroke style transforms.

This demonstrates an issue where a path is supposed to be reused in a transformed context, but the stroke should not be affected by the transform (i.e., the effective lineWidth should be 10px at all points on the path):
http://jsbin.com/bufuqani/1/edit

It's possible to work around the lack of this capability by reverting the transform on the context and supplying it as an argument to Path2D#addPath on a temporarily-created path:
http://jsbin.com/bufuqani/3/edit

However, that seems unnecessarily complicated and wasteful.

	Rik Cabanier Assignee
	Comment 10 • 10 years ago

(In reply to Till Schneidereit [:till] from comment #9)
> 
> This demonstrates an issue where a path is supposed to be reused in a
> transformed context, but the stroke should not be affected by the transform
> (i.e., the effective lineWidth should be 10px at all points on the path):
> http://jsbin.com/bufuqani/1/edit
> 
> It's possible to work around the lack of this capability by reverting the
> transform on the context and supplying it as an argument to Path2D#addPath
> on a temporarily-created path:
> http://jsbin.com/bufuqani/3/edit
> 
> However, that seems unnecessarily complicated and wasteful.

It sounds that what you really want is a way to transform a path and the addPath API lets you do it as a side effect.
Maybe you should ask for a 'transform' method on the path2d object that returns a new transformed path.

	Till Schneidereit [:till]
	Comment 11 • 10 years ago

(In reply to Rik Cabanier from comment #10)
> Maybe you should ask for a 'transform' method on the path2d object that
> returns a new transformed path.

It seems like that would be functionally identical to the workaround in the second link: creating a new path and adding the to-be-transformed on to it with the transform given.

That works, but requires creating temporary path instances. Which isn't the end of the world, to be sure.

	Till Schneidereit [:till]
	Comment 12 • 10 years ago

IIUC, the remaining open issue here is whether addPath should be moved to CanvasPathMethods, right? If so, can we land it as a member of Path2D for now? In Shumway, we currently have to polyfill the entirety of Path2D if addPath is missing, severely impacting performance.

If the issue is driving this into the tree, I'm happy to do the required work.

	Dirk Schulze
	Comment 13 • 10 years ago

(In reply to Till Schneidereit [:till] from comment #12)
> IIUC, the remaining open issue here is whether addPath should be moved to
> CanvasPathMethods, right? If so, can we land it as a member of Path2D for
> now? In Shumway, we currently have to polyfill the entirety of Path2D if
> addPath is missing, severely impacting performance.

The spec does not clarify if a path argument should still be applied if the second argument describes a singular transformation matrix.

IIRC Blink is fine with not applying the path in this cases. Speaking for WebKit, I am fine too. The patch from Rik returns earlier on singular matrices either. Just Hixie doesn't like it though.

Since browser vendors seem to agree, I don't see an issue with both... adding addPath to Path2D for now and returning a singular matrix.

If Gecko would go this way, I would remove the compiler flag in the WebKit implementation.

	Till Schneidereit [:till]
	Comment 14 • 10 years ago

(In reply to Dirk Schulze from comment #13)
> (In reply to Till Schneidereit [:till] from comment #12)
> > IIUC, the remaining open issue here is whether addPath should be moved to
> > CanvasPathMethods, right? If so, can we land it as a member of Path2D for
> > now? In Shumway, we currently have to polyfill the entirety of Path2D if
> > addPath is missing, severely impacting performance.
> 
> The spec does not clarify if a path argument should still be applied if the
> second argument describes a singular transformation matrix.
> 
> IIRC Blink is fine with not applying the path in this cases. Speaking for
> WebKit, I am fine too. The patch from Rik returns earlier on singular
> matrices either. Just Hixie doesn't like it though.
> 
> Since browser vendors seem to agree, I don't see an issue with both...
> adding addPath to Path2D for now and returning a singular matrix.

Right, I had assumed that that was the accepted resolution, as I didn't see Hixie's opposition. Can you give a link? I only saw http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/thread.html#296569

	Dirk Schulze
	Comment 15 • 10 years ago

(In reply to Till Schneidereit [:till] from comment #14)
> 
> Right, I had assumed that that was the accepted resolution, as I didn't see
> Hixie's opposition. Can you give a link? I only saw
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/thread.
> html#296569

There was no update since then.

	Till Schneidereit [:till]
	Comment 16 • 10 years ago

(In reply to Dirk Schulze from comment #15)
> (In reply to Till Schneidereit [:till] from comment #14)
> > 
> > Right, I had assumed that that was the accepted resolution, as I didn't see
> > Hixie's opposition. Can you give a link? I only saw
> > http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/thread.
> > html#296569
> 
> There was no update since then.

So Hixie voiced is opposition on IRC or IRL?

	Rik Cabanier Assignee
	Comment 17 • 10 years ago

(In reply to Till Schneidereit [:till] from comment #12)
> IIUC, the remaining open issue here is whether addPath should be moved to
> CanvasPathMethods, right? If so, can we land it as a member of Path2D for
> now? In Shumway, we currently have to polyfill the entirety of Path2D if
> addPath is missing, severely impacting performance.
 
This patch was held up because if the issue that Dirk brought up: what to do with singular matrices.

I think we can address that issue later. If I have time, I will update my patch and submit it.

	Till Schneidereit [:till]
	Comment 18 • 10 years ago

(In reply to Rik Cabanier from comment #17)
> I think we can address that issue later. If I have time, I will update my
> patch and submit it.

Great. If it turns out you don't have time, let me know and I'll drive it in.

	Rik Cabanier Assignee
	Comment 20 • 10 years ago

try run: https://tbpl.mozilla.org/?tree=Try&rev=9996933c1176

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

The webidl change needs review from a DOM peer.

	Rik Cabanier Assignee
	Comment 22 • 10 years ago

Please review the WebIDL change to this patch?

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 10 years ago

Comment on attachment 8477385 [details] [diff] [review]
Add implementation + tests for Path2D::AddPath

This doesn't match the spec's IDL.  Why not?

	Rik Cabanier Assignee
	Comment 24 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #23)
> Comment on attachment 8477385 [details] [diff] [review]
> Add implementation + tests for Path2D::AddPath
> 
> This doesn't match the spec's IDL.  Why not?

We had an agreement on the mailing list to make it optional and this is also how Blink implemented it.
See: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/296579.html

The spec has not been updated yet.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 10 years ago

Comment on attachment 8477385 [details] [diff] [review]
Add implementation + tests for Path2D::AddPath

> The spec has not been updated yet.

OK.  Please make sure a spec issue is filed (and possibly link to the spec issue from the IDL file, if you don't think it will get updated in the very immediate future).

>+               const Optional<NonNull<SVGMatrix> >&  matrix);

Please drop the space in "> >" and the extra space after the '&'.

Also, either both arguments should be named aSomething or neither should have the 'a' prefix.  Having it for one but not the other is just bizarre.  Same in the .cpp.

>+ } catch(e) {
>+  throw e;
>+  ok(false, "unexpected exception thrown in: test_pathconstructor_canvas");

This code makes no sense.  Did you mean to have those two lines in the opposite order?

Of course this same pattern occurs in other places in the test file....  Those should be fixed too.

r=me with those issues dealt with.

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 10 years ago

Comment on attachment 8478306 [details] [diff] [review]
Add implementation + tests for Path2D::AddPath

That's fine for the test changes, but please do address my other review comments too.

r=me with that.

	Rik Cabanier Assignee
	Comment 30 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #27)
> Comment on attachment 8478306 [details] [diff] [review]
> Add implementation + tests for Path2D::AddPath
> 
> That's fine for the test changes, but please do address my other review
> comments too.
> 
> r=me with that.

Done. Saw you comments after I had updated the test so I submitted another patch

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 10 years ago

You probably want to file an actual w3c bug on the spec...

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 10 years ago

And also, that last attachment doesn't address all the review comments.

	Rik Cabanier Assignee
	Comment 33 • 10 years ago

Sorry for missing that. Could you take a quick view to make sure I didn't miss something again?

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 10 years ago

Comment on attachment 8478341 [details] [diff] [review]
Add implementation + tests for Path2D::AddPath

Much better, thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 36 • 10 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 37 • 10 years ago

Mentioned on Firefox 34 for developers:
https://developer.mozilla.org/en-US/Firefox/Releases/34#Interfaces.2FAPIs.2FDOM

Reference page:
https://developer.mozilla.org/en-US/docs/Web/API/Path2D.addPath

Probably doesn't matter, but I stumbled upon the use of ctx.currentTransform in one of the test cases in the patch. It is not implemented yet (bug 928150).