Add support for the AddPath API on the Path2D object.

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cabanier, Assigned: cabanier)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Shumway])

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Updated

5 years ago
Assignee: nobody → cabanier
Depends on: 830734
Keywords: dev-doc-needed
Assignee

Updated

5 years ago
Depends on: 985257
Assignee

Comment 1

5 years ago
This should be the last addition for a while :-)
Attachment #8393918 - Flags: review?(roc)
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?
Attachment #8393918 - Flags: review?(roc) → review-
Assignee

Comment 3

5 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.

Comment 4

5 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.
Whiteboard: [Shumway]

Comment 5

5 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?
Flags: needinfo?(roc)
(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"?
Flags: needinfo?(roc)
(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?
Assignee

Comment 8

5 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.
(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.
Assignee

Comment 10

5 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.
(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.
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.
Flags: needinfo?(cabanier)

Comment 13

5 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.
(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

Comment 15

5 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.
(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?
Assignee

Comment 17

5 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.
Flags: needinfo?(cabanier)
(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.
Assignee

Comment 19

5 years ago
Attachment #8393918 - Attachment is obsolete: true
Attachment #8477385 - Flags: review?(roc)
Assignee

Updated

5 years ago
Keywords: checkin-needed
The webidl change needs review from a DOM peer.
Keywords: checkin-needed
Assignee

Updated

5 years ago
Attachment #8477385 - Flags: review?(bzbarsky)
Assignee

Comment 22

5 years ago
Please review the WebIDL change to this patch?
Comment on attachment 8477385 [details] [diff] [review]
Add implementation + tests for Path2D::AddPath

This doesn't match the spec's IDL.  Why not?
Flags: needinfo?(cabanier)
Assignee

Comment 24

5 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.
Flags: needinfo?(cabanier) → needinfo?(bzbarsky)
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.
Attachment #8477385 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(bzbarsky)
Assignee

Comment 26

5 years ago
Attachment #8477385 - Attachment is obsolete: true
Attachment #8478306 - Flags: review?(bzbarsky)
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.
Attachment #8478306 - Flags: review?(bzbarsky) → review+
Assignee

Comment 28

5 years ago
Attachment #8478306 - Attachment is obsolete: true
Assignee

Comment 29

5 years ago
Attachment #8478312 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 30

5 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
You probably want to file an actual w3c bug on the spec...
And also, that last attachment doesn't address all the review comments.
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 33

5 years ago
Sorry for missing that. Could you take a quick view to make sure I didn't miss something again?
Attachment #8478314 - Attachment is obsolete: true
Attachment #8478341 - Flags: review?(bzbarsky)
Comment on attachment 8478341 [details] [diff] [review]
Add implementation + tests for Path2D::AddPath

Much better, thanks.
Attachment #8478341 - Flags: review?(bzbarsky) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bbafd7462ab4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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).
You need to log in before you can comment on or make changes to this bug.