Last Comment Bug 923193 - Support the 'transform-origin' property in SVG and implement the 'transform-box' property
: Support the 'transform-origin' property in SVG and implement the 'transform-b...
Status: RESOLVED FIXED
[parity-chrome]
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
-- normal with 21 votes (vote)
: mozilla41
Assigned To: Jonathan Watt [:jwatt]
:
: Jet Villegas (:jet)
Mentors:
: 891074 936136 1013421 1057398 1070774 (view as bug list)
Depends on: 599732 821188 1135548 929021 929966 931369 931422 932762 933354 935008 935181 1159053 1171333 1335876
Blocks: 1208550 891074 1013421 1102145 1151258 1175492 1209853
  Show dependency treegraph
 
Reported: 2013-10-02 10:05 PDT by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2017-02-01 14:18 PST (History)
35 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
disabled
fixed
41+


Attachments
fix (2.30 KB, patch)
2013-10-02 10:33 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
cam: review+
Details | Diff | Splinter Review
fix v2 (12.93 KB, patch)
2013-10-16 02:22 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
cam: review+
Details | Diff | Splinter Review
backouts (16.09 KB, patch)
2013-11-10 19:59 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
updated patch -> still has issues with redrawing (17.22 KB, patch)
2014-10-20 20:02 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
part 1 - defer calculation of the reference box until it's needed (38.98 KB, patch)
2015-04-29 08:51 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
part 1 - defer calculation of the reference box until it's needed (50.50 KB, patch)
2015-04-29 12:52 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
part 1 - defer calculation of the reference box until it's needed (50.53 KB, patch)
2015-04-29 13:06 PDT, Jonathan Watt [:jwatt]
roc: review+
Details | Diff | Splinter Review
part 2 - Implement transform-origin for SVG (14.12 KB, patch)
2015-05-06 08:45 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
part 3 - implement the transform-box property (14.05 KB, patch)
2015-05-19 20:56 PDT, Jonathan Watt [:jwatt]
cam: review+
Details | Diff | Splinter Review
part 4 - implement the transform-origin property in SVG (16.40 KB, patch)
2015-05-19 22:30 PDT, Jonathan Watt [:jwatt]
cam: review+
Details | Diff | Splinter Review
part 5 - tests for the 'transform-origin' and 'transform-box' properties in SVG (7.63 KB, patch)
2015-05-19 23:32 PDT, Jonathan Watt [:jwatt]
cam: review+
Details | Diff | Splinter Review
part 2 - add preference with defaults (922 bytes, patch)
2015-05-26 18:07 PDT, Jonathan Watt [:jwatt]
jwatt: review+
Details | Diff | Splinter Review

Description User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-02 10:05:41 PDT
This isn't really specified, but using the SVG bbox as the defining box for transform-origin is what Chrome does and that's completely logical, so let's do it.
Comment 1 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-02 10:33:43 PDT
Created attachment 813185 [details] [diff] [review]
fix
Comment 2 User image Robert Longson 2013-10-02 10:34:54 PDT
Looks like bug 891074 is a duplicate of this.
Comment 3 User image Dirk Schulze 2013-10-02 11:09:26 PDT
All percentages and keywords are resolved according to the bbox. Absolute length based on 0,0 on the CTM (as done in SVG currently).

Example:

"0% 0%" is at the bbox top left. As is "top left".
"0px 0px" is at the origin of the current CTM.

I am sure that it is specified in a general manner. But need to check where myself.
Comment 4 User image Cameron McCormack (:heycam) 2013-10-02 13:17:36 PDT
Comment on attachment 813185 [details] [diff] [review]
fix

Review of attachment 813185 [details] [diff] [review]:
-----------------------------------------------------------------

Whoever cares about UNIFIED_CONTINUATIONS might appreciate having the same changes in the other GetFrameBoundsForTransform, though I guess we don't have to do it now.  r=me
Comment 5 User image Cameron McCormack (:heycam) 2013-10-02 13:18:47 PDT
Can you add a test with a non-percentage (or side keyword) transform-origin value too?
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-03 05:53:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef00ffc6c12

(In reply to Cameron McCormack (:heycam) from comment #5)
> Can you add a test with a non-percentage (or side keyword) transform-origin
> value too?

I did that. I noticed that with this implementation, transform-origin absolute values are relative to the element's SVG user space, not the top-left of the element's SVG bbox, which isn't what I intuitively expected, but it's what Dirk said in comment #3 and it matches Chrome, so I went with it.
Comment 7 User image Dirk Schulze 2013-10-03 06:19:56 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef00ffc6c12
> 
> (In reply to Cameron McCormack (:heycam) from comment #5)
> > Can you add a test with a non-percentage (or side keyword) transform-origin
> > value too?
> 
> I did that. I noticed that with this implementation, transform-origin
> absolute values are relative to the element's SVG user space, not the
> top-left of the element's SVG bbox, which isn't what I intuitively expected,
> but it's what Dirk said in comment #3 and it matches Chrome, so I went with
> it.

Great! I just checked the spec: http://dev.w3.org/csswg/css-transforms/#propdef-transform-origin
It is specified the way as described in comment 3. Please send an email to www-style if you think it is not sufficient enough.

But you maybe looked it up in the WD which is really outdated :(.
Comment 8 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-03 07:02:05 PDT
Backed out due to test failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=28713653&tree=Mozilla-Inbound&full=1#error1
To be honest I'm not really sure why we're hitting this!
Comment 9 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-03 07:06:29 PDT
OK, in some cases we're in UpdateImageVisibility which apparently doesn't flush reflow before building a display list.

In another case we're in nsIFrame::FinishAndStoreOverflow and we need the computed transform-origin to determine our overflow rect:
https://tbpl.mozilla.org/php/getParsedLog.php?id=28713927&tree=Mozilla-Inbound&full=1#error1
I don't understand this one. We should have finished reflowing all the descendants of the transformed element before we reach FinishAndStoreOverflow.
Comment 10 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-03 07:14:17 PDT
Oddly I can't reproduce either of these failures locally :-(
Comment 11 User image Cameron McCormack (:heycam) 2013-10-03 07:20:16 PDT
In nsSVGTextFrame2::ReflowSVG, we clear the reflow bits just after we call FinishAndStoreOverflow.  So we are "done" with reflowing the frame by that point.  I wonder if it matters if we clear those bits before calling FinishAndStoreOverflow?
Comment 12 User image Cameron McCormack (:heycam) 2013-10-03 07:23:12 PDT
Also, does this patch mean we're going to call GetBBoxContribution every time we reflow an SVG frame?  Isn't that going to be a bit expensive, e.g. if the frame has many children and we're unioning a bunch of rects?
Comment 13 User image cory 2013-10-04 11:32:34 PDT
I'm experiencing this problem right now: http://codepen.io/corysimmons/pen/qspwE :(
Comment 14 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-07 21:03:25 PDT
But there are other failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0ef00ffc6c12
Comment 15 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-07 21:06:58 PDT
(In reply to Cameron McCormack (:heycam) from comment #12)
> Also, does this patch mean we're going to call GetBBoxContribution every
> time we reflow an SVG frame?  Isn't that going to be a bit expensive, e.g.
> if the frame has many children and we're unioning a bunch of rects?

Every time we paint a transformed frame. I don't know if it will be significantly expensive. If it is we could cache the bbox rect as a property on the SVG frame.
Comment 16 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-14 01:59:04 PDT
https://tbpl.mozilla.org/?tree=Try&rev=68cb550dc7cf
Comment 17 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-14 04:24:44 PDT
The assertion is still failing. It isn't surprising that moving the state-bit-clearing to before FinishAndStoreOverflow didn't fix anything, since it's the state bits on the anonymous child that are causing the assertion failure, not the state bits on the nsSVGTextFrame2 itself.

We're failing because nsDisplayTransform::GetDeltaToMozPerspectiveOrigin calls nsDisplayTransform::GetFrameBoundsForTransform on its parent frame, which hasn't been fully reflowed yet :-(. The first child is finishing its reflow, but the second child hasn't been reflowed.

I don't know why nsDisplayTransform::GetDeltaToMozPerspectiveOrigin is checking the perspective-origin of the parent frame. Shouldn't we be checking the perspective-origin of the transformed frame?
Comment 18 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-14 04:36:19 PDT
OK, now that I've read the spec I understand why we do that.

So I think we have a fundamental spec issue here when we have an SVG element using 'perspective-origin' with a child SVG element using a 3D transform:
-- Evaluating perspective-origin for the SVG element depends on the bounding-box of the element.
-- The bounding-box of the element depends on the transform of its child.
-- The transform of the child depends on the perspective-origin of the parent.
Am I right?
Comment 19 User image Dirk Schulze 2013-10-14 04:53:06 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> OK, now that I've read the spec I understand why we do that.
> 
> So I think we have a fundamental spec issue here when we have an SVG element
> using 'perspective-origin' with a child SVG element using a 3D transform:
> -- Evaluating perspective-origin for the SVG element depends on the
> bounding-box of the element.
If you have percentage values, yes.

> -- The bounding-box of the element depends on the transform of its child.
Right.

> -- The transform of the child depends on the perspective-origin of the
> parent.
Why would that be the case? perspective and perspective-orgin compute the perspective projection matrix. This matrix is pre-multiplied to the transformation matrix of the element.

Maybe you are concerned that it is the other way around? That the bounding box should include the perspective projection matrix of the child? I did not thought about this particular scenario and wonder what we do on getClientRect for HTML.

> Am I right?
Comment 20 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-14 12:58:20 PDT
(In reply to Dirk Schulze from comment #19)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> > -- The transform of the child depends on the perspective-origin of the
> > parent.
> Why would that be the case? perspective and perspective-orgin compute the
> perspective projection matrix. This matrix is pre-multiplied to the
> transformation matrix of the element.
> 
> Maybe you are concerned that it is the other way around? That the bounding
> box should include the perspective projection matrix of the child?

However you want to describe it, the perspective projection matrix affects the ink bounds of the filled region of the child element, and therefore affects the SVG bounding box of the parent.
Comment 21 User image Dirk Schulze 2013-10-14 13:38:58 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> (In reply to Dirk Schulze from comment #19)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> > > -- The transform of the child depends on the perspective-origin of the
> > > parent.
> > Why would that be the case? perspective and perspective-orgin compute the
> > perspective projection matrix. This matrix is pre-multiplied to the
> > transformation matrix of the element.
> > 
> > Maybe you are concerned that it is the other way around? That the bounding
> > box should include the perspective projection matrix of the child?
> 
> However you want to describe it, the perspective projection matrix affects
> the ink bounds of the filled region of the child element, and therefore
> affects the SVG bounding box of the parent.

I think that is the question. Should it really affect the bounding box at all? he bounding box is still 2D at this point. We should discuss this on the mailing list.
Comment 22 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-16 02:22:00 PDT
Created attachment 817710 [details] [diff] [review]
fix v2

This has changed significantly from the previous patch. I added some stuff to nsSVGTextFrame2::GetBBoxContribution to bail out if we have a dirty anonymous child. The comment there explains why I think this is safe.

I also modified nsDisplayTransform::GetDeltaToMozTransformOrigin a fair bit since the logic there in the previous patch was quite wrong.

It turns out that in Gecko the circular dependency chain in comment #18 doesn't happen because our SVG bounding box only takes SVG transforms into account, and ignores CSS transforms, and we don't support 3D SVG transforms. We still have to resolve the spec issue and implement it, but that should be a separate bug.
Comment 23 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-16 02:22:18 PDT
Green on try, by the way. https://tbpl.mozilla.org/?tree=Try&rev=6c51c9e587d8
Comment 24 User image Cameron McCormack (:heycam) 2013-10-16 16:19:03 PDT
Comment on attachment 817710 [details] [diff] [review]
fix v2

Review of attachment 817710 [details] [diff] [review]:
-----------------------------------------------------------------

I think it's a bit odd that 'transform-origin: 50% 50%' is interpreted as being relative to the bbox of the SVG element, while 'transform-origin: calc(50% + 0px, 50% + 0px)' isn't.  There must be a better solution for the spec.  For the moment though, it's fine.
Comment 25 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-17 03:05:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a417424f9213
Comment 26 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-10-18 01:03:09 PDT
I think this caused a bunch of TSVG ASAP regressions.
Comment 27 User image Cameron McCormack (:heycam) 2013-10-18 02:47:20 PDT
If that's so, maybe we can have GetFrameBoundsForTransform only be called when we have percentage values to resolve against it?
Comment 28 User image Carsten Book [:Tomcat] 2013-10-18 02:52:59 PDT
https://hg.mozilla.org/mozilla-central/rev/a417424f9213
Comment 29 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-21 10:06:32 PDT
Filed bug 929021.
Comment 30 User image Jonathan Watt [:jwatt] 2013-10-30 06:24:43 PDT
roc: are there further things you plan on doing to undo the perf regression from this? We don't seem to be back to where we were:

http://graphs.mozilla.org/graph.html#tests=[[281,63,29]]&sel=none&displayrange=30&datatype=running
Comment 31 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-10 18:33:57 PST
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb151446ec5

We need these backouts on Aurora too.
Comment 32 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-10 18:37:20 PST
I will re-fix this when bug 935008 (the perf regression from bug 922942) is fixed.
Comment 33 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-10 19:59:17 PST
Created attachment 829981 [details] [diff] [review]
backouts

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 923193
User impact if declined: not much, maybe SVG performance regressions
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): not much. Just backouts.
String or IDL/UUID changes made by this patch: none
Comment 34 User image bhavana bajaj [:bajaj] 2013-11-11 10:31:37 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Created attachment 829981 [details] [diff] [review]
> backouts
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 923193
> User impact if declined: not much, maybe SVG performance regressions
> Testing completed (on m-c, etc.): none
> Risk to taking this patch (and alternatives if risky): not much. Just
> backouts.
> String or IDL/UUID changes made by this patch: none

:roc ,Is this the full backout of this new feature on Fx27 ? Unclear if the feature is going to stick in Fx27, can you confirm ?
Comment 36 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-11 18:20:58 PST
(In reply to bhavana bajaj [:bajaj] from comment #34)
> :roc ,Is this the full backout of this new feature on Fx27 ? Unclear if the
> feature is going to stick in Fx27, can you confirm ?

Yes. The feature is gone on trunk and it needs to go from Aurora too.
Comment 37 User image Mats Palmgren (:mats) 2013-11-19 09:46:14 PST
Tracking so we don't forget to land the backouts also on Aurora.
Comment 38 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-20 19:48:41 PST
backed out on Aurora. https://hg.mozilla.org/releases/mozilla-aurora/rev/2e429a8f2949
Comment 39 User image Ryan VanderMeulen [:RyanVM] 2013-11-21 04:55:45 PST
Backed out for reftest failures on all platforms.
https://hg.mozilla.org/releases/mozilla-aurora/rev/599e2ddfb6a9

https://tbpl.mozilla.org/php/getParsedLog.php?id=30877127&tree=Mozilla-Aurora

BTW, tree rules say you should be watching your pushes to Aurora, but based on the 36 failures I just starred (including *nightly* builds), I would say that didn't happen.
Comment 40 User image (dormant account) 2013-11-21 11:35:09 PST
*** Bug 936136 has been marked as a duplicate of this bug. ***
Comment 41 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-12-01 12:54:30 PST
Re-pushed, both changesets this time:
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e5824f3b572
https://hg.mozilla.org/releases/mozilla-aurora/rev/90c307e8c79a
Comment 42 User image bhavana bajaj [:bajaj] 2013-12-05 10:25:30 PST
[comment to Relman] - I have recently removed this from our Relnote DB so it does not appear on release notes that will be generated.
Comment 43 User image Lukas Blakk [:lsblakk] use ?needinfo 2013-12-05 14:51:03 PST
Resetting the flags to show that this is still in Firefox 28 (which is about to go to Aurora) and should be noted there, if it can stay in.  Please correct if I am reading this wrong, otherwise we'll put this in the Aurora notes for 28.
Comment 44 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-12-05 19:12:23 PST
This is not fixed in 28.
Comment 45 User image :Gijs 2014-05-21 08:14:26 PDT
*** Bug 1013421 has been marked as a duplicate of this bug. ***
Comment 46 User image Robert Longson 2014-05-29 22:55:21 PDT
*** Bug 891074 has been marked as a duplicate of this bug. ***
Comment 47 User image Rijk 2014-07-31 10:43:10 PDT Comment hidden (me-too)
Comment 48 User image Jonathan Watt [:jwatt] 2014-08-22 06:02:39 PDT
Fixing bug 932762 should make calculating bboxes a lot less expensive and may by itself reduce the perf hit from the backed out patch to an acceptable amount.
Comment 49 User image Robert Longson 2014-08-22 08:56:11 PDT
*** Bug 1057398 has been marked as a duplicate of this bug. ***
Comment 50 User image Paul LeBeau 2014-09-25 21:37:31 PDT Comment hidden (advocacy)
Comment 51 User image BenjaminRH 2014-10-01 04:00:25 PDT Comment hidden (advocacy)
Comment 52 User image lance 2014-10-01 17:57:36 PDT Comment hidden (advocacy)
Comment 53 User image Rik Cabanier 2014-10-20 20:02:59 PDT
Created attachment 8508425 [details] [diff] [review]
updated patch -> still has issues with redrawing
Comment 54 User image Dirk Schulze 2014-11-21 08:41:58 PST
CSS Tansforms spec got updated and has a new property "transform-box". This should clarify things around fixed and percentage values for transform-origin and hopefully makes implementation easier now.
Comment 55 User image Robert Longson 2015-02-12 12:01:05 PST
*** Bug 1070774 has been marked as a duplicate of this bug. ***
Comment 56 User image mail 2015-03-14 05:23:12 PDT
Hi, I'm not sure if this is the right bug. But before I open another duplicate...

FF 36.0.1 supports transform-origin. Great.
But the origin coordinates doesn't scale with a stretched svg.
http://codepen.io/anon/pen/vEQBgR
There should be a centered pointer with a 45 deg rotation. FF does rotate the path correct, but place it in the wrong place.

Chrome(41.0.2272.89), Safari(8.0.3), Opera(28.0) render it right. IE doesn't support css svg transforms.
Comment 57 User image Jonathan Watt [:jwatt] 2015-04-27 12:32:14 PDT
(In reply to Dirk Schulze from comment #54)
> CSS Tansforms spec got updated and has a new property "transform-box". This
> should clarify things around fixed and percentage values for
> transform-origin and hopefully makes implementation easier now.

For reference:

http://dev.w3.org/csswg/css-transforms/#propdef-transform-box

And background discussion:

http://www.w3.org/2014/10/30-fx-irc#T22-03-35
Comment 58 User image Jonathan Watt [:jwatt] 2015-04-29 08:51:10 PDT
Created attachment 8599362 [details] [diff] [review]
part 1 - defer calculation of the reference box until it's needed

Running Talos locally it seems that my patch for bug 1159053 and the other optimizations that were landed previously are not sufficient to avoid regressing performance. This patch tries to avoid calculation of the reference box unless it is actually needed.
Comment 59 User image Jonathan Watt [:jwatt] 2015-04-29 12:52:29 PDT
Created attachment 8599473 [details] [diff] [review]
part 1 - defer calculation of the reference box until it's needed
Comment 60 User image Jonathan Watt [:jwatt] 2015-04-29 13:06:32 PDT
Created attachment 8599485 [details] [diff] [review]
part 1 - defer calculation of the reference box until it's needed
Comment 61 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2015-04-30 04:04:29 PDT
Comment on attachment 8599485 [details] [diff] [review]
part 1 - defer calculation of the reference box until it's needed

Review of attachment 8599485 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleTransformMatrix.h
@@ +69,5 @@
> +
> +    void Initialize();
> +
> +    nscoord Width() {
> +      if (!mIsInitialized) {

if (!mIsInitialized) check is redundant. Just remove it. Change "Initialize" to "EnsureInitialized" to reflect our naming conventions for conditional initialization methods.
Comment 63 User image Wes Kocher (:KWierso) 2015-05-01 17:20:40 PDT
https://hg.mozilla.org/mozilla-central/rev/94ef7c315d16
Comment 65 User image Jonathan Watt [:jwatt] 2015-05-01 18:28:39 PDT
I repushed the patch from bug 933354, but attributed it to this bug since it's an optimization for transform-origin.
Comment 66 User image Phil Ringnalda (:philor) 2015-05-02 10:04:30 PDT
https://hg.mozilla.org/mozilla-central/rev/7cb6608e6680
Comment 67 User image Jonathan Watt [:jwatt] 2015-05-06 08:45:00 PDT
Created attachment 8602151 [details] [diff] [review]
part 2 - Implement transform-origin for SVG
Comment 68 User image Cameron McCormack (:heycam) 2015-05-06 17:22:09 PDT
Comment on attachment 8602151 [details] [diff] [review]
part 2 - Implement transform-origin for SVG

Review of attachment 8602151 [details] [diff] [review]:
-----------------------------------------------------------------

Forgive me for asking for some clarifications that I didn't ask on roc's first patch (maybe I've forgotten how this works since then).

::: layout/base/nsDisplayList.cpp
@@ +4625,4 @@
>        coords[index] =
>          NSAppUnitsToFloatPixels((refBox.*dimensionGetter[index])(), aAppUnitsPerPixel) *
> +          coord.GetPercentValue()
> +      // XXXjwatt + NSAppUnitsToFloatPixels(boundingOffsets[index], aAppUnitsPerPixel)

What's this about?

@@ +4643,5 @@
> +        // Non-<percentage> values represent offsets from the origin of the SVG
> +        // element's user space ({0,0} in frame space), not the top left of its
> +        // bounds:
> +        coords[index] -=
> +          NSAppUnitsToFloatPixels(frameOffsets[index], aAppUnitsPerPixel);

Why do we only perform this adjustment for non-percentage values?  Won't this cause calc(50% + 0px) and 50% to be different (i.e. the former will have the frame offset adjustment but the latter won't)?

@@ +4869,5 @@
>                          0);
> +
> +  Point3D offsetBetweenOrigins = roundedOrigin;
> +  if (hasSVGTransforms) {
> +    Point origpoint = transformFromSVGParent * Point(aProperties.mToTransformOrigin.x, aProperties.mToTransformOrigin.y);

Can you explain why we have this transformFromSVGParent multiplication in here now?

@@ +4872,5 @@
> +  if (hasSVGTransforms) {
> +    Point origpoint = transformFromSVGParent * Point(aProperties.mToTransformOrigin.x, aProperties.mToTransformOrigin.y);
> +    offsetBetweenOrigins += Point3D(origpoint.x, origpoint.y, 0);
> +  }
> +  else {

else on previous line.

::: layout/reftests/transform/reftest.list
@@ +126,5 @@
>  == inline-1a.html inline-1-ref.html
> +== transform-origin-svg-1a.svg transform-origin-svg-1-ref.svg
> +== transform-origin-svg-1b.svg transform-origin-svg-1-ref.svg
> +== transform-origin-svg-2a.svg transform-origin-svg-2-ref.svg
> +== transform-origin-svg-2b.svg transform-origin-svg-2-ref.svg

Can you add some tests with calc values (some with percentages and some without percentages)?

::: layout/reftests/transform/transform-origin-svg-1b.svg
@@ +1,5 @@
> +<svg xmlns='http://www.w3.org/2000/svg'>
> +<g transform="translate(30,30)">
> +  <rect x='10' y='10' width='100' height='100' fill='lime'
> +        style="transform:rotate(90deg); transform-origin:10px 110px;
> +               -webkit-transform:rotate(90deg); -webkit-transform-origin:10px 110px;"/>

Do we need these -webkit-* properties in the test?

::: layout/svg/SVGTextFrame.cpp
@@ +3988,5 @@
> +    // been reflowed, e.g. if an earlier sibling is calling FinishAndStoreOverflow and
> +    // needs our parent's perspective matrix, which depends on the SVG bbox
> +    // contribution of this frame. In the latter situation, when all siblings have
> +    // been reflowed, the parent will compute its perspective and rerun
> +    // FinishAndStoreOverflow for all its children.

Where is this done, btw?
Comment 69 User image Cameron McCormack (:heycam) 2015-05-18 18:21:08 PDT
Comment on attachment 8602151 [details] [diff] [review]
part 2 - Implement transform-origin for SVG

Cancelling review while awaiting answers to the questions above.
Comment 70 User image Jonathan Watt [:jwatt] 2015-05-19 20:56:54 PDT
Created attachment 8607921 [details] [diff] [review]
part 3 - implement the transform-box property

This implements the property, although setting it won't have any affect until the future parts in this patch series.
Comment 71 User image Cameron McCormack (:heycam) 2015-05-19 21:12:20 PDT
Comment on attachment 8607921 [details] [diff] [review]
part 3 - implement the transform-box property

Review of attachment 8607921 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine.  Since this property isn't widely implemented yet, and the spec is (still!) a Working Draft, can you put it behind a pref, enabled by default in release builds if this bug will implement its entire functionality?
Comment 72 User image Jonathan Watt [:jwatt] 2015-05-19 22:21:29 PDT
(In reply to Cameron McCormack (:heycam) from comment #68)
> @@ +3988,5 @@
> > +    // been reflowed, e.g. if an earlier sibling is calling FinishAndStoreOverflow and
> > +    // needs our parent's perspective matrix, which depends on the SVG bbox
> > +    // contribution of this frame. In the latter situation, when all siblings have
> > +    // been reflowed, the parent will compute its perspective and rerun
> > +    // FinishAndStoreOverflow for all its children.
> 
> Where is this done, btw?

See RecomputePerspectiveChildrenOverflow.
Comment 73 User image Jonathan Watt [:jwatt] 2015-05-19 22:30:27 PDT
Created attachment 8607931 [details] [diff] [review]
part 4 - implement the transform-origin property in SVG

This has changed quite a lot, and obsoletes your previous review questions I think. (At least the ones that I haven't answered.)
Comment 74 User image Cameron McCormack (:heycam) 2015-05-19 22:47:09 PDT
Comment on attachment 8607931 [details] [diff] [review]
part 4 - implement the transform-origin property in SVG

Review of attachment 8607931 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleTransformMatrix.cpp
@@ +51,5 @@
>  
>    if (mFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT) {
> +    if (mFrame->StyleDisplay()->mTransformBox !=
> +          NS_STYLE_TRANSFORM_BOX_FILL_BOX) {
> +      mX = -mFrame->GetRect().x;

Can you add a comment in here saying that this is the view-box behaviour, and that for SVG frames border-box (the only other value) is treated the same as view-box?

::: layout/svg/nsSVGUtils.h
@@ +250,5 @@
>     * Update the filter invalidation region for ancestor frames, if relevant.
>     */
>    static void NotifyAncestorsOfFilterRegionChange(nsIFrame *aFrame);
>  
> +  static Size GetContextSize(const nsIFrame* aFrame);

Add a comment to this?
Comment 75 User image Jonathan Watt [:jwatt] 2015-05-19 23:32:45 PDT
Created attachment 8607945 [details] [diff] [review]
part 5 - tests for the 'transform-origin' and 'transform-box' properties in SVG
Comment 76 User image Cameron McCormack (:heycam) 2015-05-19 23:43:02 PDT
Comment on attachment 8607945 [details] [diff] [review]
part 5 - tests for the 'transform-origin' and 'transform-box' properties in SVG

Review of attachment 8607945 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Can you just add one more test to verify that transform-box:view-box works just like the initial value?
Comment 79 User image Jonathan Watt [:jwatt] 2015-05-26 18:07:38 PDT
Created attachment 8610943 [details] [diff] [review]
part 2 - add preference with defaults

(In reply to Cameron McCormack (:heycam) from comment #71)
> This looks fine.  Since this property isn't widely implemented yet, and the
> spec is (still!) a Working Draft, can you put it behind a pref, enabled by
> default in release builds if this bug will implement its entire
> functionality?

I landed the pref defaults as 'part 2'. The pref is svg.transform-origin.enabled
Comment 80 User image Jonathan Watt [:jwatt] 2015-05-26 18:13:04 PDT
(In reply to Cameron McCormack (:heycam) from comment #71)
> Comment on attachment 8607921 [details] [diff] [review]
> part 4 - implement the transform-box property
> 
> Review of attachment 8607921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine.  Since this property isn't widely implemented yet, and the
> spec is (still!) a Working Draft, can you put it behind a pref, enabled by
> default in release builds if this bug will implement its entire
> functionality?

Landed with modifications as https://hg.mozilla.org/integration/mozilla-inbound/rev/51df0002d574

I discovered that transforms under children-only transforms (i.e. viewBox) were broken. I ended up having a lot of trouble with this, but in the end I fixed it with changes to nsDisplayTransform::GetResultingTransformMatrixInternal. The comments in the code should explain what's going on and why.
Comment 82 User image Ryan VanderMeulen [:RyanVM] 2015-05-28 09:39:39 PDT
Comment on attachment 829981 [details] [diff] [review]
backouts

Removing this old approval so this bug doesn't turn up in the "needs uplift" bug queries.
Comment 83 User image Cameron McCormack (:heycam) 2015-05-28 15:36:48 PDT
Release Note Request (optional, but appreciated)
[Why is this notable]: transform-origin applying to SVG elements is a feature many SVG developers have been waiting for
[Suggested wording]: Added support for the transform-origin property on SVG elements
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/CSS/transform-origin

Not sure whether transform-box is worth mentioning just yet?
Comment 84 User image Jean-Yves Perrier [:teoli] 2015-06-14 04:30:24 PDT
SebastianZ, do you think you can find some time to document transform-box (Spec: http://dev.w3.org/csswg/css-transforms/#transform-box )?
Comment 85 User image Sebastian Zartner [:sebo] 2015-06-14 16:17:22 PDT
Sure, I'll have a look tomorrow.

Sebastian
Comment 86 User image Sebastian Zartner [:sebo] 2015-06-15 01:05:17 PDT
(In reply to Cameron McCormack (:heycam) from comment #83)
> Not sure whether transform-box is worth mentioning just yet?

In case it's worth mentioning it, it is now documented at https://developer.mozilla.org/en-US/docs/Web/CSS/transform-box.
Comment 87 User image Cameron McCormack (:heycam) 2015-06-15 01:08:53 PDT
Thank you Sebastian!
Comment 88 User image Cameron McCormack (:heycam) 2015-06-15 01:11:35 PDT
While transform-box should remain behind the pref for the moment IMO, I think transform-origin's handling of percentages in SVG content is probably safe to unpref.  WDYT jwatt?
Comment 89 User image Jonathan Watt [:jwatt] 2015-06-17 04:31:32 PDT
I think I'm probably okay with that since for consistency with percentage values in CSS box model contexts I can't imagine us having the default be anything other than being a percentage of the fill bbox.
Comment 90 User image Ritu Kothari (:ritu) 2015-07-01 17:32:22 PDT
Release note added to Firefox 41.0a2
Comment 91 User image igloczek 2015-09-29 08:06:00 PDT
Still doesn't work... Look at these examples, FF 41.0 sucks on both of them (and in my projects too)
http://codepen.io/corysimmons/pen/qspwE
http://codepen.io/mrrocks/pen/EiplA
Comment 92 User image Daniel Holbert [:dholbert] 2015-09-29 08:56:15 PDT
Both of those testcases look *much* better in Firefox Nightly, from http://nightly.mozilla.org/

(Though Nightly doesn't quite match Chrome on the latter one -- in Nightly, the animated dot of the "i" in Axis seems to drift left and right a little bit, between the A and the i.)
Comment 93 User image Daniel Holbert [:dholbert] 2015-09-29 21:37:17 PDT
(In reply to Daniel Holbert [:dholbert] from comment #92)
> (Though Nightly doesn't quite match Chrome on the latter one -- in Nightly,
> the animated dot of the "i" in Axis seems to drift left

(Sorry, I meant the former one. Anyway, I filed bug 1209853 on that; let's take further discussion on this over there.)
Comment 94 User image Sebastian Zartner [:sebo] 2015-09-30 00:29:17 PDT
(In reply to Daniel Holbert [:dholbert] from comment #92)
> Both of those testcases look *much* better in Firefox Nightly, from
> http://nightly.mozilla.org/

I updated the documentation for transform-origin now:
https://developer.mozilla.org/en-US/docs/Web/CSS/transform-origin#Browser_compatibility

Though I'm a bit confused on this. What's the fix that improved the look? In Firefox 41.0 and 42.0b2 it looks like keywords and percentages are not interpreted at all, while they are in DevEdition 43.0a2 (2015-09-29) and Nightly 44.0a1 (2015-09-29).

Sebastian
Comment 95 User image David Baron :dbaron: ⌚️UTC-8 2015-09-30 01:08:35 PDT
(In reply to Sebastian Zartner [:sebo] from comment #94)
> Though I'm a bit confused on this. What's the fix that improved the look? In
> Firefox 41.0 and 42.0b2 it looks like keywords and percentages are not
> interpreted at all, while they are in DevEdition 43.0a2 (2015-09-29) and
> Nightly 44.0a1 (2015-09-29).

Much of the work in this bug was not enabled in this bug.  The enabling is covered by two bugs:

bug 1175492 - unpref transform-origin percentage handling for SVG elements
bug 1208550 - ship support for the 'transform-box' property
Comment 96 User image Sebastian Zartner [:sebo] 2015-09-30 04:23:29 PDT
Thanks for the info, David! I've adjusted the documentation of the two properties accordingly.

Sebastian
Comment 97 User image yem_salat 2015-12-02 23:39:52 PST Comment hidden (off-topic)

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