User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090719 Minefield/3.6a1pre Build Identifier: Extension of the CSS3 Transforms; this looks very interesting. Apple implemented it in its iPhone2 and is in the progress of adding it to Safari ( http://webkit.org/blog/386/3d-transforms/ ) Reproducible: Always

This is now working in the Webkit nightlies, which means that it will "soon" be implemented in Safari and Google Chrome.

Safari has this on Mac but not on Windows. Chrome doesn't have this on any platform.

What is the reason that Webkit has for making it Mac only as of now? Might the same thing limit our implementation?

According to this: https://bugs.webkit.org/show_bug.cgi?id=27314 it is now implemented on Windows.

Very much WIP patch queue at http://hg.mozilla.org/users/mwoodrow_mozilla.com/3d-transforms/

Creates a ContainerLayer for nsDisplayTransform objects so rendering of them can be accelerated when an accelerated backend is enabled. This gives me noticable improvements in rendering http://azarask.in/projects/tabcandy/scale/css-animation.html (within a MakeGoFaster window) - cc'ing Aza. Will push these to try once I figure out how.

Looks like this failed a few tests on try, will debug tomorrow and update.

I think you need to do what I did for opacity: add a new style change hint to nsChangeHint, report that change hint when the transform changes, mark the frame active, etc. See changeset d98f8a21727e and d290d2b97416.

Fixed reftests failures. Fixed roc's suggestions to get layers timing out working correctly.

Everything looks good except + visibleRect.x = 0; + visibleRect.y = 0; + visibleRect.width = INT_MAX; + visibleRect.height = INT_MAX; This isn't really right, the visible rect might need to extend to negative coordinates. If nsLayoutUtils::GfxRectToIntRect fails, it's because the visible rect is nearly infinite ... i.e., the transform has scaled the content down to an incredibly small size. In fact, the content will be shrunk to be basically invisible. Is that right? In that case, why not just set the visible rect to empty? Seems to me that would work in most cases, except for situations where you have one extreme transform with a child element with another extreme transform that cancels out the first one. So another thing we might want to do is to teach FrameLayerBuilder to use an internal scale factor (or possibly horizontal and vertical scale factors). For example, if we have a transform that scales everything up by 2, we could make all the child layers work at twice the resolution, and draw into the ThebesLayers with a scale applied to the context. This would give better quality results too. We would have to use some heuristics so we didn't change the scale factor too often, so we don't have to rerender content too often. We'd still have this problem of extreme visibleRects for video and maybe canvas and other leaf non-Thebes layers, but we can make those invisible using the first approach with no bad consequences. If this makes sense, I suggest we go with the empty-rect approach for now, and file a bug to implement the second approach.

Use an empty visible rect for invalid transforms - Try server says yes.

No API changes to the class, just reworked internals in preparation

This changes larges amounts of layout to directly use gfx3DMatrix wherever possible. This more or less concludes the preparation patches for 3d transforms.

Comment on attachment 461956 [details] [diff] [review] Part 2 v3: Layerify nsDisplayTransform Need dbaron review on style system changes

Comment on attachment 461956 [details] [diff] [review] Part 2 v3: Layerify nsDisplayTransform r=dbaron on the style system changes

Checked in part 2: http://hg.mozilla.org/mozilla-central/rev/9a45bd27ec75

Could this have caused bug 584494?

Yes, I'll look into that today.

Just a little curiosity on my part here - how likely is this to make it into Firefox 4?

I'm hopeful! Slowly crossing things off my list of things to implement and bugs to fix. The end is in sight!

It might be worth starting to request code reviews on the parts that you think are done.

Pushed part 1 to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/d91b0d9fd0a2

Comment on attachment 461958 [details] [diff] [review] Part 4: Upgrade gfx3DMatrix I would strongly suggest that we use an externally-developed and tested matrix library instead of growing our own. If you want something minimalist that fits in one file and has code that's very easy to understand, use CImg: http://cimg.sourceforge.net/ If you want to know what I'd really recommend, it's Eigen (disclaimer, I'm a contributor to it): http://eigen.tuxfamily.org/

This doesn't seem required for the OpenGL reftest bug - probably a mistake?

webkit's implementation also includes a media query: @media (-webkit-transform-3d) This comes in rather handy for feature detection as checking something like 'webkitPerspective' in document.body.style will false positive in recent webkit versions without the graphics-side support. I know with Firefox's multitouch it also exposes a similar -moz-touch-enabled media query. Are you planning to include a mediaQuery for your 3D implementation, not only for feature detection but for better style scoping for authors?

Including a media query sounds like a good idea.

is this dead ? could it reach firefox 4?

Fixed bitrot.

Fixed more bitrot.

More of the same.

+ UntransformRect(mVisibleRect, mFrame, ToReferenceFrame(), &untransformedVisible); What if this fails? We need at least a comment explaining what happens if we can't untransform. + if (disp->mTransform.IsFlat() && Let's call this PreservesAxisAlignedRectangles. + nsRect untransformedVisible; + UntransformRect(mVisibleRect, mFrame, ToReferenceFrame(), &untransformedVisible); Again, probably need a comment explaining what happens for a singular matrix. + if (matrix.IsSingular() || !matrix.Is2D()) + return PR_FALSE; It's probably faster to check Is2D and get the 2D matrix, then check whether the 2D matrix is invertible. Maybe add a method Is2DAndInvertible? + static gfx3DMatrix GetResultingTransformMatrix(const nsIFrame* aFrame, const nsPoint& aOrigin, float aFactor, const nsRect* aBoundsOverride = nsnull); Fix indent. +nsStyleTransformMatrix::IsFlat() const +{ + if (_12 == 0.0f && _13 == 0.0f && _14 == 0.0f && + _21 == 0.0f && _23 == 0.0f && _24 == 0.0f && + _31 == 0.0f && _32 == 0.0f && _33 == 1.0f && + _34 == 0.0f && _43 == 0.0f && _44 == 1.0f) + return PR_TRUE; Should probably just call Is2D and then gfxMatrix::PreservesAxisAlignedRectangles.

Comment on attachment 525558 [details] [diff] [review] Part 4: Upgrade gfx3DMatrix v2 Please use #include <algorithm> instead of nsAlgorithm

Progress update: I now have a large part of the spec completed and working, with the GL layers backend. http://hg.mozilla.org/users/mwoodrow_mozilla.com/3d-transforms-new/ The remaining ToDo list (in approximate order) is: 1) Get the patches cleaned up and ready to land (pref'd off) in a way that won't break unsupported configurations 2) Tests tests tests 3) Add -moz-perspective-origin a z component to -moz-transform-origin 4) D3D9/10 Backend support 5) Software backend support 6) -moz-transform-style 7) DOM interfaces 8) Transitions/Animations

Comment on attachment 525557 [details] [diff] [review] Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v2 So I haven't dug into this in a whole lot of detail yet, but I do definitely know one major thing that's wrong, and I may as well tell you about that sooner rather than later. (It could even be causing bugs that you're observing...) The whole mX/mY business is to handle percentage values of translateX() and translateY(), which are percentages relative to the width and height of the element. At the time we compute the style data, we don't know the element's width and height -- that gets computed during layout. So the basic idea of what we've done for 2D is record three separate matrices: the first is the part that's fixed (mMain and mDelta), the second is to be multiplied by the element's width (mX), and the third to be multiplied by the element's height (mY). In 2-D transforms, the only way to introduce a height-relative transform is in the Y translation component, the only way to introduce a width-relative transform is in the X translation component, and, importantly, the only other matrix components that the operators in 2-D transforms allow the X translation and Y translation components to be copied to is each other (via skew or rotate). But 3-D transforms has more operators, which means that you're pretty obviously not storing enough data. For example, if you have rotateY(45deg) translateX(10%), you're going to rotate some of that translation into data that you simply don't have a place to store, since you don't have a place to store width-relative translation on the Z axis. That reminds me of a second issue, which I'm less sure about what should happen: the issue of units. WebKit's publication of the transforms spec sort of papered over the fact that CSS doesn't have a "primary" unit type for lengths -- lengths can have different units. In the 2-D transform matrix, the components of the transform matrix technically have the following units: [ unitless unitless length ] [ unitless unitless length ] [ 1/length 1/length unitless ] (but these are fixed 0 0 1) In the 3-D transform matrix, the units really look like this: [ unitless unitless length/Z length ] [ unitless unitless length/Z length ] [ Z/length Z/length unitless Z ] [ 1/length 1/length 1/Z unitless ] It's not clear to me that we want to continue storing those two "length" values as nscoord while storing all the rest as floats, especially given the variety of units in this matrix. But we do certainly need to be careful about units and what conversions are expected. And 3-D transforms do introduce the new problem (not present in 2-D transforms) that there's an assumption that CSS pixels are the canonical unit of length and that there's some unnamed canonical unit of Z distance.

(In reply to comment #36) > But 3-D transforms has more operators, which means that you're pretty obviously > not storing enough data. For example, if you have rotateY(45deg) > translateX(10%), you're going to rotate some of that translation into data that > you simply don't have a place to store, since you don't have a place to store > width-relative translation on the Z axis. And, in particular, the fact that 3-D transforms has the arbitrary 16 value matrix3d() function means that you could end up with width-relative and height-relative values in all 16 components of the matrix. (With 2-D transforms, the two cells that were always 0 prevented them from spreading through most of the matrix.)

Thanks David, I should have spotted that. This is pushing the limit of my matrix knowledge, is there any way we can represent all the information we need, similar to what the current code is doing? From what I can see, once the relative data has spread across the matrix there will be no easy way to store everything. We could keep a linked list of matrix objects and only multiply them together once we can evaluate the length/width values, but this would require heap allocations and feels somewhat ugly.

The current approach is still quite doable: you just need 48 components total: 16 for the "constant", 16 for the width-relative, and 16 for the height-relative.

Er, actually, no, it's not, since you can have quadratic terms now too. So I think we'll just need to change to storing the list of transforms in computed style and only computing them into a (16-term) matrix once we know the width. We probably then want to cache the matrix in a frame property.

And, actually, we *already* store the specified transform as nsStyleDisplay::mSpecifiedTransform (since we already need it for animations), so we'd just need to stop storing nsStyleDisplay::mTransform. Then we'd need to compute the transform later on (maybe in nsDisplayTransform? I don't think we should need it at any stage before display lists), and then possibly cache it somewhere (if we do, we'll also need to clear that cache appropriately -- but I'm actually thinking that caching is probably unnecessary). We largely wouldn't need nsStyleTransformMatrix anymore, except we might want to keep it around as a bunch of static methods that produce a gfx3DMatrix from the transform function data. Or something like that -- it depends on what works out well.

We need to know the transform during reflow to calculate the right overflow area in FinishAndStoreOverflow.

Of course, we have the width and height there. So a utility method to compute the final transform would be fine. I agree we don't even need to cache it on the frame, just cache it in the nsDisplayTransform.

Also, since you're redoing much of the code for interpolation of transforms anyway, you should read http://lists.w3.org/Archives/Public/www-style/2010Oct/0440.html (a proposal on how to do it better than what's currently in the spec).

Huge rewrite to keep transforms as a list of transform functions until the bounds information becomes available. I'm not sure if the test for transform changed is suitable and I'm getting a few transform reftest failures (anti-aliasing differences) that I think are caused by this being hit and forcing active (and GPU composited) layers. We also probably want to cache the gfxMatrix inside nsDisplayTransform, how can I tell when the tranform and/or bounds have changed so that we can dump the cached value? This passes all reftests from transforms/ and css-transitions/ (aside from the above mentioned compositing differences), but I still need to check animations tests.

(In reply to comment #45) > We also probably want to cache the gfxMatrix inside nsDisplayTransform, how can > I tell when the tranform and/or bounds have changed so that we can dump the > cached value? Display list items are recreated every time we paint (or hit-test, or whatever) and styles and layout can't change during that time, so there's no problem.

Is this going to land in Aurora for Firefox 5 or Firefox 6? hmmm..

Fixed crashes and quite a few test failures running test_transitions_per_property.html. How do we handle compute distance with non matching transform lists? I'm a bit stuck on how to defer this calculation until the bounds information is available. We still have 17 failures in the test, 7 are rounding issues (greater than round_error_ok allows), the other 10 are distance calculations failing (since the code for them doesn't do anything). This is on top of the above mentioned anti-aliasing failures with reftests.

This now passes all tests on tryserver. I had to disable all tests that attempted to compute a distance with non-matching transform lists (as suggested).

Fixed review comment and moved most implementations into a new gfx3DMatrix.cpp file. Carrying forward r=jrmuizel.

Rebased and hopefully fixed review comments. Note that we should never actually get any 3d transforms with this current code, everything should pass Is2D(). Later patches add 3d transforms, and also implement proper untransforming of these.

Adds the 3d transform functions to nsStyleTransformMatrix. Needs parts 7 and 8 to function correctly, just broken up for easier review.

Add support in layers for 3d transforms. We need to adjust the z scale of the viewport transform to 0 because all values must be in the -1 to 1 range, and we have an infinite far plane.

Let us actually untransform 3d points wherever possible. With the pref (layout.3d-transforms-enabled) off (as it is by default), we pass all tryserver tests with this patch queue. Parts 9 - 12 are still waiting on tryserver results, and possibly more fixes. I plan to land this as soon as possible to try catch any regressions, and prevent them from being bitrotted again.

Comment on attachment 537016 [details] [diff] [review] Part 5: Use gfx3DMatrix in layout v3 Review of attachment 537016 [details] [diff] [review]: -----------------------------------------------------------------

Comment on attachment 537018 [details] [diff] [review] Part 7: Layers support for 3d transforms Review of attachment 537018 [details] [diff] [review]: -----------------------------------------------------------------

Comment on attachment 537019 [details] [diff] [review] Part 8: Add ray tracing to untransform 2d points on a 3d plane Review of attachment 537019 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfx3DPoint.h @@ +101,5 @@ > + *this /= Length(); > + } > +}; > + > +#endif /* GFX_3DPOINT_H */ \ No newline at end of file Does it make sense to make a Base3DPoint class like BasePoint, and instantiate it here for gfx3DPoint? I think it probably does.

Renamed gfx3DPoint to gfx3DVector (I think it makes more sense given the operations we use it for). Added Base3DVector.

All green on tryserver up to this point (with the pref disabled). Took a guess at appropriate reviewers for each piece, happy for anyone to change them.

Comment on attachment 538995 [details] [diff] [review] Part 8: Add ray tracing to untransform 2d points on a 3d plane v2 Review of attachment 538995 [details] [diff] [review]: ----------------------------------------------------------------- Let's break this patch up into the 3DVector stuff, the matrix changes, and the layout changes. For Base3DVector, "3D" should be last in the name for consistency with Azure and because we can't use 3DVector as a class name. Also, per discussion on IRC this should be BasePoint3D. We'll have to review it carefully to keep the API as consistent with BasePoint as makes sense. ::: gfx/src/Base3DVector.h @@ +124,5 @@ > + return sqrt(x*x + y*y + z*z); > + } > + > + void Normalize() { > + *this /= Length(); What if Length() is zero? At least document what the behavior is.

Comment on attachment 538998 [details] [diff] [review] Part 11 - Make -moz-transform-origin also support a z component. Review of attachment 538998 [details] [diff] [review]: ----------------------------------------------------------------- r=me on everything outside of layout/style, needs dbaron review for the style changes ::: layout/style/nsCSSParser.cpp @@ +7524,5 @@ > + if (!ParseBoxPositionValues(position, PR_TRUE)) > + return PR_FALSE; > + > + PRBool allow3D = > + mozilla::Preferences::GetBool("layout.3d-transforms.enabled", PR_FALSE); I think we should cache this somewhere. Use Preferences::AddBoolVarCache.

Maybe you can break out the style system changes in parts 11 and 12 into separate patches from the code that actually uses the new properties?

<joe> fwiw i'm not really in favour of having vector3 and point3 be the same class <joe> what happens when you say matrix4 * vector3? <joe> if you don't have a difference between vector and point types, you don't know whether to use the translation components of the matrix or not

Benoit, what do you say to that?

(In reply to comment #67) > Benoit, what do you say to that? We had a conversation in Real Life (tm) with Joe and we're now in agreement on the set of point/vector classes that we want. Only have one class for points/vectors with 3 coords (x,y,z). Call that e.g. Point3D. Then only have one class for points/vectors with 4 homogeneous coords (x,y,z,w). Call that e.g. Point4D. It doesn't matter to me whether they're called 'point' or 'vector', what matters to me is that we have only one non-homogeneous class and one homogeneous class, and we're now in agreement on that.

(In reply to comment #63) > ::: gfx/src/Base3DVector.h > @@ +124,5 @@ > > + return sqrt(x*x + y*y + z*z); > > + } > > + > > + void Normalize() { > > + *this /= Length(); > > What if Length() is zero? At least document what the behavior is. FWIW I consider that to be purely a documentation issue i.e. I don't think that the code should perform any check. I've found that it's just simpler to design vector/matrix classes in such a way that they behave as similarly as possible to plain floats. Surely the compiler doesn't generate any code to protect against divisions by zero, and neither should we.

(In reply to comment #68) > It doesn't matter to me whether they're called 'point' or 'vector', what > matters to me is that we have only one non-homogeneous class and one > homogeneous class, and we're now in agreement on that. They should be called Point and Vector; optional 3D and 4D. ;)

(In reply to comment #70) > (In reply to comment #68) > > It doesn't matter to me whether they're called 'point' or 'vector', what > > matters to me is that we have only one non-homogeneous class and one > > homogeneous class, and we're now in agreement on that. > > They should be called Point and Vector; optional 3D and 4D. ;) Ah, so we still disagree: I don't agree with having Points with non-homogeneous coordinates vs Vectors with homogeneous coordinates. In homogeneous coordinates there is no notion of 'length of a vector', since if you multiply all homogeneous coords by 2 it's still the same point. So in a proposal where the only vectors would have homogeneous coords, we'd be able to represent only _unit_ vectors. I am not comfortable with such a limitation. === My proposal === Vector3D class has 3 coords (x,y,z). You can also simply call it 'Vector' Vector4D class has 4 homogeneous coords (x,y,z,w). You can also call it 'HVector', where H stands for Homogeneous. No distinction between 'points' and 'vectors'.

Forgot to reply to that: (In reply to comment #66) > <joe> fwiw i'm not really in favour of having vector3 and point3 be the same > class > <joe> what happens when you say matrix4 * vector3? > <joe> if you don't have a difference between vector and point types, you > don't know whether to use the translation components of the matrix or not As I see it, Vector3D would be for ordinary 3D points, not for directions at infinity, so if one had implicit casting to homogeneous coords, (x,y,z) would unambiguously become (x,y,z,1). So there is no ambiguity, once this is agreed on. However, regardless of that, matrix4 * vector3 would have to return a vector4 since the matrix4 can have anything on the fourth row.

(In reply to comment #71) > Vector3D class has 3 coords (x,y,z). You can also simply call it 'Vector' > Vector4D class has 4 homogeneous coords (x,y,z,w). You can also call it > 'HVector', where H stands for Homogeneous. The name should be Point3D, for consistency with 2D points. Point3D and Point4D sound like the best approaches for now.

(In reply to comment #73) > (In reply to comment #71) > > Vector3D class has 3 coords (x,y,z). You can also simply call it 'Vector' > > Vector4D class has 4 homogeneous coords (x,y,z,w). You can also call it > > 'HVector', where H stands for Homogeneous. > > The name should be Point3D, for consistency with 2D points. > > Point3D and Point4D sound like the best approaches for now. I don't know what's the plan with 2D points, but I feel that it's best to pick names that reflect the difference between affine (x,y,z) and homogeneous (x,y,z,w) coordinates. Homogeneous means that we're doing projective geometry i.e. (x,y,z,w) is the same point as (a*x,a*y,a*z,a*w) for any nonzero a. So maybe my suggestion of Vector4D vs Vector3D was not good, as it wrongly suggests that the only difference is the number of coords.

Well then, Point3D vs HPoint3D?

Why not. Or maybe PointH3D. Instead of H for homogeneous, the difference can also be represented by a P for Projective. (note: i've been assuming that we wanted to have a class for points with (x,y,z,w) projective coordinates, I don't know exactly what the needs here are, I'm not implying we should).

HPoint3D, PPoint3D, PointH3D, PointP3D ... all work for me. Someone pick one.

PointH3D it is then.

s/gfx3DVector/gfxPoint3D

Comment on attachment 540957 [details] [diff] [review] Part 11b: Layout changes to use a z component for -moz-transform-origin Carrying forward r=roc

Comment on attachment 540952 [details] [diff] [review] Part 8a: Add BasePoint3D and gfxPoint3D Review of attachment 540952 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/src/BasePoint3D.h @@ +62,5 @@ > + bool operator==(const Sub& aPoint) const { > + return x == aPoint.x && y == aPoint.y && z == aPoint.z; > + } > + bool operator!=(const Sub& aPoint) const { > + return x != aPoint.x || y != aPoint.y || z != aPoint.z; For future reference, I usually find it easier and safer to write !(*this == aOther), but you needn't bother changing it.

Comment on attachment 540953 [details] [diff] [review] Part 8b: Add 3D Point support, and ray tracing to gfx3DMatrix Review of attachment 540953 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfx3DMatrix.h @@ +117,5 @@ > + */ > + gfxPoint3D Transform3D(const gfxPoint3D& point) const; > + > + gfxPoint ProjectPoint(const gfxPoint& aPoint) const; > + gfxRect ProjectRect(const gfxRect& aRect) const; Call this ProjectRectBounds since it takes the bounding rect.

Comment on attachment 540954 [details] [diff] [review] Part 8c: Use ray tracing to untransform 2d points on a 3d plane. Review of attachment 540954 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.h @@ +512,5 @@ > + > + > + static nsRect InvertRectToAncestor(nsIFrame* aFrame, > + const nsRect& aRect, > + nsIFrame* aStopAtAncestor); InvertRectToAncestorBounds? InvertRectBoundsToAncestor? Why not just TransformRectToBoundsInAncestor?

Comment on attachment 540959 [details] [diff] [review] Part 12b: Layout changes to use -moz-perspective-origin Review of attachment 540959 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsDisplayList.cpp @@ +2389,5 @@ > + } > + > + /* Default position of perspective-origin is in the centre of the element */ > + result.x -= NSAppUnitsToFloatPixels(boundingRect.width, aFactor)/2; > + result.y -= NSAppUnitsToFloatPixels(boundingRect.height, aFactor)/2; This seems wrong. The initial value of the property is 50% 50%, and this should be what's in "display" by default, so why do we need to calculate it again? @@ +2394,5 @@ > + > + result.x = -result.x; > + result.y = -result.y; > + > + return result; return -result;

> > + > > + /* Default position of perspective-origin is in the centre of the element */ > > + result.x -= NSAppUnitsToFloatPixels(boundingRect.width, aFactor)/2; > > + result.y -= NSAppUnitsToFloatPixels(boundingRect.height, aFactor)/2; > > This seems wrong. The initial value of the property is 50% 50%, and this > should be what's in "display" by default, so why do we need to calculate it > again? > A value of (0,0) has the effect of applying perspective with the vanishing point in the centre of the object. So we need to adjust the point so that (50%, 50%) returns (0,0). Excluding possible typos, this is what WebKit does and it appears to look correct.

Comment on attachment 540959 [details] [diff] [review] Part 12b: Layout changes to use -moz-perspective-origin Review of attachment 540959 [details] [diff] [review]: -----------------------------------------------------------------

Comment on attachment 537017 [details] [diff] [review] Part 6: Implement the 3d -moz-transform functions So this treats translateZ() the Z component of translate3d() differently from what the spec says: the spec says it's a <length> rather than a <number>. What does WebKit do? ParseSingleTransforms argument should be aIs3D rather than is3d; I spent a while trying to figure out what is3D was used for before realizing it was an out-param. That said, rather than having a separate switch statement inside ParseSingleTransform, I think you should just pass aIs3D into GetFunctionParseInformation and set it there for each case. In ParseMozTransform: * you really really really should cache this preference somewhere rather than getting it every time * your introduction of |prev| doesn't appear to be used for anything; remove it I'm going to wait to look at the nsStyleTransformMatrix stuff in this patch until going through the other one again...

Thanks dbaron, I'll fix those and upload a new patch soon. I'm currently stuck on calcuating overflow areas/visible regions etc for layers with -moz-preserve-3d set. Consider the (relatively) simple case of Parent Frame - preserve3d - rotateY(90deg) - Child Frame 1 - No transform - Child Frame 2 - RotateY(90deg). The current GetBounds implementation calls GetBounds on each child, Union()'s the result and then transforms it into local coordinate space. This returns a 0 sized area since Child Frame 2 (CF2) is essentially singular in 2d space, as is Parent Frame (PF). My solution for finding the bounds of PF, is to instead apply the transformation to each child's GetBounds() result and create a Union of these areas. In the case where the preserve-3d flag is set, and the specific child is also an nsDisplayTransform, we skip the transformation step. When calculating the transformation to use in an nsDisplayTransform, we check for parent frames with preserve-3d and multiply the transforms together. In short CF2::GetBounds() is returning an area in PF's coordinate space instead of it's own. The piece I'm stuck on is what to return for CF2::GetBounds() when not being used as a component of PF::GetBounds(). Can I just always return an area in the parents coordinate system? The 'actual' bounds of CF2 is 0 sized, and results in the object not being drawn. I believe HitTest, ComputeVisibility and FinishAndStoreOverflow are essentially the same problem. For FinishAndStoreOverflow, will storing the area in parents coordinate space cause the frame size for the parent to be changed? We don't want to transform it again. Ideas appreciated :)

preserve-3d always confuses me. Can we pretend the parent frame simply has no transform at all, and propagate its transform down to its children every time we compute the used transform value?

I might be able to do this for some cases. Where does the size value come from for nsIFrame::FinishAndStoreOverflow? The problem is that children that *don't* have a transform need to be transfomed, yet children that do can probably include the transform with theirs and transform themselves. If the area given to FinishAndStoreOverflow is the sum of the child areas, then sections of it would need to be transformed, while others not. I'm not sure how to achieve this.

aNewSize is computed by Reflow(), it's the border-box size which is about to be returned for the frame by Reflow(). It has not yet been set on the frame. It does *not* include child areas. aOverflowAreas contains the visual and scrollable overflow areas for this frame, which includes those areas for the child frames. But if you need to, you can recompute the child frames' contribution by iterating over all child frames and asking for their overflow areas, then transforming them, then you can add that back in to the passed-in aOverflowAreas.

I'm still struggling with the handling of preserve-3d. Slow progress is slow. I had an idea for another possible approach to solving this: Given a frame structure like: parent frame - preserves3d - rotatey(90deg) - child frame - rotatey(90deg) - child frame - not transformed The current code will generate a display item tree that looks like: nsDisplayTransform - rotatey(90deg) - preserve3d - nsDisplayTransform - rotatey(90deg) - nsDisplayText - whatever this content was Is it instead possible to generate: nsDisplayList - nsDisplayTransform - rotatey(180deg) - nsDisplayTransform - rotatey(90deg) - nsDisplayText - contentcontentcontent This looks like it will show the correct rendering for this limited example (not sure if nsDisplayList is the correct 'container') and won't require any changes in visibility calculation, hit testing etc. Are there any problems with this approach that I'm missing?

That sounds good. So if you have parent frame - preserves3d - rotatey(90deg) - child frame - opacity:0.5 - child frame - not transformed - child frame - rotatey(90deg) - child frame - not transformed You'd build nsDisplayList - nsDisplayOpacity - opacity:0.5 - nsDisplayTransform - rotatey(90deg) - nsDisplayText - contentcontentcontent - nsDisplayTransform - rotatey(180deg) - nsDisplayTransform - rotatey(90deg) - nsDisplayText - contentcontentcontent ?

I think that approach should work, and I think it shouldn't be too hard to implement. In BuildDisplayListForStackingContext, when the frame has preserves-3d, you can walk the child display list and do whatever surgery you want to do.

I guess you would walk the display items in the child list: -- for any nsDisplayTransforms, just fold in the extra transform from the parent -- for any nsDisplayOpacity or nsDisplayWrapList, recursively walk over the children (since opacity commutes with transforms) -- for any nsDisplayClip ... I dunno! Introduce a new nsDisplayClipPath which generalizes nsDisplayClipRoundedRect and can clip to an arbitrary path, and transform the rectangle to a quad, replace the nsDisplayClip with an nsDisplayClipPath for that quad, and recursively walk over the children? FrameLayerBuilder would have to know about nsDisplayClipPath. This could probably be done as a followup bug, if we do it at all, since preserving 3D through clipping is reasonably esoteric -- for any other display item, wrap it in an nsDisplayTransform with the parent's transform

I have good news and bad news. This approach seems to work fairly well, I've got it working and rendering the safari 'poster circle' demo correctly. The problem (found by looking at some of the other safari demos) is that animations/transitions of preserve-3d needs to take the child transformation into account. Example: Parent - -moz-transition: -moz-transform 2s; - preserve3d - transformed Child - transformed. If we change the transformation on the child, it should transition over 2 seconds. I'm not sure how to implement this without moving the preserve-3d handling further down again, onto the actual frames. Will think about this further.

(In reply to comment #103) > The problem (found by looking at some of the other safari demos) is that > animations/transitions of preserve-3d needs to take the child transformation > into account. > > Example: > > Parent - -moz-transition: -moz-transform 2s; - preserve3d - transformed > Child - transformed. > > If we change the transformation on the child, it should transition over 2 > seconds. Surely this is a Webkit bug? That behavior makes no sense to me.

So what's the plan here with the preference and how we enable this? Patch 6 introduces a preference for 3D transforms, defaulting to off. Is there going to be a situation where, on some machines, we have the ability to use graphics capabilities that make doing 3D transforms reasonable, and on others we don't? When we enable 3D transforms, do you expect we'll be able to enable it reasonably across all machines, or will we need to enable it only on some of them?

It depends on the use-case. The software implementation is presumably a lot slower, but it might be OK for some kinds of usage.

The rapid release guidlines (as I understand them) encourage the use of the preferences to disable new features if needed. I am working on a software rendering implementation, performance of this is yet to be tested. I want to land code with the preference disabled so we can get some user testing, with lower risk of regressions. Do you have any ETA on being able to review the first few of these patches? Parts 3-5 seem the most likely to cause regressions (and can't be trivially turned off/backed out), so I'd like to get these baking on m-c as soon as possible (and before they bitrot).

Comment on attachment 537013 [details] [diff] [review] Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v5 Why does gfx3DMatrix use |float| when gfxMatrix uses |double|? It seems like these ought to be consistent with each other, at least. I suspect this is responsible for the various rounding errors this patch introduces (especially since the code in nsComputedDOMStyle is using gfxMatrix, and is therefore using nsString::AppendFloat(double) rather than nsString::AppendFloat(float). >+ PRBool IsIdentity() const { >+ return xx == 1.0f && yx == 0.0f && >+ xy == 0.0f && yy == 1.0f && >+ x0 == 0.0f && y0 == 0.0f; >+ } Drop the 'f' suffixes, since xx, yx, etc., are double rather than float. >- (newOrigin + toMozOrigin, disp->mTransform.GetThebesMatrix(bounds, aFactor)); >+ (newOrigin + toMozOrigin, nsStyleTransformMatrix::ReadTransforms(disp->mSpecifiedTransform, >+ aFrame->GetStyleContext(), >+ aFrame->PresContext(), >+ dummy, bounds, aFactor)); Find a way to wrap before the 80th column. >+ const gfxMatrix& newTransformMatrix = GetTransform(mFrame->PresContext()->AppUnitsPerDevPixel()); Break after the = to fit within 80 columns. It would be really nice if nsDisplayTransform::BuildLayer didn't need to use AppUnitsPerDevPixel() (unlike all the other callers) and thus require the mCachedFactor business. Might be nice to clean that up in a followup. nsCSSValueList::CloneExisting() is a confusing name for what you've done. Instad, call it CloneInto, since it's the *argument* that receives the clone. Also add a comment to nsCSSValue.h saying what it does. Finally, reuse Clone() so that the contents of the method are just: aList->mValue = mValue; aList->mNext = mNext ? mNext->Clone() : nsnull; which also fixes the bug in your implementation that it doesn't overwrite a non-null next pointer with a null one. Finally, make it assert at the start that aList->mNext is null (otherwise it would leak). nsComputedDOMStyle.cpp: As I said above, I think it's bogus to serialize these numbers as doubles when they're stored as float accuracy. Patch 5 doesn't fix this, either. Furthermore, something in this patch series needs to make the 3-D parts of the matrix show up in computed style by serializing to matrix3d rather than just matrix, at least in 3D cases. I'd have expected that in patch 5 or 6 (probably 6?), but it doesn't seem to be there. >+ /* Now, we need to convert the matrix into a string. Not useful; remove the comment. nsStyleAnimation.cpp: Rather than inserting "squareDistance += 42", I propose that instead you replace the entire eUnit_Transform case within nsStyleAnimation::ComputeDistance with "return PR_FALSE;", and simultaneously remove the check_distance call in test_transform_transition() in layout/style/test/test_transitions_per_property.html . Please document AddTransformMatrix as taking gfxMatrix parameters in CSS pixels (in .h and .cpp), since it produces eCSSUnit_Pixel output based on the translation coordinates in the gfx matrix. I'm still trying to figure out if nsStyleTransformMatrix::ProcessInterpolateMatrix has a scaling error here (when dev pixels != css pixels). ... WRITE MORE HERE ... In AddDifferentTransformLists: >+ nsCSSValueList* list = arr->Item(2).SetListValue(); >+ aList1->CloneExisting(list); I think this is clearer on one line (especially if CloneExisting is renamed to CloneInto): aList1->CloneInto(arr->Item(2).SetListValue()); >+ ///XXX: If the matrix contains only numbers then we could decompose here. >+ // If it contains % values, I think we can too, less sure about >+ // calc values though. A bunch of things here: - first line goes past column 80 - use "// FIXME:" rather than "///XXX:". - % and calc() are pretty much equivalent; it's the same for both. - However, I'd add that dealing with % and calc() is not possible with matrix3d(), and having consistency between matrix() and matrix3d() is valuable > while ((*resultTail)->mNext) { >- resultTail = &(*resultTail)->mNext; >+ resultTail = &(*resultTail)->mNext; > } Leave this as it was. nsStyleAnimation.h: Please forward-declare gfxMatrix rather than including the header. nsStyleStruct.cpp: - if (mTransform != aOther.mTransform) + if (mSpecifiedTransform != aOther.mSpecifiedTransform) This should be !mSpecifiedTransform != !aOther.mSpecifiedTransform || (mSpecifiedTransform && *mSpecifiedTransform != *aOther.mSpecifiedTransform) since you want deep comparison of the lists rather than pointers. nsStyleTransformMatrix.cpp: Could you rename the aFactor param to pretty much everything to be called aAppUnitsPerMatrixUnit or something like that? Otherwise I think it's not clear that it can be used to create a matrix in either device or CSS pixels. Furthermore, you need to fix the comments above SetToTransformFunction in the .h file -- that currently says it requires device pixels, but most of the time you actually give it CSS pixels. Really, it picks which units the matrix is in terms of. (That said, as I said above, I think it would be good to clean up that inconsistency.) Please wrap the last line of ProcessTranslatePart. In nsStyleTransformMatrix::ProcessMatrix could you call the matrix result rather than temp? >+ nsAutoPtr<nsCSSValueList> result; >+ nsCSSValueList **resultTail = getter_Transfers(result); >+ >+ *resultTail = nsStyleAnimation::AddTransformMatrix(matrix1, coeff1, >+ matrix2, coeff2); Instead, write: nsAutoPtr<nsCSSValueList> result(nsStyleAnimation::AddTransformMatrix( matrix1, coeff1, matrix2, coeff2)); That said, this is rather silly -- AddTransformMatrix now painstakingly builds up a list from its decomposed data only to have its only caller immediately decompose that list. AddTransformMatrix should probably just return another matrix. Maybe you do that in a later patch...? If not, could you do it as another patch on top of this queue, if not just in this one? I hope all the gfx3DMatrix return values from nsStyleTransformMatrix::Process* don't lead to too much struct copying. Have you checked? >- ProcessSkewHelper(xSkew, ySkew, aMain); >+ return ProcessSkewHelper(xSkew, ySkew);; No double ;, please. Could you rename SetToTransformFunction to MatrixForTransformFunction? r=dbaron with all of that fixed

Also, even after patch 5, I think there are still some pieces that are awkwardly 2-D only (like animation). That'll need to be fixed before this is enabled, but it's ok for now.

(In reply to comment #108) > Why does gfx3DMatrix use |float| when gfxMatrix uses |double|? It seems > like these ought to be consistent with each other, at least. I suspect > this is responsible for the various rounding errors this patch > introduces (especially since the code in nsComputedDOMStyle is using > gfxMatrix, and is therefore using nsString::AppendFloat(double) rather > than nsString::AppendFloat(float). gfxMatrix is double because that's what cairo uses. I think gfx3DMatrix uses floats because they're far better supported on GPUs than doubles.

Yep, we use floats matrices for both OpenGL and D3D*. I think the loss of precision is probably better than creating a gfxDouble3DMatrix and converting back to floats at draw time, but I'm open to suggestions. Animations are definitely awkward with this patch, but I have a followup patch in my queue that implements these properly. Was the " ... WRITE MORE HERE ..." comment meant to be removed or is still an intentional placeholder? Thanks for the review dbaron!

(In reply to comment #111) > Was the " ... WRITE MORE HERE ..." comment meant to be removed or is still > an intentional placeholder? Er, yeah, one of us should figure out if there's actually a bug there, though it sort of looks like it to me.

Comment on attachment 537017 [details] [diff] [review] Part 6: Implement the 3d -moz-transform functions ...continued from comment 94. We should add support for number values on translatex(), translatey(), translate(), translate3d(), and the last 2 parts of matrix() -- and treat them as pixels. You should do that as part of this bug -- otherwise this patch leaves things in an oddly inconsistent state. >+ /* There are several cases to consider. >+ * Next, the values might be lengths, or they might be percents. If they're >+ * percents, store them in the dX and dY components. Otherwise, store them in >+ * the main matrix. >+ */ You copied this from somewhere, but it no longer belongs. If the original is still there, delete that too. >+ temp._11 = 1 + (1 - cosTheta) * (x * x - 1); >+ temp._12 = -z * sinTheta + (1 - cosTheta) * x * y; >+ temp._13 = y * sinTheta + (1 - cosTheta) * x * z; >+ temp._14 = 0.0f; >+ temp._21 = z * sinTheta + (1 - cosTheta) * x * y; >+ temp._22 = 1 + (1 - cosTheta) * (y * y - 1); >+ temp._23 = -x * sinTheta + (1 - cosTheta) * y * z; >+ temp._24 = 0.0f; >+ temp._31 = -y * sinTheta + (1 - cosTheta) * x * z; >+ temp._32 = x * sinTheta + (1 - cosTheta) * y * z; >+ temp._33 = 1 + (1 - cosTheta) * (z * z - 1); >+ temp._34 = 0.0f; >+ temp._41 = 0.0f; >+ temp._42 = 0.0f; >+ temp._43 = 0.0f; >+ temp._44 = 0.0f; According to the spec _44 should be 1.0f, not 0.0f. >+ case eCSSKeyword_scale3d: >+ return ProcessScale3D(aData); > case eCSSKeyword_scale: > return ProcessScale(aData); Could you stick scale3d() after scale() for consistency with the rest? r=dbaron with that fixed, though I probably want to look at the pref caching code once you address that

(Note that the number values thing is probably easiest as a separate patch -- if you merge it with an existing one I'd want to review it.)

Comment on attachment 538996 [details] [diff] [review] Part 9 - Implement the perspective() transform function and style property. >Bug 505115 - Part 9 - Implement the perspective() transform function and style property. try: -b d -p linux -u mochitest-4 -t none You don't want try syntax in your commit message. I'd also write it as: Implement the perspective() transform function and the perspective CSS property. As per patch 6, I think you should cache the pref (which also has value in encapsulating the "are 3d transforms enabled" decision to one place, which could be valuable if it's more than just a pref check in future). >+CSS_PROP_DISPLAY( >+ -moz-perspective, >+ _moz_perspective, >+ CSS_PROP_DOMPROP_PREFIXED(Perspective), >+ CSS_PROPERTY_PARSE_VALUE, >+ VARIANT_NONE|VARIANT_INHERIT|VARIANT_NUMBER, >+ kDisplayKTable, >+ offsetof(nsStyleDisplay, mChildPerspective), >+ eStyleAnimType_float) _moz_perspective -> perspective (don't prefix internals) kDisplayKTable -> nsnull put spaces around the |s The mPerspective and mChildPerspective stuff is really broken -- it's going to produce incorrect results in a lot of cases due to your code not disabling sharing in cases where you'd need to disable sharing. Rather than explain the whole thing, I'm just going to say you should rip it out. Make the style struct contain the computed value of the property for that element (what's now mChildPerspective). When you want the parent's perspective, look at parentFrame->GetStyleContext->GetParent()->GetStyleDisplay()->mPerspective (except you'll need to null-check the GetParent() call). I didn't really look too closely at rest of the code (though the whole thing with the perspective transform function seems pretty weird to me -- I haven't dug into that), since this is pretty clearly wrong and needs substantial reworking because of the mPerspective/mChildPerspective thing being broken.

(In reply to comment #115) > When you want > the parent's perspective, look at > parentFrame->GetStyleContext->GetParent()->GetStyleDisplay()->mPerspective > (except you'll need to null-check the GetParent() call). er, I meant frame->GetStyleContext()->GetParent() (not parentFrame -- otherwise you'd be walking up twice).

Comment on attachment 540955 [details] [diff] [review] Part 10 - Implement -moz-backface-visible >Bug 505115 - Part 9 - Implement the perspective() transform function and style property. How about calling it Part 10, and using an appropriate commit message for part 10. >+CSS_PROP_DISPLAY( >+ -moz-backface-visibility, >+ _moz_backface_visibility, >+ CSS_PROP_DOMPROP_PREFIXED(BackfaceVisibility), >+ CSS_PROPERTY_PARSE_VALUE, >+ VARIANT_HK, >+ kBackfaceVisibilityKTable, >+ offsetof(nsStyleDisplay, mBackfaceVisible), >+ eStyleAnimType_EnumU8) _moz_backface_visibility -> backface_visibility (don't prefix internals) Please call the member of nsStyleDisplay mBackfaceVisibility to match the property name rather than calling it mBackfaceVisible. >+#define NS_STYLE_BACKFACE_VISIBLE 1 >+#define NS_STYLE_BACKFACE_HIDDEN 0 The convention here suggests you should insert VISIBILITY_ into these names (since they're property-value). >+ COMPUTED_STYLE_MAP_ENTRY_LAYOUT(_moz_backface_visibility, MozBackfaceVisibility), This isn't layout-dependent, drop the "_LAYOUT". I'm puzzled by the nsStyleAnimation changes -- I don't see anything in the transitions or 3d transform specs saying the property is transitionable. If it is, though, I'd have expected it to work like 'visibility'... which seems to be what you test for, despite doing the code entirely differently. What does WebKit do? If the goal is to work like visibility, why not do it the same way in nsStyleAnimation? In nsStyleDisplay::CalcDifference, why is reflow required? I'd have expected repaint and maybe layer stuff. In property_database.js, please test that "collapse" is not an accepted value (put it in invalid_values). (That's the value for 'visibility' that's not valid here.) >+ // Convert to two vectors on the surface of the plane. >+ gfxPoint3D ab = a - b; >+ gfxPoint3D ac = a - c; >+ >+ return ab.CrossProduct(ac); I find this a little confusing. ab is the *negative* of the transformation of the unit Y axis vector, and ac is the *negative* of the transformation of the unit X axis vector. It seems clearer to use b-a and c-a. Also, it seems really weird that your definition of "normal vector" is something that, with no transformation, points along the negative Z axis. Is this some standard terminology, or would it make more sense to take ac.CrossProduct(ab)? I presume you've tested that this matches WebKit in the case of scaleZ(-1). The spec seems totally unclear, and it ought to be clarified (i.e., that it's the results of transforming the X and Y axes that matter, rather than the result of transforming the Z axis). One of us should email www-style about that. >+ gfxPoint3D normal = newTransformMatrix.GetNormalVector(); >+ if (newTransformMatrix.IsSingular() || >+ (mFrame->GetStyleDisplay()->mBackfaceVisible == NS_STYLE_BACKFACE_HIDDEN && >+ normal.DotProduct(gfxPoint3D(0,0,1)) >= 0.0)) { >+ return nsnull; >+ } Seems better to skip the variable |normal| and just put the whole expression inside the if(). Also, don't use two spaces before the ==. Also, just check if normal.z >= 0 and skip the DotProduct(). And if you change GetNormalVector() to work the way that makes sense to me, make it <= 0 here :-) And please add a comment in gfx3DMatrix.h saying what GetNormalVector does. Marking as review- because I'd like to look at this again for the animation issues; otherwise I'd be fine with you making the changes without my looking again.

Comment on attachment 540956 [details] [diff] [review] Part 11a: Add nsCSSValueTriplet and optionally read a z component to -moz-transform-origin >+ if (CheckEndProperty() || !ParseVariant(depth, VARIANT_AHL, nsnull) || !allow3D) { VARIANT_AHL is wrong; as far as I can tell it should just be VARIANT_LENGTH | VARIANT_CALC. Additionally, you should explicitly not look for a third value if the first value has unit inherit or initial. Also, wrap before the 80th column. You need (again) to cache the preference. I'd probably have preferred use of nsCSSValue::Array over creating nsCSSValueTriplet, but I suppose I can live with this approach. In nsCSSValue.h, could you make Triplet 51, and bump the current 51-56, to be consistent with the order you use in the code? You need to add a case to nsCSSValue::operator==, which you missed. I'm concerned that you serialize (in the specified value serialization; the computed value serialization is fine, and also has fewer constraints about needing to be faithful to what was specified) a transform-origin without a third value by adding that value, since we've generally tried to serialize to the most supported (i.e., oldest) syntax. One way to do that is to use the eCSSUnit_Null on the Z value to distinguish these cases; I think that would require adjusting a few assertions (nsCSSValue::SetTripletValue's assertion about null)... though assertions already inconsistent with what's handled in nsCSSValueTriplet::AppendToString. (The animation code can still maintain the invariant that the third value is non-null... though when the resulting value is zero it should *set* is as null.) There are probably other options for handling this as well, but that's the way that seems obvious to me. In nsCSSValue.h: >+ nsCSSValueTriplet(const nsCSSValue& aXValue, const nsCSSValue& aYValue, const nsCSSValue& aZValue) >+ : mXValue(aXValue), mYValue(aYValue), mZValue(aZValue) Please wrap before column 80. In nsRuleNode.cpp, don't give SetTripletCoords a general name; it's specific to transform origin since it has special treatment of the Z component. I tend to think it's probably best to inline it into the caller. nsStyleAnimation.cpp: I'd like to look at this more closely after you address the serialization stuff with the third value. A little more sharing of code that you're copying from the pair case would be nice, though. nsStyleStruct.h: >- nsStyleCoord mTransformOrigin[2]; // [reset] percent, coord, calc >+ nsStyleCoord mTransformOrigin[3]; // [reset] percent, coord, calc comment should explain that the third value can't be a percent I'd like to look at this again once you address the third-value serialization issue, so marking review-.

Comment on attachment 540958 [details] [diff] [review] Part 12a: Implement -moz-perspective-origin style property. >diff --git a/dom/interfaces/css/nsIDOMCSS2Properties.idl b/dom/interfaces/css/nsIDOMCSS2Properties.idl You need to bump the IID here. (And I forgot to mention that for 2 other patches, but it applies there too!) >+PRBool CSSParserImpl::ParseMozPerspectiveOrigin() The indentation of this function is weird. Also, I think you should share ParseMozTransformOrigin and add a property argument (and use it for whether to do the third value), rather than have two separate almost-identical functions. > CSS_PROP_DISPLAY( >+ -moz-perspective-origin, >+ _moz_perspective_origin, No _moz_ here, again. >+ CSS_PROP_DOMPROP_PREFIXED(PerspectiveOrigin), >+ CSS_PROPERTY_PARSE_FUNCTION | >+ CSS_PROPERTY_STORES_CALC, Please indent the line after the | following local style. >+ COMPUTED_STYLE_MAP_ENTRY_LAYOUT(_moz_perspective_origin, MozPerspectiveOrigin), The use of _LAYOUT here is correct (unlike the last time, when I pointed out it was wrong). Just pointing out that you shouldn't change this one too. nsRuleNode.cpp: Please match the local 2-space indent. property_database.js: Indentation here is wacky. r=dbaron with that fixed

(In reply to comment #108) > > nsComputedDOMStyle.cpp: > > As I said above, I think it's bogus to serialize these numbers as > doubles when they're stored as float accuracy. > > Patch 5 doesn't fix this, either. I've added this to patch 5 now. > > Furthermore, something in this patch series needs to make the 3-D parts > of the matrix show up in computed style by serializing to matrix3d > rather than just matrix, at least in 3D cases. I'd have expected that > in patch 5 or 6 (probably 6?), but it doesn't seem to be there. This is currently in a much later part of the patch queue, I can split this out and bring it forward if needed. > > >+ /* Now, we need to convert the matrix into a string. > > Not useful; remove the comment. > > nsStyleAnimation.cpp: > > Rather than inserting "squareDistance += 42", I propose that instead you > replace the entire eUnit_Transform case within > nsStyleAnimation::ComputeDistance with "return PR_FALSE;", and > simultaneously remove the check_distance call in > test_transform_transition() in > layout/style/test/test_transitions_per_property.html . Alright, done! > > That said, this is rather silly -- AddTransformMatrix now painstakingly > builds up a list from its decomposed data only to have its only caller > immediately decompose that list. AddTransformMatrix should probably > just return another matrix. Maybe you do that in a later patch...? If > not, could you do it as another patch on top of this queue, if not just > in this one? Good point, made this this change too. > > I hope all the gfx3DMatrix return values from > nsStyleTransformMatrix::Process* don't lead to too much struct copying. > Have you checked? Looking at the assembly for my debug mac build, it appears that it is constructing the temporary gfx3DMatrix objects directly into a pointer passed by the caller, and not copying the result at all.

Fixed review comments, and rebased against tip.

Fixed serialization of gfx3DMatrix objects to stay as floats. Rebased against tip/Part 3 changes. Carrying forward r=roc

Landed parts 3-5 on inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/7e16ec834b15 http://hg.mozilla.org/integration/mozilla-inbound/rev/92bd75756f43 http://hg.mozilla.org/integration/mozilla-inbound/rev/89f90f9fac80

Fixed review comments, including caching the preference. Carrying forward r=dbaron

Rebased against tip/previous changes. Carrying forward r=roc

Fixed review comments and changed handling of perspective() function to match the spec properly.

Fixed review comments, removed animations code entirely.

Fixed review comments

Fixed review comments, carrying forward r=dbaron

Initial set of basic tests for 3d transforms, that really don't test enough yet. Still thinking of more ideas to test perspective etc.

The preserve-3d patches raises a few interesting question, which also probably need to go through www-style. The depth sorting in the spec is very vague, although this is intentional I think it needs to be clarified. Currently I've implemented sorting based on the z translate component of the matrix (same as Chromium does/did). One interesting case is elements that contain the preserve-3d flag, but no actual transform. Webkit is treating these as still 'transformed' and is including any parent perspective into child transforms. This could also be considered as having children of a preserve-3d frame being brought up into the frame space (if that makes sense). The other problem case is where there is multiple levels of preserve-3d (such as the WebKit poster circle demo). This has 12 elements transformed into a ring, 3 rings transformed into a cylinder, and the whole cylinder rotates. My depth sorting code arranges elements within a single ring correctly, and each ring correctly, but elements can still weirdly overlap between rings. WebKit appears to be sorting all 36 elements by depth, which seems to go against the fact that a transform creates a stacking context/containing block I'm not sure what the correct behaviour should be, but I this needs to be clarified in the spec.

part 3 http://hg.mozilla.org/mozilla-central/rev/7e16ec834b15 part 4 http://hg.mozilla.org/mozilla-central/rev/92bd75756f43 part 5 http://hg.mozilla.org/mozilla-central/rev/89f90f9fac80

Comment on attachment 540953 [details] [diff] [review] Part 8b: Add 3D Point support, and ray tracing to gfx3DMatrix >+gfxRect gfx3DMatrix::ProjectRect(const gfxRect& aRect) const It would be nice to add a comment explaining what is the rect returned by this function, since it's not just returning the transformed rect (that wouldn't be a gfxRect). If I understand the code correctly, it returns the smallest gfxRect containting the image of aRect under the transformation.

Fixed a few bugs

Initial testing for 3d transitions/animations. I really need to add more tests here, working on it.

Comment on attachment 548076 [details] [diff] [review] Part 9 - Implement the perspective() transform function and style property. v2 >diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp >+#include "mozilla/Preferences.h" No need for this include anymore. >+ parentDisp->mChildPerspective != 0.0) { Per spec, you should test > 0.0, since negative values should also lead to no perspective. >+ const nsStyleDisplay* parentDisp = aFrame->GetParent()->GetStyleDisplay(); You should null-check the result of aFrame->GetParent() before calling GetStyleDisplay() on it. >diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp >+nsIDOMCSSValue* >+nsComputedDOMStyle::DoGetMozPerspective() >+{ >+ nsROCSSPrimitiveValue* val = GetROCSSPrimitiveValue(); >+ val->SetNumber(GetStyleDisplay()->mChildPerspective); >+ return val; Given that 'none' is the initial value, I think you should probably report '0' as 'none'. >+ COMPUTED_STYLE_MAP_ENTRY_LAYOUT(perspective, MozPerspective), Remove the "_LAYOUT". >diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp >--- a/layout/style/nsRuleNode.cpp >+++ b/layout/style/nsRuleNode.cpp >@@ -4479,16 +4479,22 @@ nsRuleNode::ComputeDisplayData(void* aSt > parentDisplay->mTransformOrigin[0], > parentDisplay->mTransformOrigin[1], > SETCOORD_LPH | SETCOORD_INITIAL_HALF | > SETCOORD_BOX_POSITION | SETCOORD_STORE_CALC, > aContext, mPresContext, canStoreInRuleTree); > NS_ASSERTION(result, "Malformed -moz-transform-origin parse!"); > } > >+ SetFactor(*aRuleData->ValueForPerspective(), >+ display->mChildPerspective, canStoreInRuleTree, >+ parentDisplay->mChildPerspective, 0.0f, >+ SETFCT_NONE); >+ >+ No need for the double linebreak here. >diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp >+ if (!(mChildPerspective == aOther.mChildPerspective)) Just use != rather than ! ( == ). >diff --git a/layout/style/nsStyleTransformMatrix.cpp b/layout/style/nsStyleTransformMatrix.cpp >+ if (depth > 0.0f) { >+ temp._34 = -1.0/depth; >+ } The spec says non-positive values should be rejected by the parser, so by this point you should be able to assert that depth > 0.0f -- though I'd hope you haven't done any double-to-float conversions since it was parsed! That said -- you need to add code to the parser to reject nonpositive values, since you're missing that code. >diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js >+ "-moz-perspective": { >+ domProp: "MozPerspective", >+ inherited: false, >+ type: CSS_TYPE_LONGHAND, >+ initial_values: [ "0" ], you should add "none" here >+ other_values: [ "1000", "500.2" ], and add some negative values here >+ invalid_values: [ "pants" ] >+ }, I'd also note that the spec says the perspective() function and the 'perspective' property take <length> rather than <number>. Which does WebKit do? Does the spec need to be fixed, or should you be changing this to accept lengths rather than numbers? Number does seem to make more sense to me. If the spec needs fixing, please email www-style. r=dbaron with that fixed

(In reply to comment #140) Thanks for the comments, fixed and running through tryserver now. > > I'd also note that the spec says the perspective() function and the > 'perspective' property take <length> rather than <number>. Which does > WebKit do? Does the spec need to be fixed, or should you be changing this > to accept lengths rather than numbers? Number does seem to make more sense > to me. If the spec needs fixing, please email www-style. Where does it say this? The document I've been referring to says <number>. http://www.w3.org/TR/css3-3d-transforms/#perspective

http://dev.w3.org/csswg/css3-3d-transforms/ is newer than that

Oh right, guess I've been reading the wrong thing :) Thanks for that, I'll switch to parsing lengths then! WebKit appears to be parsing lengths too.

Fixed review comments. Requesting review again because of the number -> length changes.

Fixed test failures.

Comment on attachment 548080 [details] [diff] [review] Part 13: Add basic reftests for 3d transforms and expose 3d transform status in GfxInfo Review of attachment 548080 [details] [diff] [review]: ----------------------------------------------------------------- Instead of exposing whether 3D transforms are enabled, how about just making the tests conditional on GPU acceleration enabled (for now)? ::: layout/reftests/transform-3d/reftest.list @@ +23,5 @@ > +== rotate3d-2a.html rotatey-1-ref.html > +!= backface-visibility-1a.html about:blank > +fails-if(!transforms3d) == backface-visibility-1b.html about:blank > +fails-if(!transforms3d) != perspective-origin-1a.html rotatex-perspective-1a.html > +== perspective-origin-1b.html perspective-origin-1a.html Can you put these in alphabetical order?

Comment on attachment 548084 [details] [diff] [review] Part 14b: Layout changes for preserve-3d Review of attachment 548084 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsDisplayList.cpp @@ +2441,4 @@ > { > NS_PRECONDITION(aFrame, "Cannot get transform matrix for a null frame!"); > + //NS_PRECONDITION(aFrame->GetStyleDisplay()->HasTransform(), > + // "Cannot get transform matrix if frame isn't transformed!"); Just remove this @@ +2446,5 @@ > + if (aOutAncestor) { > + *aOutAncestor = aFrame->GetParent(); > + } > + > + if (!aFrame->GetStyleDisplay()->HasTransform()) { Add comment that this is the preserve-3d case @@ +2476,5 @@ > + aFrame->PresContext(), > + dummy, bounds, aFactor); > + } else { > + NS_ASSERTION(aFrame->Preserves3DChildren(), > + "If we don't have a transform, then we must be at least preserving transforms of our children"); Shouldn't this be asserting aFrame->Preserves3D? @@ +2776,5 @@ > const nsRect* aBoundsOverride) > { > NS_PRECONDITION(aFrame, "Can't take the transform based on a null frame!"); > + //NS_PRECONDITION(aFrame->GetStyleDisplay()->HasTransform(), > + // "Cannot transform a rectangle if there's no transformation!"); Just remove this, and the ones below ::: layout/generic/nsFrame.cpp @@ +1431,5 @@ > + nsresult rv = NS_OK; > + nsDisplayList newList; > + while (nsDisplayItem *item = aList->RemoveBottom()) { > + if (item->GetType() == nsDisplayItem::TYPE_TRANSFORM && item->GetUnderlyingFrame()->GetParent()->Preserves3DChildren()) { > + NS_ASSERTION(item->GetUnderlyingFrame()->Preserves3D(), ""); Need a message! But can't this assertion fire if you have a preserves-3D parent with a child that has a transform but also a clip? @@ +1481,5 @@ > */ > if ((mState & NS_FRAME_MAY_BE_TRANSFORMED) && > disp->HasTransform()) { > /* If we have a complex transform, just grab the entire overflow rect instead. */ > + if (Preserves3DChildren() || GetParent()->Preserves3DChildren() || !nsDisplayTransform::UntransformRect(dirtyRect, this, nsPoint(0, 0), &dirtyRect)) { Just call Preserves3D() here? @@ +1560,5 @@ > resultList.AppendToTop(set.BlockBorderBackgrounds()); > // 5: floats > resultList.AppendToTop(set.Floats()); > // 7: general content > + set.Content()->SortByZPosition(aBuilder, GetContent()); Hmm, this is a little bit scary. Maybe make this if (Preserves3DChildren())? And maybe instead of calling SortByZPosition and having SortByZPosition account for 3D, use a separate SortByZPosition3D path which you use here? @@ +4534,5 @@ > * See bug #452496 for more details. > */ > + > + // Check the transformed flags and remove it > + PRBool isTransformed = (aFlags & INVALIDATE_ALREADY_TRANSFORMED); "rectIsTransformed" to be clearly different from IsTransformed()? @@ +4601,5 @@ > > /* If we're transformed, the matrix will be relative to our > * cross-doc parent frame. > */ > + //*aOutAncestor = nsLayoutUtils::GetCrossDocParentFrame(this); Remove @@ +6670,5 @@ > + } > + } else { > + // When we are preserving 3d we need to iterate over all children separately. > + // If the child also preserves 3d then their overflow will already been in our > + // coordinate space, otherwise we need to transform. Move this code to a helper function? FinishAndStoreOverflow is already a bit long :-) ::: layout/generic/nsIFrame.h @@ +1192,5 @@ > /** > + * Returns whether this frame will attempt to preserve the 3d transforms of its > + * children. This is a direct indicator of -moz-transform-style: preserve-3d. > + */ > + virtual PRBool Preserves3DChildren() const; Doesn't need to be virtual? @@ +1195,5 @@ > + */ > + virtual PRBool Preserves3DChildren() const; > + > + /** > + * Returns whether this child frame will preserve 3d transforms. Needs a better comment to distinguish it from the previous method. @@ +1197,5 @@ > + > + /** > + * Returns whether this child frame will preserve 3d transforms. > + */ > + virtual PRBool Preserves3D() const; Doesn't need to be virtual?

Comment on attachment 548077 [details] [diff] [review] Part 10 - Implement -moz-backface-visible v2 r=dbaron

Comment on attachment 548078 [details] [diff] [review] Part 11a: Add nsCSSValueTriplet and optionally read a z component to -moz-transform-origin v2 >+ if (CheckEndProperty() || >+ !ParseVariant(depth, VARIANT_LENGTH | VARIANT_CALC, nsnull) || >+ !nsLayoutUtils::Are3DTransformsEnabled()) { I think you can drop the CheckEndProperty || here. >+ NS_ABORT_IF_FALSE(xValue.GetUnit() != eCSSUnit_Null && >+ yValue.GetUnit() != eCSSUnit_Null && >+ xValue.GetUnit() != eCSSUnit_Inherit && >+ yValue.GetUnit() != eCSSUnit_Inherit && >+ zValue.GetUnit() != eCSSUnit_Inherit && >+ xValue.GetUnit() != eCSSUnit_Initial && >+ yValue.GetUnit() != eCSSUnit_Initial && >+ zValue.GetUnit() != eCSSUnit_Initial, >+ "inappropriate pair value"); "pair" -> "triplet" Also, please add a comment noting that a null Z value *is* allowed, since it isn't obvious when skimming. (both comments repeated for both SetTripletValue variants) nsRuleNode.cpp: You still have the SetTripletCoords function, but you're not calling it. Remove it. (It has bugs, too, but I won't bother describing them!) It would probably be useful to use mozilla::DebugOnly<PRBool> in your new code in ComputeDisplayData. >+ if (valZ.GetUnit() == eCSSUnit_Null) { >+ display->mTransformOrigin[2].SetCoordValue(0); You should add a comment here that we've already separately checked transformOriginValue against eCSSUnit_Null, so this is safe (i.e., clearly means 0 rather than unspecified). r=dbaron with those things fixed

Comment on attachment 549699 [details] [diff] [review] Part 9 - Implement the perspective() transform function and style property. v4 Carrying forward r=dbaron

Landed parts 6 through 12b on mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/ba5eb1cd42f3 http://hg.mozilla.org/integration/mozilla-inbound/rev/e7dc1c09ae24 http://hg.mozilla.org/integration/mozilla-inbound/rev/a637d482b540 http://hg.mozilla.org/integration/mozilla-inbound/rev/74eac3a09043 http://hg.mozilla.org/integration/mozilla-inbound/rev/08b756f93436 http://hg.mozilla.org/integration/mozilla-inbound/rev/96941ed69aeb http://hg.mozilla.org/integration/mozilla-inbound/rev/85cc7836c552 http://hg.mozilla.org/integration/mozilla-inbound/rev/ea882f18d8ac http://hg.mozilla.org/integration/mozilla-inbound/rev/ad334152010a http://hg.mozilla.org/integration/mozilla-inbound/rev/2d3b6382054d http://hg.mozilla.org/integration/mozilla-inbound/rev/166f4f247772

Excitement! /be

http://hg.mozilla.org/mozilla-central/rev/ba5eb1cd42f3 http://hg.mozilla.org/mozilla-central/rev/e7dc1c09ae24 http://hg.mozilla.org/mozilla-central/rev/a637d482b540 http://hg.mozilla.org/mozilla-central/rev/74eac3a09043 http://hg.mozilla.org/mozilla-central/rev/08b756f93436 http://hg.mozilla.org/mozilla-central/rev/96941ed69aeb http://hg.mozilla.org/mozilla-central/rev/85cc7836c552 http://hg.mozilla.org/mozilla-central/rev/ea882f18d8ac http://hg.mozilla.org/mozilla-central/rev/ad334152010a http://hg.mozilla.org/mozilla-central/rev/2d3b6382054d http://hg.mozilla.org/mozilla-central/rev/166f4f247772

Cool!

> > @@ +1560,5 @@ > > resultList.AppendToTop(set.BlockBorderBackgrounds()); > > // 5: floats > > resultList.AppendToTop(set.Floats()); > > // 7: general content > > + set.Content()->SortByZPosition(aBuilder, GetContent()); > > Hmm, this is a little bit scary. Maybe make this if (Preserves3DChildren())? > And maybe instead of calling SortByZPosition and having SortByZPosition > account for 3D, use a separate SortByZPosition3D path which you use here? > Can you explain this a bit more please. I've actually removed this line in my queue because it was causing tests to fail. What exactly is in the Content() group? I was under the impression that everything would have a z-order of 0 (positive/negative z-order content is separate), and no z-position (exisiting tests won't have 3d transforms), so this would just be a content-order sort and a no-op.

Relatedly, all the 3d content that I've tested ends up in the positive z-order group, so this line doesn't actually appear to be necessary at all.

(In reply to comment #155) > > > > @@ +1560,5 @@ > > > resultList.AppendToTop(set.BlockBorderBackgrounds()); > > > // 5: floats > > > resultList.AppendToTop(set.Floats()); > > > // 7: general content > > > + set.Content()->SortByZPosition(aBuilder, GetContent()); > > > > Hmm, this is a little bit scary. Maybe make this if (Preserves3DChildren())? > > And maybe instead of calling SortByZPosition and having SortByZPosition > > account for 3D, use a separate SortByZPosition3D path which you use here? > > > > Can you explain this a bit more please. The scariness is that you're re-sorting everything by CSS z-index and content order, and if any code has deliberately violated that order then you'll be undoing that. I don't remember off the top of my head what does that, but it sounds like that's what's causing your tests failing. There's also an efficiency issue, you're adding an O(N log N) pass over the content list. > I've actually removed this line in my queue because it was causing tests to > fail. What exactly is in the Content() group? Content() contains everything "normal" that doesn't belong in one of the other lists. See nsDisplayList.h. > Relatedly, all the 3d content that I've tested ends up in the positive > z-order group, so this line doesn't actually appear to be necessary at all. Right, nsDisplayTransform items should always be in the PositionedDescendants list since they are pseudo-stacking-contexts.

Is it planned to have a media query available to test for 3d transform support? Regards, Lr

(In reply to Louis-Rémi BABE from comment #158) > Is it planned to have a media query available to test for 3d transform > support? This feature can't be detected afaik. So a mediaQuery sounds appropriate. Webkit does that, right?

(In reply to Paul Rouget [:paul] from comment #159) > (In reply to Louis-Rémi BABE from comment #158) > > Is it planned to have a media query available to test for 3d transform > > support? > > This feature can't be detected afaik. So a mediaQuery sounds appropriate. > Webkit does that, right? There's also dbaron's @supports[1], but I guess that wouldn't be ready in time. [1] http://dev.w3.org/csswg/css3-conditional/#at-supports

hi guys. i really love how much progress and effort goes into supporting new standards and bringing the web forward. i’m following this bug report for quite some time now. but every time i see how some parts are advancing and getting care from so many, it feels funny how other small deetails are neglected despite having hideous bugs for years. specifically, i’m talking about some elements not being stylable, partly or at all (bug 418833, bug 52500, bug 402625 and bug 79107), buttons still have this invisible-yet-padded “::-moz-focus-inner” (bug 140562), and cursor themes are not completely supported (bug 609889) (firefox uses it’s own 90ies-cursors sometimes) i hope this gets one or two of you to look at these, and my whinig didn’t distract you too much, but i think new standards are not everything, and little old inconsitencies matter, too. i just tried to direct some of the positive energy here elwhere, where it is needed imho, too.

You could technically feature detect by setting a 3d transform on something and examining the computed style, but that's annoying and why we implemented the media query in webkit.

Currently the plan is to support 3d-transforms everywhere (see bug 675837). The performance won't be great without GPU acceleration, so maybe we need a query for whether transforms are GPU-accelerated.

At least the Apple port of WebKit for Safari only does 3d in accelerated content. We don't have a software fallback. I'm not sure if other ports do. In other words, for Safari our media query for 3d was basically enough to guarantee performance. We were quite hesitant to introduce a media query that is used as feature detection. As roc says, this is really testing performance, not the media. Oh well :(