Robert Longson [:longsonr] Assignee
	Description • 6 years ago

      No description provided.

	Daniel Holbert [:dholbert]
	Comment 2 • 6 years ago

Comment on attachment 8959831 [details] [diff] [review]
patch with reftest

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

::: dom/svg/SVGTextPathElement.h
@@ +23,5 @@
>  
> +// textPath side types
> +static const uint32_t TEXTPATH_SIDETYPE_UNKNOWN = 0;
> +static const uint32_t TEXTPATH_SIDETYPE_LEFT    = 1;
> +static const uint32_t TEXTPATH_SIDETYPE_RIGHT   = 2;

Two nits:
(1) These should probably be uint16_6, since that's how you use them elsewhere in this patch ("uint16_t side =..." in SVGTextFrame.cpp)

(2) Is TEXTPATH_SIDETYPE_UNKNOWN ever used?  It doesn't look like it, to me... I don't see anything that'd map any user-provided input to that value.  And we set up the EnumInfo to make the default be _LEFT, not _UNKNOWN.  So unless there's a usage I'm not seeing, I think _UNKNOWN should be removed...?  (Though if we end up exposing this enum via WebIDL, per below, I suppose we'll end up having to define it anyway).

(3) It strikes me as odd that these are defined here vs. others in the .webidl file:
  https://dxr.mozilla.org/mozilla-central/source/dom/webidl/SVGTextPathElement.webidl#13
...but I suppose this is correct per spec, because the spec's own webidl only lists those other enums & their attrs, and not this .side or its values:
https://www.w3.org/TR/SVG2/text.html#InterfaceSVGTextPathElement

That seems like an odd inconsistency, though.  Was that just an oversight on the part of the SVGWG? (i.e. did they just intend to add it to the WebIDL & just forgot?) It seems like the SVG spec exposes every other enum-valued attribute that I can find at the moment, AFAICT... I'm having a hard time finding another nsSVGEnum value that's not exposed via WebIDL, at least.

So maybe we should get clarification from the SVGWG, and (assuming they weren't intentionally excluding it) we should list the attribute & values alongside the others in the webidl?

::: layout/reftests/svg/textPath-side-attribute-01.svg
@@ +20,5 @@
> +    <textPath href="#path2">Text on a path.</textPath>
> +  </text>
> +
> +  <text y="50" font-size="30" fill="lime" stroke="lime" stroke-width="3">
> +    <textPath side="right" href="#path1">Text on a path.</textPath>

Is the "y" attribute here supposed to matter?  It seems like you're *relying* on it mattering, to separate the top half of the test from the bottom half.  And it does seem to matter, in Firefox Nightly, but I think that's actually a bug... (I'll file)

(In Chrome, "y" on <text> doesn't seem to affect the rendering of <text><textPath>... contents, and in Firefox Nightly, it *only* affects the rendering if there's no additional text in the <text> element.  So it seems like the fact that "y" is impacting the rendering here is in fact a Firefox bug.)

Could you use another method of separating the parts of this test, so that you're not reliant on that possible Firefox bug?

	Daniel Holbert [:dholbert]
	Comment 3 • 6 years ago

(Also, could you add a test with <set> to be sure we're honoring the animated value of this attribute, and not just the base value?)

	Daniel Holbert [:dholbert]
	Comment 4 • 6 years ago

(I filed bug 1447169 on the fact that "y" seems to make a difference in the reftest here, when it doesn't in other browsers)

	Robert Longson [:longsonr] Assignee
	Comment 5 • 6 years ago

I tried to get w3c to add side to the webidl but they don't seem keen. https://github.com/w3c/svgwg/issues/391

I guess I can set a transform on the text element instead.

	Daniel Holbert [:dholbert]
	Comment 7 • 6 years ago

It looks like the testcase was lost in the latest version of the patch -- it adds a reftest.list line mentioning "textPath-side-attribute-01.svg", but that file isn't present in the patch.

	Daniel Holbert [:dholbert]
	Comment 8 • 6 years ago

(I think this is r+ assuming the test is good, BTW.)

	Daniel Holbert [:dholbert]
	Comment 10 • 6 years ago

Comment on attachment 8961169 [details] [diff] [review]
path with test this time

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

r=me with one more test added:

::: layout/reftests/svg/textPath-side-attribute-01.svg
@@ +8,5 @@
> +
> +  <rect fill="lime" width="100%" height="100%"/>
> +
> +  <text font-size="30" fill="red">
> +    <textPath side="right" href="#path1">Text on a path.</textPath>

It looks like we're not testing side="left" at all in this file -- please add at least one example that uses that.

(IIUC, left behaves as if the attribute weren't set, so that part of the test will presumably "pass" already in current Firefox, and that's fine. This is just to be sure the new code doesn't interpret "left" as "right", or crash when it hits "left" due to some unhandled case, etc.)

	Robert Longson [:longsonr] Assignee
	Comment 11 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=590aa2536651b7994b8686f21ad484a1b9a5f008

	Robert Longson [:longsonr] Assignee
	Comment 12 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64f2a23904ff28c10bf6d483c22ef1acd7e566a0

	Pulsebot
	Comment 13 • 6 years ago

Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/962be3a60013
support the SVG 2 side attribute for textPaths r=dholbert

	Narcis Beleuzu [:NarcisB]
	Comment 14 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/962be3a60013

	Florian Scholz (Open Web Docs)
	Comment 15 • 6 years ago

Mentioned on developer release notes for Fx 61:
https://developer.mozilla.org/en-US/Firefox/Releases/61#SVG

Updated compat data:
https://developer.mozilla.org/en-US/docs/Web/SVG/Element/textPath#Browser_compatibility

	Florian Scholz (Open Web Docs)
	Comment 16 • 6 years ago

Also updated https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_2_support_in_Mozilla#Text