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

RESOLVED FIXED in Firefox 41

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: roc, Assigned: jwatt)

Tracking

(Depends on: 2 bugs, {dev-doc-complete})

Trunk
mozilla41
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27+ disabled, firefox41 fixed, relnote-firefox 41+)

Details

(Whiteboard: [parity-chrome])

Attachments

(5 attachments, 7 obsolete attachments)

50.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.05 KB, patch
heycam
: review+
Details | Diff | Splinter Review
16.40 KB, patch
heycam
: review+
Details | Diff | Splinter Review
7.63 KB, patch
heycam
: review+
Details | Diff | Splinter Review
922 bytes, patch
jwatt
: review+
Details | Diff | Splinter Review
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.
Created attachment 813185 [details] [diff] [review]
fix
Attachment #813185 - Flags: review?(cam)

Comment 2

4 years ago
Looks like bug 891074 is a duplicate of this.

Comment 3

4 years ago
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 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
Attachment #813185 - Flags: review?(cam) → review+
Can you add a test with a non-percentage (or side keyword) transform-origin value too?
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

4 years ago
(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 :(.
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!
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.
Oddly I can't reproduce either of these failures locally :-(
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?
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

4 years ago
I'm experiencing this problem right now: http://codepen.io/corysimmons/pen/qspwE :(
But there are other failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0ef00ffc6c12
(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.
https://tbpl.mozilla.org/?tree=Try&rev=68cb550dc7cf
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?
Flags: needinfo?(matt.woodrow)
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?
Flags: needinfo?(matt.woodrow)

Comment 19

4 years ago
(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?
(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

4 years ago
(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.
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.
Attachment #813185 - Attachment is obsolete: true
Attachment #817710 - Flags: review?(cam)
Green on try, by the way. https://tbpl.mozilla.org/?tree=Try&rev=6c51c9e587d8
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.
Attachment #817710 - Flags: review?(cam) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a417424f9213
I think this caused a bunch of TSVG ASAP regressions.
If that's so, maybe we can have GetFrameBoundsForTransform only be called when we have percentage values to resolve against it?
https://hg.mozilla.org/mozilla-central/rev/a417424f9213
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: dev-doc-needed
Depends on: 929021
Filed bug 929021.

Updated

4 years ago
Depends on: 929966

Updated

4 years ago
Depends on: 821188
relnote-firefox: --- → ?

Updated

4 years ago
Depends on: 931369
(Assignee)

Comment 30

4 years ago
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

Updated

4 years ago
Depends on: 931422
Depends on: 933354

Updated

4 years ago
relnote-firefox: ? → 27+

Updated

4 years ago
Depends on: 935181
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb151446ec5

We need these backouts on Aurora too.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I will re-fix this when bug 935008 (the perf regression from bug 922942) is fixed.
Depends on: 935008
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
Attachment #829981 - Flags: approval-mozilla-aurora?
(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 ?
Merge of backouts:
https://hg.mozilla.org/mozilla-central/rev/3fb151446ec5
https://hg.mozilla.org/mozilla-central/rev/4a5c39f4cf22

Updated

4 years ago
Depends on: 599732
(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.

Updated

4 years ago
Attachment #829981 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking so we don't forget to land the backouts also on Aurora.
tracking-firefox27: --- → ?

Updated

4 years ago
status-firefox27: --- → affected
tracking-firefox27: ? → +
backed out on Aurora. https://hg.mozilla.org/releases/mozilla-aurora/rev/2e429a8f2949
status-firefox27: affected → ---
tracking-firefox27: + → ---
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.

Updated

4 years ago
Duplicate of this bug: 936136

Updated

4 years ago
status-firefox27: --- → affected
tracking-firefox27: --- → ?

Updated

4 years ago
tracking-firefox27: ? → +
Re-pushed, both changesets this time:
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e5824f3b572
https://hg.mozilla.org/releases/mozilla-aurora/rev/90c307e8c79a
status-firefox27: affected → fixed

Updated

4 years ago
status-firefox27: fixed → disabled
relnote-firefox: 27+ → ?
[comment to Relman] - I have recently removed this from our Relnote DB so it does not appear on release notes that will be generated.
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.
status-firefox28: --- → fixed
tracking-firefox28: --- → +
Flags: needinfo?(roc)
Target Milestone: mozilla27 → mozilla28
This is not fixed in 28.
status-firefox28: fixed → ---
tracking-firefox28: + → ---
relnote-firefox: ? → -
Flags: needinfo?(roc)

Updated

3 years ago
Blocks: 1013421

Updated

3 years ago
Duplicate of this bug: 1013421

Updated

3 years ago
Blocks: 891074

Updated

3 years ago
Duplicate of this bug: 891074
Comment hidden (me-too)
(Assignee)

Comment 48

3 years ago
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.
Depends on: 932762

Updated

3 years ago
Duplicate of this bug: 1057398
Whiteboard: [parity-chrome]
Comment hidden (advocacy)
Comment hidden (advocacy)
Comment hidden (advocacy)

Comment 53

3 years ago
Created attachment 8508425 [details] [diff] [review]
updated patch -> still has issues with redrawing
Attachment #817710 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1102145

Comment 54

3 years ago
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.

Updated

2 years ago
Duplicate of this bug: 1070774

Updated

2 years ago
Depends on: 1135548

Comment 56

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

Comment 57

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

Updated

2 years ago
Assignee: roc → jwatt
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: mozilla28 → ---
(Assignee)

Updated

2 years ago
Depends on: 1159053
(Assignee)

Comment 58

2 years ago
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.
Attachment #8599362 - Flags: review?(roc)
(Assignee)

Comment 59

2 years ago
Created attachment 8599473 [details] [diff] [review]
part 1 - defer calculation of the reference box until it's needed
Attachment #8599362 - Attachment is obsolete: true
Attachment #8599362 - Flags: review?(roc)
Attachment #8599473 - Flags: review?(roc)
(Assignee)

Comment 60

2 years ago
Created attachment 8599485 [details] [diff] [review]
part 1 - defer calculation of the reference box until it's needed
Attachment #8599473 - Attachment is obsolete: true
Attachment #8599473 - Flags: review?(roc)
Attachment #8599485 - Flags: review?(roc)
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.
Attachment #8599485 - Flags: review?(roc) → review+

Comment 62

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/94ef7c315d16
(Assignee)

Updated

2 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/94ef7c315d16

Comment 64

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb6608e6680
(Assignee)

Comment 65

2 years ago
I repushed the patch from bug 933354, but attributed it to this bug since it's an optimization for transform-origin.
https://hg.mozilla.org/mozilla-central/rev/7cb6608e6680
(Assignee)

Comment 67

2 years ago
Created attachment 8602151 [details] [diff] [review]
part 2 - Implement transform-origin for SVG
Attachment #829981 - Attachment is obsolete: true
Attachment #8508425 - Attachment is obsolete: true
Attachment #8602151 - Flags: review?(cam)
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 on attachment 8602151 [details] [diff] [review]
part 2 - Implement transform-origin for SVG

Cancelling review while awaiting answers to the questions above.
Attachment #8602151 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Summary: Support transform-origin in SVG → Support the 'transform-origin' property in SVG and implement the 'transform-box' property
(Assignee)

Comment 70

2 years ago
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.
Attachment #8602151 - Attachment is obsolete: true
Attachment #8607921 - Flags: review?(cam)
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?
Attachment #8607921 - Flags: review?(cam) → review+
(Assignee)

Comment 72

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

Comment 73

2 years ago
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.)
Attachment #8607931 - Flags: review?(cam)
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?
Attachment #8607931 - Flags: review?(cam) → review+
(Assignee)

Comment 75

2 years ago
Created attachment 8607945 [details] [diff] [review]
part 5 - tests for the 'transform-origin' and 'transform-box' properties in SVG
Attachment #8607945 - Flags: review?(cam)
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?
Attachment #8607945 - Flags: review?(cam) → review+

Comment 77

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

Comment 78

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/51df0002d574
https://hg.mozilla.org/integration/mozilla-inbound/rev/be715c1edaeb
(Assignee)

Updated

2 years ago
Attachment #8607921 - Attachment description: part 2 - implement the transform-box property → part 3 - implement the transform-box property
(Assignee)

Updated

2 years ago
Attachment #8607931 - Attachment description: part 3 - implement the transform-origin property in SVG → part 4 - implement the transform-origin property in SVG
(Assignee)

Updated

2 years ago
Attachment #8607945 - Attachment description: part 4 - tests for the 'transform-origin' and 'transform-box' properties in SVG → part 5 - tests for the 'transform-origin' and 'transform-box' properties in SVG
(Assignee)

Comment 79

2 years ago
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
Attachment #8610943 - Flags: review+
(Assignee)

Comment 80

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

Updated

2 years ago
Keywords: leave-open
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
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Attachment #829981 - Flags: approval-mozilla-aurora+
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?
relnote-firefox: - → ?

Updated

2 years ago
Blocks: 1151258

Updated

2 years ago
Depends on: 1171333
SebastianZ, do you think you can find some time to document transform-box (Spec: http://dev.w3.org/csswg/css-transforms/#transform-box )?
Flags: needinfo?(sebastianzartner)
Sure, I'll have a look tomorrow.

Sebastian
Flags: needinfo?(sebastianzartner)
(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.
Keywords: dev-doc-needed → dev-doc-complete
Thank you Sebastian!
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?
Flags: needinfo?(jwatt)
(Assignee)

Comment 89

2 years ago
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.
Flags: needinfo?(jwatt)
Blocks: 1175492
Release note added to Firefox 41.0a2
relnote-firefox: ? → 41+
Blocks: 1208550

Comment 91

2 years ago
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
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.)
Blocks: 1209853
(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.)
(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
Flags: needinfo?(dholbert)
(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
Flags: needinfo?(dholbert)
Thanks for the info, David! I've adjusted the documentation of the two properties accordingly.

Sebastian
Comment hidden (off-topic)
Depends on: 1335876
You need to log in before you can comment on or make changes to this bug.