Closed Bug 1446617 Opened 6 years ago Closed 6 years ago

Support SVG 2 path attribute for textPath

Categories

(Core :: SVG, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Attached patch patch with reftest (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8959784 - Flags: review?(dholbert)
Comment on attachment 8959784 [details] [diff] [review]
patch with reftest

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

::: dom/svg/SVGAnimatedPathSegList.cpp
@@ -1,1 @@
> -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This removal looks accidental -- please restore the emacs modeline here.

@@ +65,4 @@
>      }
>      return rv2;
>    }
> +  mIsAttrSet = true;

Do we hit this line even if aValue is ""? (e.g. if SetBaseValueString gets called with the empty string)

If so, I think that's a problem...

Or if not (i.e. if this part is fine), maybe add a brief code-comment here, hinting at where empty/error values were handled (presumably somewhere above), and stating that if we get here, we know we've got a valid non-empty-string base value.

@@ +147,5 @@
>  
> +bool
> +SVGAnimatedPathSegList::IsExplicitlySet() const
> +{
> +  return mIsAttrSet || !mBaseVal.IsEmpty() || mAnimVal;

Do we need the "mBaseVal.IsEmpty()" check there?  At first glance, it seems like that might be redundant after the mIsAttrSet check.

Note that in other IsExplicitlySet() impls, we seem to *just* check mAnimVal and a bool:
 https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGAnimatedNumberList.h#79
 https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGAnimatedPreserveAspectRatio.h#71
 https://dxr.mozilla.org/mozilla-central/source/dom/svg/nsSVGString.h#41
 https://dxr.mozilla.org/mozilla-central/source/dom/svg/nsSVGViewBox.h#64
...though maybe something is different here?

::: dom/svg/SVGAnimatedPathSegList.h
@@ +108,4 @@
>  
>    SVGPathData mBaseVal;
>    nsAutoPtr<SVGPathData> mAnimVal;
> +  bool mIsAttrSet;

Maybe this should be named "mIsBaseSet" to be a bit more specific (since IIUC this bool only covers the attr's base value, not its anim value)?  This seems to be the naming we've used in other places:
https://dxr.mozilla.org/mozilla-central/search?q=mIsBaseSet

(Though if this is different in a way I'm not yet seeing, then maybe there's a reason to use different naming)

Also might be nice to include a brief comment here or somewhere, hinting at why we need to bother with this bool (i.e. why can't we just use "!mBaseVal.IsEmpty()"?)

::: layout/reftests/svg/textPath-path-attribute-01.svg
@@ +6,5 @@
> +  <title>Testcase to check that textPath can point to a path attribute</title>
> +
> +  <text>
> +    <textPath path="M 100 100 h 400" font-size="20" fill="black">Should see this</textPath>
> +  </text>

Could you also test the "animVal" codepath, by adding:
   <textPath [...no path attribute here...]>Should see this
     <set attributeName="path" to="Some path expression here">

(and double-check that we fail the test if the "set" element is removed/commented-out, to be sure this part of the test can tell us if we break)

::: layout/svg/SVGTextFrame.cpp
@@ +5062,5 @@
>  {
> +  nsIContent* content = aTextPathFrame->GetContent();
> +  dom::SVGTextPathElement* tp = static_cast<dom::SVGTextPathElement*>(content);
> +  if (tp->mPath.IsExplicitlySet()) {
> +    return 1.0;

A code comment here would be helpful, now that this function is getting a bit more complex. I had to poke around a bit to figure out what this function did & why 1.0 is always the right answer for the |path| attribute.

(at least hint at "pathLength" and the distinction between getting path data from path attribute vs. path element)
Attachment #8959784 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #2)
> > -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> This removal looks accidental -- please restore the emacs modeline here.

Oops, will do.

> 
> @@ +65,4 @@
> >      }
> >      return rv2;
> >    }
> > +  mIsAttrSet = true;
> 
> Do we hit this line even if aValue is ""? (e.g. if SetBaseValueString gets
> called with the empty string)

> 
> If so, I think that's a problem...

Indeed, that's wrong, will correct. Only valid paths override per the w3 specification.

> 
> @@ +147,5 @@
> >  
> > +bool
> > +SVGAnimatedPathSegList::IsExplicitlySet() const
> > +{
> > +  return mIsAttrSet || !mBaseVal.IsEmpty() || mAnimVal;
> 
> Do we need the "mBaseVal.IsEmpty()" check there?  At first glance, it seems
> like that might be redundant after the mIsAttrSet check.

This code will go per the above comment.

> 
> (Though if this is different in a way I'm not yet seeing, then maybe there's
> a reason to use different naming)
> 
> Also might be nice to include a brief comment here or somewhere, hinting at
> why we need to bother with this bool (i.e. why can't we just use
> "!mBaseVal.IsEmpty()"?)

In theory you could use the SVG DOM to set an attribute so that's why you would need both checks. In practice as we don't expose this to the SVG DOM you can't but I didn't want to introduce a hard to find bug if the w3c did eventually mandate SVG DOM exposure. Anyway I'll need to change this for some other implementation.

> 
> ::: layout/reftests/svg/textPath-path-attribute-01.svg
> @@ +6,5 @@
> > +  <title>Testcase to check that textPath can point to a path attribute</title>
> > +
> > +  <text>
> > +    <textPath path="M 100 100 h 400" font-size="20" fill="black">Should see this</textPath>
> > +  </text>
> 
> Could you also test the "animVal" codepath, by adding:
>    <textPath [...no path attribute here...]>Should see this
>      <set attributeName="path" to="Some path expression here">
> 
> (and double-check that we fail the test if the "set" element is
> removed/commented-out, to be sure this part of the test can tell us if we
> break)

sure

> 
> ::: layout/svg/SVGTextFrame.cpp
> @@ +5062,5 @@
> >  {
> > +  nsIContent* content = aTextPathFrame->GetContent();
> > +  dom::SVGTextPathElement* tp = static_cast<dom::SVGTextPathElement*>(content);
> > +  if (tp->mPath.IsExplicitlySet()) {
> > +    return 1.0;
> 
> A code comment here would be helpful, now that this function is getting a
> bit more complex. I had to poke around a bit to figure out what this
> function did & why 1.0 is always the right answer for the |path| attribute.
> 
> (at least hint at "pathLength" and the distinction between getting path data
> from path attribute vs. path element)

// A path attribute has no way of setting a pathLength or transform so we return a unit scale.
Attached patch updated patch (obsolete) — Splinter Review
Attachment #8959784 - Attachment is obsolete: true
Attachment #8960744 - Flags: review?(dholbert)
Comment on attachment 8960744 [details] [diff] [review]
updated patch

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

::: dom/svg/SVGAnimatedPathSegList.cpp
@@ +146,5 @@
>  
> +bool
> +SVGAnimatedPathSegList::IsRendered() const
> +{
> +  return !mBaseVal.IsEmpty() || mAnimVal;

I think you need to *also* check mAnimVal.IsEmpty().  This should be:

  !mBaseVal.IsEmpty() || (mAnimVal && !mAnimVal.IsEmpty());

As noted in the comment above mAnimVal's declaration: if we animate the attribute to be empty (via having a nonempty base value combined with <set to="">), then that'll produce a non-null mAnimVal whose IsEmpty() returns true, and I think that should be treated as "not rendered" for the purposes of this function.

Please also extend your reftest with an example that would've caught this, too. (Like your existing <set> chunk in the testcase, but just with a valid "path" attribute on <textPath>, and with <set to=""> -- I imagine that'd fail with this patch as it currently stands.)

::: dom/svg/SVGTextPathElement.h
@@ +10,5 @@
>  #include "nsSVGEnum.h"
>  #include "nsSVGLength2.h"
>  #include "nsSVGString.h"
> +#include "SVGAnimatedPathSegList.h"
> +#include "SVGTextContentElement.h"

Please add "mozilla/dom/" before the filename, for these #include statements.

(It's not strictly necessary for this to compile, but it's nice to be consistent about how we #include headers that are exported in mozilla/dom/ etc. Note that e.g. "Element.h" is nearly universally #included using its full path - there are only two exceptions which are likely oversights:
https://searchfox.org/mozilla-central/search?q=%23include+%22Element.h%22 )

::: layout/reftests/svg/textPath-path-attribute-01.svg
@@ +10,5 @@
> +  </text>
> +
> +  <text>
> +    <textPath font-size="20" fill="black">Should see this too
> +      <set attributeName="path" to="M 100 200 h 400"/>

(Per comments above, please also test <set ... to="">, and a case with a transformed ancestor, in this test file or in another one.)

::: layout/svg/SVGTextFrame.cpp
@@ +5019,5 @@
>  already_AddRefed<Path>
>  SVGTextFrame::GetTextPath(nsIFrame* aTextPathFrame)
>  {
> +  nsIContent* content = aTextPathFrame->GetContent();
> +  dom::SVGTextPathElement* tp = static_cast<dom::SVGTextPathElement*>(content);

Note: This file has using namespace "mozilla::dom" at the top, so you don't strictly need the "dom::" prefix here.  (And we don't seem to bother with it a few lines down when we declare "SVGGeometryElement* element".)

@@ +5021,5 @@
>  {
> +  nsIContent* content = aTextPathFrame->GetContent();
> +  dom::SVGTextPathElement* tp = static_cast<dom::SVGTextPathElement*>(content);
> +  if (tp->mPath.IsRendered()) {
> +    return tp->mPath.GetAnimValue().BuildPathForMeasuring();

We're just directly returning the path here -- but in the GetTextPathGeometryElement-using case below this, we first modify the path, with a call to 
  gfxMatrix matrix = element->PrependLocalTransformsTo(gfxMatrix());
  [...] path->TransformedCopyToBuilder(ToMatrix(matrix));
  etc.

Do we need to do that here, too? (If we, the <text>/<textPath> element, have a transformed <g> ancestor up a few levels, for example, then don't we need that to be reflected in the path that we use for drawing?)

If so, please add (ideally sharing code if we can).  If not, please add a comment highlighting that difference & why we don't need to bother.  Either way, please add a testcase with a transformed <g> ancestor to be sure that works correctly.

@@ +5059,1 @@
>    SVGGeometryElement* element = GetTextPathGeometryElement(aTextPathFrame);

This variable "element" seems too vaguely named now.  Might be worth a followup patch, to rename this preexisting variable to "geomElem" perhaps? (That'd help distinguish it from the new vairables for aTextPathFrame's own element, called "content" and "tp" above this.)

This applies both to GetTextPath and GetOffsetScale.
Attachment #8960744 - Flags: review?(dholbert) → review-
The whole file is a mess of mozilla::dom:: sometimes and not others. 

Should it even be mozilla::dom or just mozilla given that it's a Frame and not an Element?
Perhaps I should clean up the namespacing completely rather than piecemeal. Either in a separate bug or a separate patch on this bug.
Flags: needinfo?(dholbert)
(In reply to Robert Longson [:longsonr] from comment #6)
> The whole file is a mess of mozilla::dom:: sometimes and not others. 

Darn. :-/

> Should it even be mozilla::dom or just mozilla given that it's a Frame and
> not an Element?

This file (SVGTextFrame.cpp) already has all of its functionality inside of just plain old "namespace mozilla{ ... }", and that's correct.  (Excluding some old-fashioned nsFoo at the very bottom, "class nsDisplaySVGText : public nsDisplayItem ...")

It uses types from mozilla::dom (and has "using namespace mozilla::dom"), and that's also fine.

So: no need for any re-categorization here. Just opportunities for brevity.

(In reply to Robert Longson [:longsonr] from comment #7)
> Perhaps I should clean up the namespacing completely rather than piecemeal.
> Either in a separate bug or a separate patch on this bug.

That sounds great! (Though: in comment 5, I was just pointing it out for those few "dom::SVGTextPathElement" usages in new lines, for brevity & consistency-with-directly-adjacent-code.)

But in general, IMO, we should feel empowered to take advantage of the "using namespace mozilla::dom;" and leave off the namespace prefix when mentioning types (except in cases where the bare typename is super short/generic & hence collisions might be possible).  Otherwise, there's no point in us having the "using namespace" declaration in the first place.
Flags: needinfo?(dholbert)
[Triage 2018/03/23 - P3]
Priority: -- → P3
Comment on attachment 8961642 [details] [diff] [review]
part 1 - remove unnecessary namespace usage

r=me
Attachment #8961642 - Flags: review?(dholbert) → review+
Keywords: leave-open
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/209b8e6ce45a
part 1 - remove unnecessary namespace usage r=dholbert
(In reply to Daniel Holbert [:dholbert] from comment #5)
> 
> I think you need to *also* check mAnimVal.IsEmpty().  This should be:

done (with test).

> >  #include "nsSVGString.h"
> > +#include "SVGAnimatedPathSegList.h"
> > +#include "SVGTextContentElement.h"
> 
> Please add "mozilla/dom/" before the filename, for these #include statements.

done for this case. This is a general mess too, but I've not done anything else about that here.

> 
> (Per comments above, please also test <set ... to="">, and a case with a
> transformed ancestor, in this test file or in another one.)

I think this is a misunderstanding of what's going on, see below.

> We're just directly returning the path here -- but in the
> GetTextPathGeometryElement-using case below this, we first modify the path,
> with a call to 
>   gfxMatrix matrix = element->PrependLocalTransformsTo(gfxMatrix());
>   [...] path->TransformedCopyToBuilder(ToMatrix(matrix));
>   etc.

In this case we transform the path with the path's transform (it's clearer I suppose now I've renamed element to geomElement). The textPath path attribute is not an element and has no transform.

> 
> Do we need to do that here, too? (If we, the <text>/<textPath> element, have
> a transformed <g> ancestor up a few levels, for example, then don't we need
> that to be reflected in the path that we use for drawing?)

No we don't/can't.

> 
> If so, please add (ideally sharing code if we can).  If not, please add a
> comment highlighting that difference & why we don't need to bother.  Either
> way, please add a testcase with a transformed <g> ancestor to be sure that
> works correctly.

It's not about ancestor transforms, it's the geometry element's transform we're looking at here. We don't have a geometry element so I can't really write a test for that.

> 
> @@ +5059,1 @@
> >    SVGGeometryElement* element = GetTextPathGeometryElement(aTextPathFrame);
> 
> This variable "element" seems too vaguely named now.  Might be worth a
> followup patch, to rename this preexisting variable to "geomElem" perhaps?
> (That'd help distinguish it from the new vairables for aTextPathFrame's own
> element, called "content" and "tp" above this.)
> 

Done, hopefully that will reduce the "ancestor element" confusion too.
Attachment #8960744 - Attachment is obsolete: true
Attachment #8962060 - Flags: review?(dholbert)
Comment on attachment 8962060 [details] [diff] [review]
part 2 - patch with reftest

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

Thanks! r=me
Attachment #8962060 - Flags: review?(dholbert) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd40ea4d0a95
support the SVG 2 path attribute for textPath elements r=dholbert
Keywords: leave-open
I tried and failed to get something that would compare to lime. This is a comparison of path vs href

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e631d2d714cf917ee004b4963e61a4a997034e
Flags: needinfo?(longsonr)
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d148dc003b46
support the SVG 2 path attribute for textPath elements r=dholbert
https://hg.mozilla.org/mozilla-central/rev/d148dc003b46
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: