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.

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

nsStyleDisplay::mTransformBox; // transform-box nsStyleDisplay::mTransformOrigin // transform-origin

The reference box for viewbox scaling transform is always view-box, the value of transform-box affects only css or svg transform.

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)

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

Comment on attachment 8833217 [details] Bug 1335876 - Part 1. Rename coord as transformOrigin. https://reviewboard.mozilla.org/r/109446/#review110580

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.

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

Comment on attachment 8832943 [details] Bug 1335876 - Part 3. Reftest. https://reviewboard.mozilla.org/r/109182/#review113526 Awesome, thank you!

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.

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.

Actually rename hasTransformFromSVGParent to parentHasChildrenOnlyTransform.

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);

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.

(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.)

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

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

I am going to move part 1 into another bug so that we can land stylo independent now.

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

https://hg.mozilla.org/mozilla-central/rev/7be8bbea7289 https://hg.mozilla.org/mozilla-central/rev/b6bc87b2a558 https://hg.mozilla.org/mozilla-central/rev/9a7bfd0dd402