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?

(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?)

(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)

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.

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.

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

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

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

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

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