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

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 574626 [details] [diff] [review]
patch
Attachment #574626 - Flags: review?(longsonr)

Comment 2

6 years ago
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?
(Assignee)

Comment 3

6 years ago
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*.
(Assignee)

Comment 4

6 years ago
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.
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="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)
(Assignee)

Comment 8

6 years ago
Thanks, and done.

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f8970ab04037
https://hg.mozilla.org/mozilla-central/rev/f8970ab04037
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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 → ---
Target Milestone: --- → mozilla11
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.
Attachment #577463 - Flags: review?(jwatt)
(Assignee)

Comment 12

6 years ago
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+
Landed followup:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/b41aa261098d
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/b41aa261098d
(Assignee)

Comment 15

6 years ago
Thanks guys.
You need to log in before you can comment on or make changes to this bug.