Closed Bug 702696 Opened 8 years ago Closed 8 years ago

Path length scale factor should not be affected by the 'transform' attribute


(Core :: SVG, defect)

Not set





(Reporter: jwatt, Assigned: jwatt)



(2 files, 1 obsolete file)

The 'pathLength' attribute is supposed to be used to scale the values in stroke-dasharray etc. The scale factor is the actual path length as calculated by the user agent, divided by the path length as specified by the content author in the pathLength attribute. Both values are supposed to be user unit values. Unfortunately we currently have a bug whereby we take the 'transform' attribute on the path into account when calculating its length, which means we're not calculating the actual length in user units. We should be measuring the path length without any transform.
Attached patch patch (obsolete) — Splinter Review
Attachment #574626 - Flags: review?(longsonr)
You're changing how textPath works since that depends on this method doesn't it. Can you do a reftest to ensure that works as expected?
Hmm, yes, none of our current tests fail:

But this patch does break textPath. I'll write a new test to cover this case.

I thought that this fix would be the right thing for textPath too, but actually it's not since the spec special cases 'transform' on a <path> referenced by a <textPath>:

  The ‘transform’ attribute on the referenced ‘path’ element represents
  a supplemental transformation relative to the current user coordinate
  system for the current ‘text’ element, including any adjustments to
  the current user coordinate system due to a possible ‘transform’
  attribute on the current ‘text’ element.

So in the case of textPath we *do* want to include the 'transform' on the referenced <path>, but in the case of adjusting stroke attributes on a path, we do *not*.
Requesting review from dholbert since longsonr is away on holiday.
Attachment #574626 - Attachment is obsolete: true
Attachment #574626 - Flags: review?(longsonr)
Attachment #575882 - Flags: review?(dholbert)
Comment on attachment 575882 [details] [diff] [review]
patch that doesn't change the existing (correct) textPath behavior

>+  enum {
>+    eForTextPath,
>+    eForStroking
>+  };
>   /**
>    * Gets the ratio of the actual path length to the content author's estimated
>    * length (as provided by the <path> element's 'pathLength' attribute). This
>    * is used to scale stroke dashing, and to scale offsets along a textPath.
>    */
>-  gfxFloat GetPathLengthScale();
>+  gfxFloat GetPathLengthScale(PRUint32 aFor);

Could you give the enum a type name (e.g. PathLengthCalculationType), and make the function take that instead of PRUint32?

I know enums are generally int types under the hood, so it doesn't really matter functionally, but this way if someone sees the function decl, they immediately know where to look for the list of possible values.
Comment on attachment 575882 [details] [diff] [review]
patch that doesn't change the existing (correct) textPath behavior

>   if (mPathLength.IsExplicitlySet()) {
>+    gfxMatrix matrix;
>+    if (aFor == eForTextPath) {
>+      PrependLocalTransformTo(matrix);
>+    }
>+    nsRefPtr<gfxFlattenedPath> path = GetFlattenedPath(matrix);
>+    float authorsPathLengthEstimate = mPathLength.GetAnimValue();
>+    if (path && authorsPathLengthEstimate > 0) {
>+      return path->GetLength() / authorsPathLengthEstimate;

So, "mPathLength.GetAnimValue();" is the cheapest thing in this function, and all the other work isn't necessary if it returns an invalid value.

So, it might be worth bumping that check up to the top of this clause, and skipping the rest of the work if it's invalid.  So right after the "mPathLength.IsExplicitlySet()" check, we'd call: 
  float authorsPathLengthEstimate = mPathLength.GetAnimValue();
  if (authorsPathLengthEstimate > 0) {
That's basically a free optimization (even though it's for an uncommon/unexpected case), so it's probably worth doing, but I'm also fine with the patch as-is if you prefer the readability or something.

r=dholbert either way (with the enum thing fixed)
Attachment #575882 - Flags: review?(dholbert) → review+
Comment on attachment 575882 [details] [diff] [review]
patch that doesn't change the existing (correct) textPath behavior

>+++ b/layout/reftests/svg/textPath-02.svg
>@@ -0,0 +1,15 @@
>+<svg xmlns="" version="1.1" xmlns:xlink="">
>+  <title>Test affect on startOffset of a scale transform on a textPath's path</title>

also: s/affect/effect/ (former is a verb, latter is a noun)
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
From the pushed patch:
    1.27 +      gfxMatrix matrix;
    1.28 +      if (aFor == eForTextPath) {
    1.29 +        // For textPath, a transform on the referenced path affects the
    1.30 +        // textPath layout, so when calculating the actual path length
    1.31 +        // we need to take that into account.
    1.32 +        matrix = PrependLocalTransformTo(gfxMatrix());
    1.33 +      }

Ah, right -- looks like we need to catch the return-value of PrependLocalTransformTo there.  Good catch.

One nit on that change though -- the temporary gfxMatrix() that you create as an argument there is unnecessary.  We can just pass in "matrix".  Mind fixing that in a followup?
Target Milestone: mozilla11 → ---
Comment on attachment 577463 [details] [diff] [review]
followup to remove unnecessary temporary gfxMatrix() argument

Sorry, I have this in my queue to commit with the next non-trivial patch I commit, but feel free to commit.
Attachment #577463 - Flags: review?(jwatt) → review+
Thanks guys.
You need to log in before you can comment on or make changes to this bug.