1325320 - Support SVGGeometryElement for other elements than path

Status: RESOLVED FIXED

(Core :: SVG, enhancement, P3)

Description

[Sebastian Zartner [:sebo]]

Bug 1239100 implemented the SVGGeometryElement interface from the SVG 2 spec. for path elements. Support for elements other than path still needs to be added.

Sebastian

Comment 1

[Robert Longson [:longsonr]]

Attachment: needs some tests

Comment 2

[Robert Longson [:longsonr]]

Attachment: includes wpt updates

Comment 3

[Daniel Holbert [:dholbert]]

This patch is kind of doing two things at once:

 (1) A non-functional refactoring (tweaking the SVGGeometryElement::GetOrBuildPath(...) API to take a const pointer rather than a const reference and updating all the callsites).

 (2) A mix of functional changes, to actually support this interface on these elements.

Would you mind splitting this into two separate patches, so the non-functional s/reference/pointer/ refactoring isn't mixed together with the functional changes?

Comment 4

[Robert Longson [:longsonr]]

Attachment: part 1 - use reference rather than pointer when all callers have a pointer

Note that part of what was here is now split off into bug 1443685. That bug will also need to land before we can pass SVGGeometryElement-rect.svg

Comment 5

[Robert Longson [:longsonr]]

Attachment: part 2 - change interface and implement GetOrBuildPath for all geometry elements

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8956670 [details] [diff] [review]
part 1 - use reference rather than pointer when all callers have a pointer

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

The commit message (which I'm inferring from the attachment title) is a bit vague right now.

Specifically:
 - It should probably mention SVGGeometryElement::GetOrBuildPath, because that is the one API that is being modified in this patch.
 - I'm not sure what it means by "when all callers have a pointer".  (Of course all callers have a pointer - that's necessarily true, for any API that takes a pointer-valued arg.)  Maybe you meant to say non-null pointer here?  (I'm willing to believe that's true, though it's also not superficially clear that all callers are enforcing/verifying that themselves.  E.g. SVGGeometryFrame::Render() passes in the result of a function called "GetDrawTarget" which sounds fallible. Though looking at gfxContext.h/cpp, it looks like it can typically be expected to return true.)

Maybe commit message should be something like:
Bug 1325320: Change SVGGeometryElement::GetOrBuildPath() to take DrawTarget as a reference (not pointer)"
...and then in a second line, some minor justification like the following:
Its implementation already depends on this arg being non-null, so we might as well be consistent with some other DrawTarget-handling code and use a reference.

Comment 7

[Daniel Holbert [:dholbert]]

> (In reply to Daniel Holbert [:dholbert] from comment #6)
> E.g. SVGGeometryFrame::Render() passes in the result of a
> function called "GetDrawTarget" which sounds fallible. Though looking at
> gfxContext.h/cpp, it looks like it can typically be expected to return true.)

(er, I meant "non-null" rather than "true" there, of course)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8956674 [details] [diff] [review]
part 2 - change interface and implement GetOrBuildPath for all geometry elements

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

::: dom/svg/SVGGeometryElement.cpp
@@ +128,5 @@
> +  RefPtr<DrawTarget> drawTarget =
> +    gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget();
> +  FillRule fillRule = mCachedPath ?
> +    mCachedPath->GetFillRule() : FillRule::FILL_WINDING;
> +  return GetOrBuildPath(drawTarget, fillRule);

For the "no cached path yet" case here:  is it bad that we're just using FILL_WINDING?  (Should we be looking at our "fill-rule" property and deferring to that, or does it not matter?)

Note that GetOrBuildPath does end up *setting* mCachedPath in this scenario, so if other APIs may come along and use a different fill-rule (and potentially get incorrect results or make us throw away our cached path), that might be bad from an either correctness or perf perspective. So maybe better to use the correct fill-rule up front?

Anyway -- even if this is fine for some reason, it probably merits a brief explanatory/justifying code-comment here.

Comment 9

[Daniel Holbert [:dholbert]]

(Thanks for splitting this up, BTW -- much easier to reason about like this.)

Comment 10

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #6)

>  - I'm not sure what it means by "when all callers have a pointer".  (Of

I meant that in the old API all callers started out with a pointer to a drawTarget which they converted using * before calling
the API which took a reference. drawTarget is generally obtained as a pointer and then on the whole passed around as one.

Comment 11

[Daniel Holbert [:dholbert]]

OK, gotcha. (I misread that part as a partial justification for the s/pointer/reference/ API conversion.)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd1e1288722
change SVGGeometryElement::GetOrBuildPath to take DrawTarget as a pointer since all its callers have drawTarget as a pointer themselves.

Comment 13

[Robert Longson [:longsonr]]

Attachment: part 2 - change interface and implement GetOrBuildPath for all geometry elements

I've changed it to call GetFillRule() instead.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8957484 [details] [diff] [review]
part 2 - change interface and implement GetOrBuildPath for all geometry elements

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

OK, thanks. r=me

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2bd1e1288722

Comment 16

[Robert Longson [:longsonr]]

Attachment: part 3 - move pathLength code to SVGGeometry element

With this (and part 2) we pass these tests: https://github.com/w3c/web-platform-tests/pull/9408/files

Comment 17

[Robert Longson [:longsonr]]

Attachment: Additional reftest for part 3

Comment 18

[Robert Longson [:longsonr]]

Comment on attachment 8957484 [details] [diff] [review]
part 2 - change interface and implement GetOrBuildPath for all geometry elements

Needs a DOM peer review.

In SVG 2 there is a new SVGGeometryElement interface and all basic shapes such as rect should derive from that rather than SVGGraphicsElement. E.g. https://www.w3.org/TR/SVG2/shapes.html#InterfaceSVGRectElement

Note that we already have the SVGGeometryElement interface in the tree, path elements implement it so this is just for the rest of the SVG shapes.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8957967 [details] [diff] [review]
part 3 - move pathLength code to SVGGeometry element

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

r=me, one nit:

::: dom/svg/SVGGeometryElement.h
@@ +56,4 @@
>                                  const nsAttrValue* aOldValue,
>                                  nsIPrincipal* aSubjectPrincipal,
>                                  bool aNotify) override;
> +  virtual bool IsNodeOfType(uint32_t aFlags) const override;

Drop the "virtual" keyword here -- it's implied by "override" and coding style says to use at most one of these keywords.
( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods )

(Existing code doesn't follow this particularly well yet, but we're getting there -- see e.g. bug 1436263 and bug 1428535.)

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8957968 [details] [diff] [review]
Additional reftest for part 3

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

r=me, one nit:

::: layout/reftests/svg/textPath-line-01.svg
@@ +7,5 @@
> +
> +  <defs>
> +    <line id="x" x1="100" y1="100" x2="500" y2="100"/>
> +  </defs>
> +	

nix whitespace on blank line there

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e18f1f40d080
part 2 - change shapes so they implement SVGGeometryElement and implement SVGGeometryElement::GetOrBuildPath r=dholbert r=mystor (DOM Peer)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8276eda78b8
part 3 - move GetPathLengthScale to SVGGeometryElement and convert callers of SVGPathElement methods to work on the base SVGGeometryElement class and work for all shapes, not just paths r=dholbert

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/788a11631abd
part 4 - additional reftest for textPath pointing to a line r=dholbert

Comment 24

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e18f1f40d080
https://hg.mozilla.org/mozilla-central/rev/e8276eda78b8
https://hg.mozilla.org/mozilla-central/rev/788a11631abd

Comment 25

[Florian Scholz (Open Web Docs)]

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement#Browser_compatibility
https://developer.mozilla.org/en-US/Firefox/Releases/61#SVG
https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_2_support_in_Mozilla

Comment 27

[Daniel Holbert [:dholbert]]

I think there's a bit more "dev-doc-needed" here, actually --> adding back keyword.

In particular, the MDN page https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement/getTotalLength has a reference to this very bug, saying:
> This method is currently only supported on <path> elements
> (where it was already supported in earlier Firefox versions
> through the SVGPathElement interface). Support for other elements
> will be added in bug 1325320.

The future tense "will be" is stale now that this bug has made it to release (in version 61).

Also, the compat table on that page incorrectly (?) says that this API *stopped being supported* in Firefox 53, whereas https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement says it started being supported in Firefox 53.  I'm not sure that makes sense (maybe this is trying to capture the fact that it moved from one interface to another in version 53? But I don't think so, because both pages have this API marked as "supported" in Chrome.)

Comment 28

[Daniel Holbert [:dholbert]]

https://developer.mozilla.org/docs/Web/API/SVGPathElement/getPointAtLength has the same problem, BTW (it's got a link to this bug saying that "support for other elements will be added" in this bug.)

I think https://github.com/mdn/browser-compat-data/blob/master/api/SVGPathElement.json is what we need to update to address these issues, but I'm not entirely sure what the correct update to make here is.

It seems like:
 (1) the notes pointing to this bug want to be removed
 (2) I'm not sure if that JSON file page should be updated to indicate that the API *is supported* in Firefox, vs. the API is not-supported in other browsers (e.g. Chrome, which like Firefox supports these APIs via inheriting it from the SVGGeometryElement interface). i.e. it's listed here:
https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/dom/webidl/SVGGeometryElement.webidl#17
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/svg/svg_geometry_element.idl?l=39
...but not here:
https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/dom/webidl/SVGPathElement.webidl
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/svg/svg_path_element.idl


The correct solution for (2) depends on the semantics of these compat tables in MDN, I guess, so I'm not submitting a pull request myself because I don't fully understand what the correct way to capture this "supported via a superclass/super-interface" data is in MDN.

Comment 29

[Chris Mills [:cmills]]

Hi Daniel,

I've submitted a new PR to our BCD repo to try and clear up this confusion.

In the notes on those methods, I've:

* Removed references to this bug. It is now supported on other types, as of Fx61, and web developers don't really care about this kind of historical detail.
* Made it clear what the relationship is between the two interfaces (i.e. the inheritance)
* Removed the notion that they stopped being supported on SVGPathElement — they are still available, but are now specified on its parent.

Let me know if this clears things up OK. Thanks!

Comment 30

[Daniel Holbert [:dholbert]]

Thanks, Chris! That sounds great.