923193 - Support the 'transform-origin' property in SVG and implement the 'transform-box' property

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 2

[Robert Longson [:longsonr]]

Looks like bug 891074 is a duplicate of this.

Comment 3

[Dirk Schulze]

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

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

Can you add a test with a non-percentage (or side keyword) transform-origin value too?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Dirk Schulze]

(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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oddly I can't reproduce either of these failures locally :-(

Comment 11

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

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

[cory]

I'm experiencing this problem right now: http://codepen.io/corysimmons/pen/qspwE :(

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But there are other failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0ef00ffc6c12

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=68cb550dc7cf

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Dirk Schulze]

(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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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

[Dirk Schulze]

(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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Green on try, by the way. https://tbpl.mozilla.org/?tree=Try&rev=6c51c9e587d8

Comment 24

[Cameron McCormack (:heycam)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a417424f9213

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think this caused a bunch of TSVG ASAP regressions.

Comment 27

[Cameron McCormack (:heycam)]

If that's so, maybe we can have GetFrameBoundsForTransform only be called when we have percentage values to resolve against it?

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 929021.

Comment 30

[Jonathan Watt [:jwatt]]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb151446ec5

We need these backouts on Aurora too.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I will re-fix this when bug 935008 (the perf regression from bug 922942) is fixed.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[bhavana bajaj [:bajaj]]

(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 35

[Ryan VanderMeulen [:RyanVM]]

Merge of backouts:
https://hg.mozilla.org/mozilla-central/rev/3fb151446ec5
https://hg.mozilla.org/mozilla-central/rev/4a5c39f4cf22

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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

[Mats Palmgren (inactive)]

Tracking so we don't forget to land the backouts also on Aurora.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

backed out on Aurora. https://hg.mozilla.org/releases/mozilla-aurora/rev/2e429a8f2949

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[bhavana bajaj [:bajaj]]

[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

[Lukas Blakk [:lsblakk] use ?needinfo]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is not fixed in 28.

Comment 47

[Rijk]

Still bugging us: http://codepen.io/rijk/pen/vfmHa

http://stackoverflow.com/questions/25064859/transform-origin-not-working-properly-with-svg-in-firefox

Comment 48

[Jonathan Watt [:jwatt]]

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 50

[Paul LeBeau]

Just a friendly bump.

Note: Even just applying "transform-origin: 50% 50%" to an SVG element will cause the SVG to be rendered incorrectly.  Even without any animation applied.

See: http://jsfiddle.net/eekqyfjp/

Comment 51

[BenjaminRH]

Yeah, I'm having this issue as well. It would be wonderful to be able to use things like http://codepen.io/mrrocks/pen/EiplA

Comment 52

[lance]

I would give you guys so many hugs if you fixed this. I'm running into this constantly.

Comment 53

[Rik Cabanier]

Attachment: updated patch -> still has issues with redrawing

Comment 54

[Dirk Schulze]

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 56

[mail]

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

[Jonathan Watt [:jwatt]]

(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

[Jonathan Watt [:jwatt]]

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

[Jonathan Watt [:jwatt]]

Attachment: part 1 - defer calculation of the reference box until it's needed

Comment 60

[Jonathan Watt [:jwatt]]

Attachment: part 1 - defer calculation of the reference box until it's needed

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 62

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94ef7c315d16

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/94ef7c315d16

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb6608e6680

Comment 65

[Jonathan Watt [:jwatt]]

I repushed the patch from bug 933354, but attributed it to this bug since it's an optimization for transform-origin.

Comment 66

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7cb6608e6680

Comment 67

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Implement transform-origin for SVG

Comment 68

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

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

[Jonathan Watt [:jwatt]]

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

[Cameron McCormack (:heycam)]

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

[Jonathan Watt [:jwatt]]

(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

[Jonathan Watt [:jwatt]]

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

[Cameron McCormack (:heycam)]

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

[Jonathan Watt [:jwatt]]

Attachment: part 5 - tests for the 'transform-origin' and 'transform-box' properties in SVG

Comment 76

[Cameron McCormack (:heycam)]

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 77

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e56ad034e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c02250c4d3c9

Comment 78

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51df0002d574
https://hg.mozilla.org/integration/mozilla-inbound/rev/be715c1edaeb

Comment 79

[Jonathan Watt [:jwatt]]

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

[Jonathan Watt [:jwatt]]

(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 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4e56ad034e5
https://hg.mozilla.org/mozilla-central/rev/c02250c4d3c9
https://hg.mozilla.org/mozilla-central/rev/51df0002d574
https://hg.mozilla.org/mozilla-central/rev/be715c1edaeb

Comment 82

[Ryan VanderMeulen [:RyanVM]]

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

[Cameron McCormack (:heycam)]

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

[Jean-Yves Perrier [:teoli]]

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

[Sebastian Zartner [:sebo]]

Sure, I'll have a look tomorrow.

Sebastian

Comment 86

[Sebastian Zartner [:sebo]]

(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

[Cameron McCormack (:heycam)]

Thank you Sebastian!

Comment 88

[Cameron McCormack (:heycam)]

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

[Jonathan Watt [:jwatt]]

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

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Release note added to Firefox 41.0a2

Comment 91

[igloczek]

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

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(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

[Sebastian Zartner [:sebo]]

(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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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

[Sebastian Zartner [:sebo]]

Thanks for the info, David! I've adjusted the documentation of the two properties accordingly.

Sebastian

Comment 97

[yem_salat]

This issue is NOT FIXED, please refer to: https://bugzilla.mozilla.org/show_bug.cgi?id=1229699