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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

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]]

Attachment: patch

Comment 2

[Robert Longson [: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?

Comment 3

[Jonathan Watt [:jwatt]]

Hmm, yes, none of our current tests fail:

  https://tbpl.mozilla.org/?tree=Try&rev=5da076a8a3f1

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]]

Attachment: 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]]

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]]

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]]

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="http://www.w3.org/2000/svg" version="1.1" xmlns:xlink="http://www.w3.org/1999/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]]

Thanks, and done.

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f8970ab04037

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f8970ab04037

Comment 10

[Daniel Holbert [:dholbert]]

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]]

Attachment: 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]]

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]]

Landed followup:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/b41aa261098d

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/b41aa261098d

Comment 15

[Jonathan Watt [:jwatt]]

Thanks guys.