Last Comment Bug 702696 - Path length scale factor should not be affected by the 'transform' attribute
: Path length scale factor should not be affected by the 'transform' attribute
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jonathan Watt [:jwatt]
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2011-11-15 11:04 PST by Jonathan Watt [:jwatt]
Modified: 2012-02-01 13:58 PST (History)
2 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.12 KB, patch)
2011-11-15 11:05 PST, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch that doesn't change the existing (correct) textPath behavior (7.44 KB, patch)
2011-11-21 09:01 PST, Jonathan Watt [:jwatt]
dholbert: review+
Details | Diff | Splinter Review
followup to remove unnecessary temporary gfxMatrix() argument (1.15 KB, patch)
2011-11-28 19:16 PST, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2011-11-15 11:04:19 PST
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.
Comment 1 Jonathan Watt [:jwatt] 2011-11-15 11:05:30 PST
Created attachment 574626 [details] [diff] [review]
Comment 2 Robert Longson 2011-11-15 11:15:14 PST
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?
Comment 3 Jonathan Watt [:jwatt] 2011-11-20 13:02:03 PST
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*.
Comment 4 Jonathan Watt [:jwatt] 2011-11-21 09:01:11 PST
Created attachment 575882 [details] [diff] [review]
patch that doesn't change the existing (correct) textPath behavior

Requesting review from dholbert since longsonr is away on holiday.
Comment 5 Daniel Holbert [:dholbert] 2011-11-21 10:42:35 PST
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 6 Daniel Holbert [:dholbert] 2011-11-21 11:00:37 PST
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)
Comment 7 Daniel Holbert [:dholbert] 2011-11-21 11:08:50 PST
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)
Comment 8 Jonathan Watt [:jwatt] 2011-11-21 13:20:57 PST
Thanks, and done.

Comment 9 Ed Morley [:emorley] 2011-11-21 19:07:21 PST
Comment 10 Daniel Holbert [:dholbert] 2011-11-21 19:49:20 PST
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?
Comment 11 Daniel Holbert [:dholbert] 2011-11-28 19:16:02 PST
Created attachment 577463 [details] [diff] [review]
followup to remove unnecessary temporary gfxMatrix() argument

Here's a trivial followup to address the thing I brought up at the end of comment 10.
Comment 12 Jonathan Watt [:jwatt] 2011-11-29 02:07:23 PST
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.
Comment 13 Daniel Holbert [:dholbert] 2011-11-29 14:07:26 PST
Landed followup:
Comment 14 Marco Bonardo [::mak] 2011-11-30 03:56:22 PST
Comment 15 Jonathan Watt [:jwatt] 2011-11-30 04:01:21 PST
Thanks guys.

Note You need to log in before you can comment on or make changes to this bug.