transform-box property is affecting SVG positioning attributes

RESOLVED FIXED in Firefox 54

Status

()

Core
SVG
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: Amelia Bellamy-Royds, Assigned: cjku)

Tracking

53 Branch
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [webcompat])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce:

Apply the `transform-box: fill-box` CSS property to an SVG shape with non-zero positioning attributes (x,y,cx,cy)

Test case: http://codepen.io/AmeliaBR/pen/1816eafb75d2416b1b0cd491a30aaee1?editors=1000



Actual results:

The position of the shape changes.

I'm not sure exactly what's going on, but it looks like the viewBox that is used to determine x/y/cx/cy is being scaled by the fill-box size.  It's not affected by transform-origin: the effect is entirely caused by transform-box and the basic positioning attributes.


Expected results:

Nothing. The `transform-box` property should only affect transforms.
(Reporter)

Comment 1

7 months ago
PS, for tracking down the code that caused the problem, you'll want to look at
https://bugzilla.mozilla.org/show_bug.cgi?id=1208550
https://bugzilla.mozilla.org/show_bug.cgi?id=923193
https://bugzilla.mozilla.org/show_bug.cgi?id=1175492
Blocks: 1208550
Blocks: 923193
Status: UNCONFIRMED → NEW
Component: Untriaged → SVG
Ever confirmed: true
(Assignee)

Comment 2

7 months ago
nsStyleDisplay::mTransformBox; // transform-box
nsStyleDisplay::mTransformOrigin // transform-origin
Assignee: nobody → cku
Comment hidden (mozreview-request)
Comment hidden (typo)
(Assignee)

Comment 5

7 months ago
The reference box for viewbox scaling transform is always view-box, the value of transform-box affects only css or svg transform.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

7 months ago
Created attachment 8832969 [details]
test cases for fill-box
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8832773 - Flags: review?(cam)
Attachment #8833217 - Flags: review?(cam)
Attachment #8832911 - Flags: review?(cam)
Attachment #8832943 - Flags: review?(cam)

Comment 14

7 months ago
mozreview-review
Comment on attachment 8832773 [details]
Bug 1335876 - Part 1. Declare mTransformBox as StyleGeometryBox.

https://reviewboard.mozilla.org/r/109020/#review110514

::: layout/style/nsStyleConsts.h:90
(Diff revision 2)
>  };
>  
>  // Define geometry box for clip-path's reference-box, background-clip,
> -// background-origin, mask-clip and mask-origin.
> +// background-origin, mask-clip, mask-origin, shape-box and transform-box.
>  enum class StyleGeometryBox : uint8_t {
> -  Content,
> +  Content, // Used by anything, except transform-box.

s/anything/everything/ (in this and the other comments)
Attachment #8832773 - Flags: review?(cam) → review+

Comment 15

7 months ago
mozreview-review
Comment on attachment 8833217 [details]
Bug 1335876 - Part 1. Rename coord as transformOrigin.

https://reviewboard.mozilla.org/r/109446/#review110524

::: layout/painting/nsDisplayList.cpp:5913
(Diff revision 1)
>  
>    for (uint8_t index = 0; index < 2; ++index) {
>      /* If the transform-origin specifies a percentage, take the percentage
>       * of the size of the box.
>       */
> -    const nsStyleCoord &coord = display->mTransformOrigin[index];
> +    const nsStyleCoord &transformOrigin = display->mTransformOrigin[index];

I think instead we should rename the |coords| array to |transformOrigin|, since that's the value the represents the entire (2D) transform origin value.  Then maybe rename |coord| to |transformOriginCoord|, since it is one of the origin's coordinates.  WDYT?

Comment 16

7 months ago
mozreview-review
Comment on attachment 8832911 [details]
Bug 1335876 - Part 2. (Main) Setup basis of the matrix according to transform-box.

https://reviewboard.mozilla.org/r/109162/#review110530

I don't really understand these changes, so I think they need more comments to explain what's happening.  But it might be better for jwatt to review this change anyway.
Attachment #8832911 - Flags: review?(jwatt)
Attachment #8832943 - Flags: review?(cam) → review?(jwatt)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

7 months ago
mozreview-review
Comment on attachment 8833217 [details]
Bug 1335876 - Part 1. Rename coord as transformOrigin.

https://reviewboard.mozilla.org/r/109446/#review110580
Attachment #8833217 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

6 months ago
mozreview-review
Comment on attachment 8832943 [details]
Bug 1335876 - Part 3. Reftest.

https://reviewboard.mozilla.org/r/109182/#review113172

This looks pretty good, but please address the comment below and re-request review.

::: layout/reftests/transform/transform-box-svg-3a.svg:9
(Diff revision 6)
> +    .ref {
> +      fill: lime;
> +    }
> +    .test {
> +      fill: red;
> +    }

It would really be better to invert the colors here. Having the 'test' elements be red means that it is possible that the test elements could be transformed out of the viewport (or, less likely, under another element) when things go wrong. In that case the test could pass when it should in fact fail. Inverting the colors so that the fail elements (the ones without transforms) are rect means that we can be fairly confident that the 'fail' rects will be on screen, and to pass the transforms need to be exactly right to cover each and every fail rect.
Attachment #8832943 - Flags: review?(jwatt) → review-
By "fail elements" I mean "ref elements". Also "so that the fail elements (the ones without transforms) are rect" should have been "so that the *ref* elements (the ones without transforms) are *red*".
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

6 months ago
mozreview-review
Comment on attachment 8832911 [details]
Bug 1335876 - Part 2. (Main) Setup basis of the matrix according to transform-box.

https://reviewboard.mozilla.org/r/109162/#review113522

This is pretty hard code to get your head around, so I really appreciate you working on this! r- for now though until we work out the comments below.

::: layout/painting/nsDisplayList.cpp:6429
(Diff revision 8)
>    } else {
>      Point3D refBoxOffset(NSAppUnitsToFloatPixels(refBox.X(), aAppUnitsPerPixel),
>                           NSAppUnitsToFloatPixels(refBox.Y(), aAppUnitsPerPixel),
>                           0);
> +
>      // We have both a transform and children-only transform. The

Please change "We have both a transform and children-only transform" to "We have both an SVG transform and an SVG children-only transform". And please change "The 'transform-origin'" to "Any translation due to 'transform-origin' and/or 'transform-box'".

::: layout/painting/nsDisplayList.cpp:6431
(Diff revision 8)
>                           NSAppUnitsToFloatPixels(refBox.Y(), aAppUnitsPerPixel),
>                           0);
> +
>      // We have both a transform and children-only transform. The
>      // 'transform-origin' must apply between the two, so we need to apply it
>      // now before we apply transformFromSVGParent. Since mToTransformOrigin is

Given the changes that you are making, and given the changes that I suggest below, please delete everything from "Since mToTransformOrigin is..." to the end of the comment.

::: layout/painting/nsDisplayList.cpp:6444
(Diff revision 8)
>        frame->PresContext()->AppUnitsPerCSSPixel() / aAppUnitsPerPixel;
>      transformFromSVGParent._31 *= pixelsPerCSSPx;
>      transformFromSVGParent._32 *= pixelsPerCSSPx;
> -    result = result * Matrix4x4::From2D(transformFromSVGParent);
>  
> -    // Similar to the code in the |if| block above, but since we've accounted
> +    // There are two kinds of transforms that we will apply to 'frame':

This text sort of duplicates the "We have both a transform and children-only transform" comment above it.

::: layout/painting/nsDisplayList.cpp:6450
(Diff revision 8)
> -    // for mToTransformOrigin so we don't include that. We also need to reapply
> -    // refBoxOffset.
> +    // frame's owned transform(stores in result, denotes as TO) and a
> +    // transform from 'frame''s parent(stores in transformFromSVGParent,
> +    // denotes as TP).
> +    //
> +    // The origin of TP has nothing to do with transform-box, and should
> +    // always be at the origin of user space.

To understand TP the user has to have read the documentation for nsSVGContainerFrame::HasChildrenOnlyTransform. With that done I don't think noting "TP has nothing to do with transform-box" is helpful. Also your use of the term "user space" here is not how it tends to be used elsewhere, which makes this comment confusing (in SVG "user space" is the space in which an element's x,y,etc. apply).

::: layout/painting/nsDisplayList.cpp:6458
(Diff revision 8)
> +    // fill-box, or at the origin of user space if transform-box is
> +    // view-box(or border-box, which will be fallback to view-box in SVG
> +    // layout).
> +    // So the final transform will look like
> +    // 1. For fill-box:
> +    //   TO.ChangeBasis(toFillBoxOffset) * TP.ChangeBasis(toUserSpaceOffset)

Again, the use of "UserSpace" here is confusing.

::: layout/painting/nsDisplayList.cpp:6461
(Diff revision 8)
> +    // So the final transform will look like
> +    // 1. For fill-box:
> +    //   TO.ChangeBasis(toFillBoxOffset) * TP.ChangeBasis(toUserSpaceOffset)
> +    // 2. For view-box(or border-box):
> +    //   TO.ChangeBasis(toUserSpaceOffset) * TP.ChangeBasis(toUserSpaceOffset)
>      result.ChangeBasis(refBoxOffset);

Why put this down here? This line would be best to come directly after the '`result.ChangeBasis(aProperties.mToTransformOrigin - refBoxOffset);`' line further up. In fact, it cancels out the '`- refBoxOffset`' so instead of adding this line, remove that.

As for the comment, I think you could drop it, although maybe there's some useful info you could integrate into the comment above the '`result.ChangeBasis(aProperties.mToTransformOrigin - refBoxOffset);`' line.
Attachment #8832911 - Flags: review?(jwatt) → review-

Comment 37

6 months ago
mozreview-review
Comment on attachment 8832943 [details]
Bug 1335876 - Part 3. Reftest.

https://reviewboard.mozilla.org/r/109182/#review113526

Awesome, thank you!
Attachment #8832943 - Flags: review?(jwatt) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 40

6 months ago
mozreview-review
Comment on attachment 8832911 [details]
Bug 1335876 - Part 2. (Main) Setup basis of the matrix according to transform-box.

https://reviewboard.mozilla.org/r/109162/#review113676

::: layout/painting/nsDisplayList.cpp:6425
(Diff revision 9)
>      // This is a simplification of the following |else| block, the
>      // simplification being possible because we don't need to apply
>      // mToTransformOrigin between two transforms.
>      result.ChangeBasis(aProperties.mToTransformOrigin);
>    } else {
>      Point3D refBoxOffset(NSAppUnitsToFloatPixels(refBox.X(), aAppUnitsPerPixel),

You'll need to remove this variable now that it is unused, otherwise you're going to get build errors when your patches are pushed to m-i.

::: layout/painting/nsDisplayList.cpp:6434
(Diff revision 9)
> +    // We have both an SVG transform and an SVG children-only transform. Any
> +    // translation due to 'transform-origin' and/or 'transform-box' must apply
> +    // between the two, so we need to apply it now before we apply
> +    // transformFromSVGParent.
> +    //
> +    // PS:

Replace the "PS" section of this comment with:

    // (See the comment for nsSVGContainerFrame::HasChildrenOnlyTransform for
    // an explanation of what children-only transforms are.)

::: layout/painting/nsDisplayList.cpp:6438
(Diff revision 9)
> +    //
> +    // PS:
> +    // Please refer to the comment of
> +    // nsSVGContainerFrame::HasChildrenOnlyTransform for more information
> +    // regard to SVG children-only transform.
> +    result.ChangeBasis(aProperties.mToTransformOrigin);

Put a line break immediately after the comment above, then have a new comment directly before the ChangeBasis line:

// Apply any translation due to 'transform-origin' and/or 'transform-box':

::: layout/painting/nsDisplayList.cpp:6454
(Diff revision 9)
> -    // refBoxOffset.
> -    result.ChangeBasis(refBoxOffset);
> +      NSAppUnitsToFloatPixels(-frame->GetPosition().y, aAppUnitsPerPixel),
> +      0);
> +    Matrix4x4 transformFromParent =
> +      Matrix4x4::From2D(transformFromSVGParent).ChangeBasis(frameOffset);
> +
> +    // Compose frame's owned transform with the transform from parent

Please remove this comment. The comment further up should adequetly explain what's going on here, and this just adds noise IMO.
Attachment #8832911 - Flags: review?(jwatt) → review-
Actually, now that this code is being boiled down and simplified (yay!), it's clear that the |if (!hasSVGTransforms || !hasTransformFromSVGParent) {| branch can be removed. In fact, now that I think about it that branch is wrong since we will fail to apply the children-only transform if there is no SVG transform! So please replace all of the code:

  if (!hasSVGTransforms || !hasTransformFromSVGParent) {
    ...
  } else {
    ...
  }

with:

  if (hasTransformFromSVGParent) {
    float pixelsPerCSSPx =
      frame->PresContext()->AppUnitsPerCSSPixel() / aAppUnitsPerPixel;
    childrenOnlyTransform._31 *= pixelsPerCSSPx;
    childrenOnlyTransform._32 *= pixelsPerCSSPx;

    Point3D frameOffset(
      NSAppUnitsToFloatPixels(-frame->GetPosition().x, aAppUnitsPerPixel),
      NSAppUnitsToFloatPixels(-frame->GetPosition().y, aAppUnitsPerPixel),
      0);
    Matrix4x4 childrenOnlyTransform3D =
      Matrix4x4::From2D(childrenOnlyTransform).ChangeBasis(frameOffset);

    result *= childrenOnlyTransform3D;
  }

Also move the block of code starting with |Matrix4x4 perspectiveMatrix;| down to after the close of this new |if| block since it interrupts the logical flow of the code.

And move the line:

  bool hasTransformFromSVGParent =
    hasSVGTransforms && !transformFromSVGParent.IsIdentity();

down to just above the |if (hasTransformFromSVGParent) {| line, rename the variable to "hasChildrenOnlyTransformFromParent" and put the following comment before that line:

  // See the comment for nsSVGContainerFrame::HasChildrenOnlyTransform for
  // an explanation of what children-only transforms are.

I'll leave this as r- for now since I'd like to see the final patch.
Flags: needinfo?(cku)
Actually rename hasTransformFromSVGParent to parentHasChildrenOnlyTransform.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 45

6 months ago
mozreview-review
Comment on attachment 8832911 [details]
Bug 1335876 - Part 2. (Main) Setup basis of the matrix according to transform-box.

https://reviewboard.mozilla.org/r/109162/#review113780

::: layout/painting/nsDisplayList.cpp:6390
(Diff revision 10)
>    RuleNodeCacheConditions dummy;
>    bool dummyBool;
>    Matrix4x4 result;
>    // Call IsSVGTransformed() regardless of the value of
> -  // disp->mSpecifiedTransform, since we still need any transformFromSVGParent.
> -  Matrix svgTransform, transformFromSVGParent;
> +  // disp->mSpecifiedTransform, since we still need any childrenOnlyTransform.
> +  Matrix svgTransform, childrenOnlyTransform;

Now that I see these changes in context, this could be a bit easy for readers to think that "childrenOnlyTransform" is for 'frame's children. Can you rename this to parentsChildrenOnlyTransform?

::: layout/painting/nsDisplayList.cpp:6415
(Diff revision 10)
> -
> -  if (!hasSVGTransforms || !hasTransformFromSVGParent) {
> -    // This is a simplification of the following |else| block, the
> -    // simplification being possible because we don't need to apply
> -    // mToTransformOrigin between two transforms.
>      result.ChangeBasis(aProperties.mToTransformOrigin);

Seems I missed one part of the code changes from comment 40 when writing comment 41. This will break things. mToTransformOrigin needs to be applied outside the |if| block (it applies regardless of whether or not there are children-only transforms). So delete this line and put the following above the "See the comment for..." comment:

  // Apply any translation due to 'transform-origin' and/or 'transform-box':
  result.ChangeBasis(aProperties.mToTransformOrigin);
Attachment #8832911 - Flags: review?(jwatt) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 48

6 months ago
mozreview-review
Comment on attachment 8832911 [details]
Bug 1335876 - Part 2. (Main) Setup basis of the matrix according to transform-box.

https://reviewboard.mozilla.org/r/109162/#review113796

This patch makes the code significantly better. Thank you.
Attachment #8832911 - Flags: review?(jwatt) → review+
(For what it's worth it looks like the order in which perspective transforms are applied is wrong, since surely they should apply before the children-only transforms. I'm not sure about the ordering relative to mToTransformOrigin though. Anyway, fixing that is outside the scope of this bug and relatively low priority since we aren't seeing much use of perspective transforms in SVG.)
Flags: needinfo?(cku)

Comment 50

6 months ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e447c3fcd79
Part 1. Declare mTransformBox as StyleGeometryBox. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9d290cbe5aad
Part 2. Rename coord as transformOrigin. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3e160602fc1a
Part 3. (Main) Setup basis of the matrix according to transform-box. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/b870f9671bcd
Part 4. Reftest. r=jwatt

Comment 51

6 months ago
sorry had to back this out for stylo tc build bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=77417642&repo=autoland&lineNumber=8241
https://hg.mozilla.org/integration/autoland/rev/8d89ae502eb7
Flags: needinfo?(cku)
(Assignee)

Updated

6 months ago
Blocks: 1339674
(Assignee)

Updated

6 months ago
Flags: needinfo?(cku)
Whiteboard: [webcompat]
(Assignee)

Comment 52

6 months ago
I am going to move part 1 into another bug so that we can land stylo independent now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8832773 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
No longer blocks: 1339674

Comment 56

6 months ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7be8bbea7289
Part 1. Rename coord as transformOrigin. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b6bc87b2a558
Part 2. (Main) Setup basis of the matrix according to transform-box. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/9a7bfd0dd402
Part 3. Reftest. r=jwatt

Comment 57

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7be8bbea7289
https://hg.mozilla.org/mozilla-central/rev/b6bc87b2a558
https://hg.mozilla.org/mozilla-central/rev/9a7bfd0dd402
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

5 months ago
Depends on: 1345851
You need to log in before you can comment on or make changes to this bug.