Closed Bug 1207734 Opened 5 years ago Closed 2 years ago

Implement individual transform properties: the 'translate', 'scale', and 'rotate' properties

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: dbaron, Assigned: birtles)

References

(Blocks 2 open bugs, )

Details

(Keywords: css3, dev-doc-complete)

Attachments

(56 files, 20 obsolete files)

WIP
44.01 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
heycam
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
528 bytes, text/html
Details
398 bytes, text/html
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
billm
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
1017 bytes, text/html
Details
375 bytes, text/html
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
boris
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
41 bytes, text/x-github-pull-request
Details | Review
59 bytes, text/x-review-board-request
Details
6.79 KB, patch
emilio
: review+
Details | Diff | Splinter Review
We should implement the separate translate, rotate, and scale properties described in https://drafts.csswg.org/css-transforms-2/#individual-transforms .  I think the proposal is pretty non-controversial, and it's frequently requested by developers.
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Here is the plan:

Phase 1: CSSValue parsing, storing, and computing
=> Parsing: Parse each individual transform property as a new CSS attribute
=> Storing: Store each parsed value into a rule node (maybe more customized data structure if needed)
=> Computing: Compute and transform each individual property to a geometric transform matrix

Phase 2: Combine the computed matrix with original css-transform matrix, and then do the rendering
=> Either do the combination first, then take a single combined transform matrix to the renderer; or take separated matrices to the renderer and let the renderer to the compositing. Haven't study yet, just few thoughts.

Phase 3: Test case
=> Refer to the original css-transform and make similar basic test cases

I'm about in 50% of Phase 1 before interrupted by another bug. I should be back soon. Once I back to this bug, I'll upload a working patch and keep update it until I resolve this.
Attached file wip (obsolete) —
Attachment #8722844 - Attachment mime type: text/plain → text/html
With this wip, I can get individual translate work w/ following css style:

#div-transform {
  translate: 50px 50px;
}

However, when setting individual translate together w/ transform, none of both would work. For example, if I set a div with following css style:

#div-transform {
  translate: 50px 50px;
  transform: translateX(30px);
}

Neither two translate properties will work. Maybe there's some other code paths I should've taken into consideration.


Hi David, could you take a look at this? I'm wondering if this is the right place to combine parsed individual transforms into the existing transform? If yes, what else I may miss to get the property work? If not, could you guide me to the right direction? Any suggestion would be very appreciated.
Attachment #8722844 - Attachment is obsolete: true
Attachment #8727704 - Flags: feedback?(dbaron)
Comment on attachment 8727704 [details] [diff] [review]
(wip) Implement individual translate property

This is not the right place to implement the combining.

The 'translate' property should not change the computed value of 'transform'.  But presumably it should change nearly everything that operates on nsStyleDisplay::mSpecifiedTransform.

Also, animations of 'transform' and 'translate' should essentially work independently -- which actually makes implementing this a bit more complicated than I was thinking.


I'm actually having second thoughts about whether this is worth implementing; I think it's a little more complicated than I previously thought.  Do we know if any other browser vendors are doing it?
Attachment #8727704 - Flags: feedback?(dbaron) → feedback-
(In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #4)
> Also, animations of 'transform' and 'translate' should essentially work
> independently -- which actually makes implementing this a bit more
> complicated than I was thinking.

In particular, I believe this means we'd need to send them separately to the compositor, add add significantly more animation capability to the compositor, which is a good bit of work.
(In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #4)
> Comment on attachment 8727704 [details] [diff] [review]
> (wip) Implement individual translate property
> 
> This is not the right place to implement the combining.
> 
> The 'translate' property should not change the computed value of
> 'transform'.  But presumably it should change nearly everything that
> operates on nsStyleDisplay::mSpecifiedTransform.
> 
> Also, animations of 'transform' and 'translate' should essentially work
> independently -- which actually makes implementing this a bit more
> complicated than I was thinking.
> 
> 
> I'm actually having second thoughts about whether this is worth
> implementing; I think it's a little more complicated than I previously
> thought.  Do we know if any other browser vendors are doing it?

Thank you for the feedback. Actually, there's a bug filed in chromium :https://bugs.chromium.org/p/chromium/issues/detail?id=492455 But there are no further actions since last May. AFAIK, it seems that none of other vendors have plans about implementing this.

After discussing with David offline, I'm going to un-take this bug and work on other topics.
Assignee: jeremychen → nobody
Status: ASSIGNED → NEW
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #4)
> I'm actually having second thoughts about whether this is worth
> implementing; I think it's a little more complicated than I previously
> thought.  Do we know if any other browser vendors are doing it?

Blink has shipped this, and according to the issue Edge reported to CSSWG [1], they are going to ship (or also have shipped) this feature.

[1] https://github.com/w3c/csswg-drafts/issues/856
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #5)
> (In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #4)
> > Also, animations of 'transform' and 'translate' should essentially work
> > independently -- which actually makes implementing this a bit more
> > complicated than I was thinking.
> 
> In particular, I believe this means we'd need to send them separately to the
> compositor, add add significantly more animation capability to the
> compositor, which is a good bit of work.

It seems to me that there aren't many places accessing nsStyleDisplay::mSpecifiedTransform (probably three outside the style system), but there are certain amount of specific code for eCSSProperty_transform which may need investigation. That is ~35 places which are mainly in dom/animation, gfx/layers, and layout code (excluding style system).

I'm not completely understand the difficulty here. I guess it would be a good bit of work, but shouldn't be too bad. Given that two other engines is shipping this, we may want to do it as well.
I agree we want to do this. It makes animation a lot easier, at least until we ship additive animation (and even then, it provides a fixed order of operation which is useful).

The dom/animation side shouldn't be too hard--we need to add this to the list of compositor-animatable properties and make sure all the optimizations that, for example, disable async transform animations in certain situations, also apply to these properties. The hardest part of the dom/animation work is probably refactoring the tests accordingly but perhaps we can just extend the existing tests to cover some of these extra combinations.
Summary: implement 'translate', 'rotate', and 'scale' properties → Implement individual transform properties: the 'translate', 'scale', and 'rotate' properties
Alias: individual-transform
> Blink has shipped this

The properties are behind a flag in Blink (enable experimental web platform features).

Web platform tests are in
https://github.com/w3c/web-platform-tests/tree/master/css/css-transforms-2


For WebKit I've created bug
https://bugs.webkit.org/show_bug.cgi?id=178117
Attachment #8727704 - Attachment is obsolete: true
Assignee: nobody → cku
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch WIP-without-animation-support (obsolete) — Splinter Review
Attached patch WIPSplinter Review
Attachment #8934648 - Attachment is obsolete: true
Attachment #8935269 - Flags: feedback?(boris.chiou)
Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review211738

OK, the original transform list handle `None` by a different way which based on the whole transform list, instead of the individual transform operation. For the individual transform properties, we have to implement `Animate` by ourself. This makes sense.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2252
(Diff revision 1)
>      }
>  }
>  
>  /// <https://drafts.csswg.org/css-transforms/#interpolation-of-transforms>
> +impl ComputedRotate {
> +    fn fill_unspecified(rotate_: &ComputedRotate)

I think you don't need to add `_` after the variable name.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2257
(Diff revision 1)
> +    fn fill_unspecified(rotate_: &ComputedRotate)
> +        -> Result<(Number, Number, Number, Angle), ()>{
> +        // If the axis is unspecified, it defaults to "0 0 1"
> +        match rotate_ {
> +            &Rotate::None =>
> +                Ok((0., 0., 1., Angle::Deg(0.))),

`Angle::zero()`?
nit: And maybe put this in the same line with `&Rotate::None` might be better.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2262
(Diff revision 1)
> +                Ok((0., 0., 1., Angle::Deg(0.))),
> +            &Rotate::Specified(None, None, None, angle) =>
> +                Ok((0., 0., 1., angle)),
> +            &Rotate::Specified(Some(rx), Some(ry), Some(rz), angle) =>
> +                Ok((rx, ry, rz, angle)),
> +            &_ =>

I guess using `_` is enough?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2272
(Diff revision 1)
> +
> +impl Animate for ComputedRotate {
> +    #[inline]
> +    fn animate(
> +        &self,
> +        other_: &Self,

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2288
(Diff revision 1)
> +        Ok(Rotate::Specified(Some(rx), Some(ry), Some(rz), angle))
> +    }
> +}
> +
> +impl ComputedTranslate {
> +    fn fill_unspecified(translate_: &ComputedTranslate)

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2292
(Diff revision 1)
> +            &Translate::None =>
> +                Ok((LengthOrPercentage::Length(Length::new(0.)),
> +                    LengthOrPercentage::Length(Length::new(0.)),
> +                    Length::new(0.))),

I prefer use `{}` for multiple lines.
i.e.
```
&Translate:None => {
  Ok((
      LengthOrPercentage::Length(Length::new(0.)),
      LengthOrPercentage::Length(Length::new(0.)),
      Length::new(0.),
  ))
},
```
Besides, maybe you can use `Length::zero()`, I guess.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2300
(Diff revision 1)
> +                    Length::new(0.))),
> +            &Translate::Specified(tx, Some(ty), None) =>
> +                Ok((tx, ty, Length::new(0.))),
> +            &Translate::Specified(tx, Some(ty), Some(tz)) =>
> +                Ok((tx, ty, tz)),
> +            &_ =>

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2310
(Diff revision 1)
> +
> +impl Animate for ComputedTranslate {
> +    #[inline]
> +    fn animate(
> +        &self,
> +        other_: &Self,

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2325
(Diff revision 1)
> +        Ok(Translate::Specified(sx, Some(sy), Some(sz)))
> +    }
> +}
> +
> +impl ComputedScale {
> +    fn fill_unspecified(scale_: &ComputedScale)

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2335
(Diff revision 1)
> +                Ok((1.0, 1.0, 1.0)),
> +            &Scale::Specified(sx, Some(sy), None) =>
> +                Ok((sx, sy, 1.0)),
> +            &Scale::Specified(sx, Some(sy), Some(sz)) =>
> +                Ok((sx, sy, sz)),
> +            &_ =>

same here

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2345
(Diff revision 1)
> +
> +impl Animate for ComputedScale {
> +    #[inline]
> +    fn animate(
> +        &self,
> +        other_: &Self,

same here
Attachment #8935269 - Flags: feedback?(boris.chiou)
boris said: add test cases in test_distance_of_transform.html for ComputedSquareDistance
Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review211744

BTW, we still need to check ComputeSqauredDistance and add some tests in [1].
[1] https://searchfox.org/mozilla-central/source/dom/animation/test/mozilla/test_distance_of_transform.html
(In reply to C.J. Ku[:cjku](UTC+8) from comment #27)
> boris said: add test cases in test_distance_of_transform.html for
> ComputedSquareDistance

Yaya. Or file a new bug for this because there are too many patches in this bug, I think.
test cases in chromium for individual transform:
https://src.chromium.org/viewvc/blink?view=revision&revision=197547
Alias: individual-transform
Attachment #8935262 - Attachment is obsolete: true
Attachment #8935266 - Attachment is obsolete: true
Attachment #8935258 - Flags: review?(emilio)
Attachment #8935259 - Flags: review?(emilio)
Attachment #8935260 - Flags: review?(emilio)
Attachment #8935261 - Flags: review?(emilio)
Attachment #8935263 - Flags: review?(emilio)
Attachment #8935264 - Flags: review?(emilio)
Attachment #8935265 - Flags: review?(emilio)
Attachment #8936571 - Flags: review?(emilio)
Attachment #8936572 - Flags: review?(emilio)
Attachment #8936573 - Flags: review?(emilio)
Attachment #8935268 - Flags: review?(emilio)
Attachment #8935269 - Flags: review?(emilio)
Attachment #8936574 - Flags: review?(emilio)
Attachment #8936575 - Flags: review?(emilio)
Attachment #8936577 - Flags: review?(emilio)
Attachment #8936578 - Flags: review?(emilio)
Attachment #8935267 - Flags: review?(emilio)
Attachment #8936576 - Flags: review?(emilio)
CJ, I haven't gone through all these patches, but from what I skimmed, I didn't see any changes to the data we send to the compositor. I would have thought we'd need to let the compositor know which property we're animating so we make sure to compose the transform in the correct order on the compositor. Did I miss it? Or does it just work somehow?
(In reply to Brian Birtles (:birtles) from comment #49)
> CJ, I haven't gone through all these patches, but from what I skimmed, I
> didn't see any changes to the data we send to the compositor. I would have
> thought we'd need to let the compositor know which property we're animating
> so we make sure to compose the transform in the correct order on the
> compositor. Did I miss it? Or does it just work somehow?

In Part 4.c., nsDisplayTransform read the final transform from nsStyleDisplay, and nsDisplayTransform will take care of other thing(I guess)
From what I understand, I don't think that works. I think that means we will end up sending the combined transform to the compositor but I think we actually need to be able to animate 'scale' etc. independently, run those animations on the compositor, and then compose them in the correct order on the compositor.
(In reply to Brian Birtles (:birtles) from comment #51)
> From what I understand, I don't think that works. I think that means we will
> end up sending the combined transform to the compositor but I think we
> actually need to be able to animate 'scale' etc. independently, run those
> animations on the compositor, and then compose them in the correct order on
> the compositor.

div {
  scale: 1 2;
  transform: scale(2, 3);
}

The data that we store in the combined transform is
["scale3d", "1", "2" "1"], ["scale2d", "2" , "3"]

'scale' property will be converted into a scale3d function and be put into mCombinedTransform. ('translate' property will be converted into a translate3d function, and 'rotate' will be converted into a rotate3d one).


Although I named it as mCombinedTransform, it actually stores transform-functions separately. 'combined' here means combine individual transform functions with transform functions in 'transform' property. The logic of how to combine all these function is in 4.a(nsStyleDisplay::UpdateCombinedTransform())
Comment on attachment 8935258 [details]
Bug 1207734 - Part 1.a. Implement ReleaseSharedListOnMainThread to reuse the code of releasing nsCSSValueSharedList objects hold by a style struct.

https://reviewboard.mozilla.org/r/206148/#review213114
Attachment #8935258 - Flags: review?(emilio) → review+
Comment on attachment 8935259 [details]
Bug 1207734 - Part 1.b. Do not initialize mSpecifiedTransform in ctor.

https://reviewboard.mozilla.org/r/206150/#review213116
Attachment #8935259 - Flags: review?(emilio) → review+
Comment on attachment 8935260 [details]
Bug 1207734 - Part 1.d. Carry the computed value of individual transform in nsStyleDisplay.

https://reviewboard.mozilla.org/r/206152/#review213118

::: layout/style/nsStyleStruct.cpp:3751
(Diff revision 2)
>        shapeImage->ResolveImage(aPresContext);
>      }
>    }
>  }
>  
> +#define COMPARE_TRANSFORM_VALUES(list_) \

Can this be a static function instead?

```
static inline nsChangeHint
CompareTransformValues(..)
{
}
```

Where the "they're equal" path returns `nsChangeHint(0)`?

Then you can use:

```
transformHint |= CompareTransformValues(mSpecifiedTransform, other.mSpecifiedTransform);
// ...
```
Attachment #8935260 - Flags: review?(emilio) → review+
I probably should change these two place to use mCombinedTransform?
https://searchfox.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#4639
https://searchfox.org/mozilla-central/source/layout/painting/ActiveLayerTracker.cpp#251

hi Brian,
And I guess I don't need to take care of StyleAnimationValue?
https://searchfox.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#4639
Flags: needinfo?(bbirtles)
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213120

::: layout/style/nsCSSPropList.h:3631
(Diff revision 2)
> +    CSS_PROPERTY_PARSE_FUNCTION |
> +        CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH |
> +        CSS_PROPERTY_CREATES_STACKING_CONTEXT |
> +        CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR |
> +        CSS_PROPERTY_FIXPOS_CB,
> +    "",

Should we pref them off by default?

::: layout/style/nsCSSPropList.h:3635
(Diff revision 2)
> +        CSS_PROPERTY_FIXPOS_CB,
> +    "",
> +    0,
> +    nullptr,
> +    offsetof(nsStyleDisplay, mSpecifiedRotate),
> +    eStyleAnimType_Custom)

I'm assuming you're checking with Birtles all the animation bits and such.

::: layout/style/nsCSSPropList.h:4278
(Diff revision 2)
> +        CSS_PROPERTY_FIXPOS_CB,
> +    "",
> +    0,
> +    nullptr,
> +    offsetof(nsStyleDisplay, mSpecifiedScale),
> +eStyleAnimType_Custom)

nit: indentation.
Attachment #8935261 - Flags: review?(emilio) → review+
Comment on attachment 8935263 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/206158/#review213126

::: servo/components/style/values/generics/transform.rs:761
(Diff revision 2)
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
> +/// A value of the `Rotate` property

Please link to the spec.

::: servo/components/style/values/generics/transform.rs:766
(Diff revision 2)
> +/// A value of the `Rotate` property
> +pub enum Rotate<Number, Angle> {
> +    /// 'none'
> +    None,
> +    /// '<number>{3}? <angle>'
> +    Specified(Option<Number>, Option<Number>, Option<Number>, Angle)

This should be `Option<(Number, Number, Number)>, Angle`, and that way you can remove the `panic!`s.

::: servo/components/style/values/generics/transform.rs:769
(Diff revision 2)
> +    None,
> +    /// '<number>{3}? <angle>'
> +    Specified(Option<Number>, Option<Number>, Option<Number>, Angle)
> +}
> +
> +impl<Number: ToCss + Copy + fmt::Debug, Angle: ToCss + Copy + fmt::Debug> ToCss for Rotate<Number, Angle> {

I think this can just be `derive`d, adding `ToCss` to the `derive()` list, any reason it cannot be done?
Attachment #8935263 - Flags: review?(emilio)
Attachment #8935261 - Flags: review?(bbirtles)
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213120

> I'm assuming you're checking with Birtles all the animation bits and such.

I think this value is for old system
Attachment #8935268 - Flags: review?(bbirtles)
Attachment #8935269 - Flags: review?(bbirtles)
Attachment #8936574 - Flags: review?(bbirtles)
Attachment #8936574 - Flags: review?(bbirtles)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #54)
> div {
>   scale: 1 2;
>   transform: scale(2, 3);
> }
> 
> The data that we store in the combined transform is
> ["scale3d", "1", "2" "1"], ["scale2d", "2" , "3"]
> 
> 'scale' property will be converted into a scale3d function and be put into
> mCombinedTransform. ('translate' property will be converted into a
> translate3d function, and 'rotate' will be converted into a rotate3d one).
> 
> 
> Although I named it as mCombinedTransform, it actually stores
> transform-functions separately. 'combined' here means combine individual
> transform functions with transform functions in 'transform' property. The
> logic of how to combine all these function is in
> 4.a(nsStyleDisplay::UpdateCombinedTransform())

Right, but I'm still not sure that if, for example, we start a scale animation, then later start a translate animation, that when we animate those two on the compositor, we'll know that we need to combine the scale after the translate (since normally we'll send those independent animations in composite order, which in this case will be based on when they are created).
Comment on attachment 8935264 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/206160/#review213130

::: servo/components/style/values/computed/transform.rs:15
(Diff revision 2)
>  use values::animated::ToAnimatedZero;
>  use values::computed::{Angle, Integer, Length, LengthOrPercentage, Number, Percentage};
>  use values::computed::{LengthOrNumber, LengthOrPercentageOrNumber};
>  use values::generics::transform::{self, Matrix as GenericMatrix, Matrix3D as GenericMatrix3D};
>  use values::generics::transform::{Transform as GenericTransform, TransformOperation as GenericTransformOperation,
> -    Rotate as GenericRotate};
> +    Rotate as GenericRotate, Translate as GenericTranslate};

use multiple `use` lines instead of multi-line imports.

::: servo/components/style/values/generics/transform.rs:798
(Diff revision 2)
>          }
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]

I think you could `derive(ToCss)` here too.

::: servo/components/style/values/generics/transform.rs:799
(Diff revision 2)
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
> +/// A value of the `Translate` property

Link to spec.

::: servo/components/style/values/generics/transform.rs:804
(Diff revision 2)
> +/// A value of the `Translate` property
> +pub enum Translate<LengthOrPercentage, Length> {
> +    /// 'none'
> +    None,
> +    /// '<number>{1,3}'
> +    Specified(LengthOrPercentage, Option<LengthOrPercentage>, Option<Length>)

These are definitely not `<number>`s, so either the comment needs updating, or the types here should change.
Attachment #8935264 - Flags: review?(emilio)
Comment on attachment 8935265 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/206162/#review213136

::: servo/components/style/properties/gecko.mako.rs:683
(Diff revision 2)
> +        use values::generics::transform::TransformOperation;
> +
> +        unsafe { self.gecko.${gecko_ffi_name}.clear() };
> +
> +        match other {
> +            Scale::None => (),

This needs to reset the property, not do nothing.

::: servo/components/style/properties/gecko.mako.rs:685
(Diff revision 2)
> +        unsafe { self.gecko.${gecko_ffi_name}.clear() };
> +
> +        match other {
> +            Scale::None => (),
> +            Scale::Specified(sx, None, None) => {
> +                let operation = vec![TransformOperation::ScaleX(sx)];

Please (this also applies to the previous patches) don't use `vec![]` here, that implies a memory allocation that you can trivially avoid everywhere.

::: servo/components/style/properties/longhand/box.mako.rs:626
(Diff revision 2)
>                            spec="https://drafts.csswg.org/css-transforms-2/#individual-transforms")}
>  
> +${helpers.predefined_type("scale", "Scale",
> +                          "generics::transform::Scale::None",
> +                          animation_value_type="ComputedValue",
> +                          gecko_ffi_name="mSpecifiedScale",

I think gecko_ffi_name (in the previous patches too) is useless here.

::: servo/components/style/values/generics/transform.rs:841
(Diff revision 2)
>      }
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
> +/// A value of the `Scale` property

As before, I think you should add a comment to the spec and derive `ToCss`.

::: servo/components/style/values/generics/transform.rs:845
(Diff revision 2)
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq)]
> +/// A value of the `Scale` property
> +pub enum Scale<Number> {
> +    /// 'none'
> +    None,
> +    /// '<length-percentage> [ <length-percentage> <length>? ]?'

These are definitely not lengths, so just as before, either this comment should be updated, or the types should change.
Attachment #8935265 - Flags: review?(emilio)
(In reply to Brian Birtles (:birtles) from comment #62)
> Right, but I'm still not sure that if, for example, we start a scale
> animation, then later start a translate animation, that when we animate
> those two on the compositor, we'll know that we need to combine the scale
> after the translate (since normally we'll send those independent animations
> in composite order, which in this case will be based on when they are
> created).
According to the spec:
https://drafts.csswg.org/css-transforms-2/#ctm
'translate' is always before 'scale'. If you want to combine 'translate' after 'scale', you can only use 'transform' property.
Attachment #8936571 - Flags: review?(bbirtles)
Attachment #8936572 - Flags: review?(bbirtles)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #65)
> (In reply to Brian Birtles (:birtles) from comment #62)
> > Right, but I'm still not sure that if, for example, we start a scale
> > animation, then later start a translate animation, that when we animate
> > those two on the compositor, we'll know that we need to combine the scale
> > after the translate (since normally we'll send those independent animations
> > in composite order, which in this case will be based on when they are
> > created).
> According to the spec:
> https://drafts.csswg.org/css-transforms-2/#ctm
> 'translate' is always before 'scale'. If you want to combine 'translate'
> after 'scale', you can only use 'transform' property.

Right, but what makes us preserve that order when animating on the compositor? We don't tell the compositor if we are animating 'translate' or 'transform' right?
Flags: needinfo?(bbirtles)
Once I have a build of this patch series going, I'll make up a test case to demonstrate what I mean.
Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review213142

I'm not sure how this plays with all the animation stuff... Probably birtles is better reviewer for this.

::: layout/style/nsStyleStruct.h:2601
(Diff revision 1)
>    StyleGeometryBox mTransformBox; // [reset] see nsStyleConsts.h
>    RefPtr<nsCSSValueSharedList> mSpecifiedTransform; // [reset]
>    RefPtr<nsCSSValueSharedList> mSpecifiedRotate; // [reset]
>    RefPtr<nsCSSValueSharedList> mSpecifiedTranslate; // [reset]
>    RefPtr<nsCSSValueSharedList> mSpecifiedScale; // [reset]
> +  RefPtr<nsCSSValueSharedList> mCombinedTransform;

Please document.

::: layout/style/nsStyleStruct.cpp:4022
(Diff revision 1)
> +  // According to the spec:
> +  // https://drafts.csswg.org/css-transforms-2/#ctm
> +  //
> +  // Combine transform properties by the following sequence:
> +  //   translate > rotate > scale > transform
> +  const RefPtr<nsCSSValueSharedList> shareLists[] = { mSpecifiedTranslate,

There's no need for this to addref everything, `const RefPtr<nsCSSValueSharedList>&`?

::: layout/style/nsStyleStruct.cpp:4027
(Diff revision 1)
> +  const RefPtr<nsCSSValueSharedList> shareLists[] = { mSpecifiedTranslate,
> +                                                      mSpecifiedRotate,
> +                                                      mSpecifiedScale,
> +                                                      mSpecifiedTransform};
> +
> +  nsTArray<nsCSSValueList*> valueLists;

Let's reserve space beforehand to avoid reallocating.
Attachment #8936571 - Flags: review?(emilio)
Comment on attachment 8936572 [details]
Bug 1207734 - Part 4.b. (stylo) Update nsStyleDisplay::mCombinedTransform once transform value changed.

https://reviewboard.mozilla.org/r/207328/#review213148

You should set this after cascading all the properties, look at:

  https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/servo/components/style/properties/properties.mako.rs#3508

::: servo/components/style/properties/gecko.mako.rs:569
(Diff revision 1)
>              x => {
>                  panic!("Found unexpected value in style struct for rotate property: {:?}", x)
>              }
>          }
> +        unsafe {
> +            Gecko_UpdateTransform(&mut self.gecko);

This is not the right place to do this, this doesn't handle `inherit` or `reset` keywords properly.
Attachment #8936572 - Flags: review?(emilio)
Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

https://reviewboard.mozilla.org/r/207330/#review213154

I don't really know about `FrameTransformProperties` :)
Attachment #8936573 - Flags: review?(emilio)
Comment on attachment 8935267 [details]
Bug 1207734 - Part 2. Add a preference to enable/disable individual transform.

https://reviewboard.mozilla.org/r/206166/#review213156
Attachment #8935267 - Flags: review?(emilio) → review+
Comment on attachment 8935268 [details]
Bug 1207734 - Part 6.a. Update LayerAnimationInfo list according to the new added transform properties.

https://reviewboard.mozilla.org/r/206168/#review213158

If birtles can review this, that'd be great. I can take a look at what LayerAnimationInfo does, but I'm not terribly familiar with this code.
Attachment #8935268 - Flags: review?(emilio)
Comment on attachment 8936577 [details]
Bug 1207734 - Part 8.b. (stylo) Update alias names for rotate/translate/scale.

https://reviewboard.mozilla.org/r/207338/#review213164

::: servo/components/style/properties/longhand/box.mako.rs:611
(Diff revision 1)
>                            flags="CREATES_STACKING_CONTEXT FIXPOS_CB",
>                            spec="https://drafts.csswg.org/css-transforms/#propdef-transform")}
>  
>  ${helpers.predefined_type("rotate", "Rotate",
>                            "generics::transform::Rotate::None",
> +                          extra_prefixes="webkit",

Can you elaborate on why the alias? Are they in any compat spec?

Does this have any webcompat implications?

I'd prefer not supporting the webkit prefix if we don't need to.
Attachment #8936577 - Flags: review?(emilio)
(In reply to Brian Birtles (:birtles) from comment #66)
> (In reply to C.J. Ku[:cjku](UTC+8) from comment #65)
> > (In reply to Brian Birtles (:birtles) from comment #62)
> > > Right, but I'm still not sure that if, for example, we start a scale
> > > animation, then later start a translate animation, that when we animate
> > > those two on the compositor, we'll know that we need to combine the scale
> > > after the translate (since normally we'll send those independent animations
> > > in composite order, which in this case will be based on when they are
> > > created).
> > According to the spec:
> > https://drafts.csswg.org/css-transforms-2/#ctm
> > 'translate' is always before 'scale'. If you want to combine 'translate'
> > after 'scale', you can only use 'transform' property.
> 
> Right, but what makes us preserve that order when animating on the
> compositor? We don't tell the compositor if we are animating 'translate' or
> 'transform' right?

(To be honest, I am not that familiar of how animation coworks with compositor. So, I try to explain how I think it works in my mind)

Let said we have a div matches these two rules
div {
  scale: 1 1;
  translate: 50px 50px;
  transition: scale 2s, translate 2s;
}
div:hover {
  scale: 2 2;
  translate: 100px 100px;
}
<div></div>


When we hover on that div, transition being trigger, the transform-functions that we give to nsDisplayTransform looks like
1. At the beginning, the content in mCombinedTransform is
   [['translate3d', '50px', '50px', '0px'], ['scale3d', '1', '1', '1']]
2. 1 second pass, by the function that I added in part 6.b, the content of mCombinedTransform becomes:
   [['translate3d', '75px', '75px', '0px'], ['scale3d', '1.5', '1.5', '1.5']]
3. In the end(2 seconds pass), the content of mCombinedTransform becomes:
   [['translate3d', '100px', '100px', '0px'], ['scale3d', '2', '2', '2']]
The animation trait of individual transform(in part 6.b) keep update interpolated prop value for scale/translate and rotate separatly.
(In reply to Brian Birtles (:birtles) from comment #67)
> Once I have a build of this patch series going, I'll make up a test case to
> demonstrate what I mean.

That will be really helpful. Thanks
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213170

::: layout/style/nsCSSPropList.h:3626
(Diff revision 2)
>      eStyleAnimType_Sides_Right)
> +CSS_PROP_DISPLAY(
> +    rotate,
> +    rotate,
> +    Rotate,
> +    CSS_PROPERTY_PARSE_FUNCTION |

Hmm, I'm not sure this should use `CSS_PROPERTY_PARSE_FUNCTION`, should it?

Let's cam have a look, since he's more familiar with Gecko's CSS parser.
Attachment #8935261 - Flags: review?(cam)
Attachment #8936573 - Flags: review?(bbirtles)
Comment on attachment 8936576 [details]
Bug 1207734 - Part 8.a. (gecko) Update alias names for rotate/translate/scale.

https://reviewboard.mozilla.org/r/207336/#review213172

I don't really think we should add a webkit alias for these properties just because transform happens to have it too. What's the reasoning for this change?
Attachment #8936576 - Flags: review?(emilio)
Comment on attachment 8936578 [details]
Bug 1207734 - Part 9. Make Inspector happy.

https://reviewboard.mozilla.org/r/207340/#review213174

I'm not sure this is right, this doesn't parse the properties at all, so inspector is going to do the wrong thing, right?
Attachment #8936578 - Flags: review?(emilio)
Comment on attachment 8936578 [details]
Bug 1207734 - Part 9. Make Inspector happy.

https://reviewboard.mozilla.org/r/207340/#review213174

Without this change, we will hit the assertion at [1](for sure)

And, no, it acutally works correctly. I can set and get the prop value of individual transform correctly, all I need to do is to bypass that assertion. 

[1] https://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#11835
Comment on attachment 8936577 [details]
Bug 1207734 - Part 8.b. (stylo) Update alias names for rotate/translate/scale.

https://reviewboard.mozilla.org/r/207338/#review213164

> Can you elaborate on why the alias? Are they in any compat spec?
> 
> Does this have any webcompat implications?
> 
> I'd prefer not supporting the webkit prefix if we don't need to.

Will remove these two patches
Do we also know why Blink implemented this but never shipped this?
Flags: needinfo?(cku)
It seems like the animations in this test case are not running on the compositor. Let me try to work out a list of the places that need to be fixed.
I am curious how the spec defines animations for these properties, especially transitions.   For example;

* changes scale value to the same value of transform

  element.style = 'transition: scale 1s; transform: scale(2);';

  element.style.scale = '2';

This case triggers a transition for scale, I think, and users want to see a transition from scale(2) to scale(4)?.  I guess this case does not work with the current patches.

Another test case that does not work I can think of

* changes scale value with static translate value

  element.style = 'transition: scale 1s; translate: 100px';

  element.style.scale = '2';

Anyway, I'd highly recommend to write various animation test cases first.
I had a look at what we need to support compositor animations and I think the main part we need to change is the interaction between EffectCompositor and nsDisplayList.

The simplest thing is probably to not change the compositor but to just send a suitably sorted list of transform animations to the compositor.
  
So we could either change the set of animations returned by EffectCompositor / Animation or we could handle it all in nsDisplayList.cpp
  
I think the simplest thing would be to do the grouping and sorting in nsDisplayList although that will probably be less efficient since we'll do the loop in KeyframeEffectReadOnly::GetEffectiveAnimationOfProperty more times than are strictly needed. That's probably ok though, and we can optimize that later if needed.
  
We need to bear in mind that we can have, for example, two animations that animation *both* translate and transform, for example. In that case, we'll want to sort the translate components of both animations before the transform components.

I think one way to do this is to handle it all in AddAnimationsForProperty (in nsDiplayList.cpp).

Where we call EffectCompositor::GetAnimationsForCompositor, we'll want to call it, then iterate over the result, four times for translate, rotate, scale, and transform, and in that order.

Apart from that we'll also need to change at least these places:

dom/animation/EffectCompositor.cpp
  FindAnimationsForCompositor
   - Needs to check for the new properties when synchronizing with geometric properties
     (Needs tests for this too)
   
dom/animation/KeyframeEffectReadOnly.cpp
  CanThrottle
   - Needs to check for the new properties
  ShouldBlockAsyncTransformAnimations
   - Likewise
  MaybeUpdateFrameForCompositor()
   - Likewise
  ContainsAnimatedScale
   - Likewise
  UpdateEffectSet
   - Likewise
   
dom/animation/Animation.cpp
  ShouldBeSynchronizedWithMainThread
   - Needs to check for the new properties

layout/painting/nsDisplayList.cpp
  SetAnimatable
   - Needs to check for the new properties
  nsDisplayTransform::MayBeAnimated
   - Likewise
   
layout/base/nsLayoutUtils.cpp
  ComputeSuitableScaleForAnimation
   - Needs to look for both scale and transform animations
(In reply to Hiroyuki Ikezoe (:hiro) from comment #83)
> I am curious how the spec defines animations for these properties,
> especially transitions.   For example;
> 
> * changes scale value to the same value of transform
> 
>   element.style = 'transition: scale 1s; transform: scale(2);';
> 
>   element.style.scale = '2';
> 
> This case triggers a transition for scale, I think, and users want to see a
> transition from scale(2) to scale(4)?.

Looking into this case I notice that the spec says:

> The scale property accepts 1-3 values, each specifying a scale along one axis, in order X, Y, then Z. Unspecified scales default to 1.

As in, 'scale: 2' actually is the same as 'scale: 2 1' NOT 'scale: 2 2'. That seems inconsistent with 'scale(2)' so we should clarify if that is what the spec intends.

Also, as hiro points out, transitions don't appear to work. (I haven't checked but I guess we're failing to get the correct before-change and after-change scale values.)

Once they are implemented we'll need to make sure that 'none' -> 2 interpolates as '1 1' to '2 2'
Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review213318

::: layout/style/nsStyleStruct.cpp:4025
(Diff revision 1)
> +  // Combine transform properties by the following sequence:
> +  //   translate > rotate > scale > transform
> +  const RefPtr<nsCSSValueSharedList> shareLists[] = { mSpecifiedTranslate,
> +                                                      mSpecifiedRotate,
> +                                                      mSpecifiedScale,
> +                                                      mSpecifiedTransform};

Nit: Space before }

::: layout/style/nsStyleStruct.cpp:4028
(Diff revision 1)
> +                                                      mSpecifiedRotate,
> +                                                      mSpecifiedScale,
> +                                                      mSpecifiedTransform};
> +
> +  nsTArray<nsCSSValueList*> valueLists;
> +  for (uint8_t i = 0; i < sizeof(shareLists) / sizeof(shareLists[0]); i++) {

Does ArrayLength not work here? Or, better still, a range-based for loop?
(In reply to Brian Birtles (:birtles) from comment #85)
> As in, 'scale: 2' actually is the same as 'scale: 2 1' NOT 'scale: 2 2'.
> That seems inconsistent with 'scale(2)' so we should clarify if that is what
> the spec intends.

Started following up this issue: https://github.com/w3c/csswg-drafts/issues/856#issuecomment-351547064

I also noticed there's some discussion about the rotate property syntax:

  https://github.com/w3c/csswg-drafts/issues/1269
I'm not sure if I should go ahead and review these patches any further. It seems like it might be better to wait until these patches support transitions first since that might require revisions to some of them.
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213120

> Should we pref them off by default?

I agree, we should.

> I think this value is for old system

You may need to add some case somewhere in StyleAnimationValue to avoid an assertion about not handling this property's animation in the old style system.  Can you make it eStyleAnimType_None, or would that prevent animation in Servo too?
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213170

> Hmm, I'm not sure this should use `CSS_PROPERTY_PARSE_FUNCTION`, should it?
> 
> Let's cam have a look, since he's more familiar with Gecko's CSS parser.

As you mention in comment 79, this means that when Stylo is disabled, we'll get into nsCSSParser::ParsePropertyByFunction and assert that we got an unexpected property.  If we don't want to implement support for these properties in the old style system, we don't have a great way to make the property completely invisible (including e.g. on getComputedStyle objects).

I guess it's probably fine to just make ParsePropertyByFunction not assert and just return false, and live with the fact that we'd expose the property on getComputedStyle objects.  As long as we put these properties behind a pref and don't enable the pref by default before we remove the old style system.
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213370
Attachment #8935261 - Flags: review?(cam) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #81)
> Do we also know why Blink implemented this but never shipped this?

There are no implementation issues blocking Blink shipping.

I didn't send a Blink Intent to Ship as
- The properties are defined in Transforms Level 2, and Transforms Level 1 is still a Working Draft.
- There wasn't a second implementation.

Transforms Level 1 is soon moving to CR.

We started a polyfill for web animation of individual transform properties and motion path: https://github.com/Motion-Path-Polyfill/motion-path-js with examples tested in Firefox https://ewilligers.github.io/motion-path-polyfill-examples/  This was an intern project, we don't have engineering resources assigned to polish this. The second implementation being a native UA implementation is much preferred.
There's something that bothers me to some extent, which is that these properties introduce some really weird behavior.

In particular, you can no longer set transform: none and expect the element to not be transformed... I wonder if `transform` should become some sort of shorthand property for these three...
(In reply to Brian Birtles (:birtles) from comment #87)
> (In reply to Brian Birtles (:birtles) from comment #85)
> > As in, 'scale: 2' actually is the same as 'scale: 2 1' NOT 'scale: 2 2'.
> > That seems inconsistent with 'scale(2)' so we should clarify if that is what
> > the spec intends.
> 
> Started following up this issue:
> https://github.com/w3c/csswg-drafts/issues/856#issuecomment-351547064
> 
> I also noticed there's some discussion about the rotate property syntax:
> 
>   https://github.com/w3c/csswg-drafts/issues/1269
thThat's why I rewite Animation trait for individual transform rather than reuse the code of TransformFunction for interop in Part 6.b. The behavior of unspecified value is totally different between individual transform and transform function in 'transform' property.
(In reply to Brian Birtles (:birtles) from comment #82)
> Created attachment 8936659 [details]
> Test case for compositor animations
> 
> It seems like the animations in this test case are not running on the
> compositor. Let me try to work out a list of the places that need to be
> fixed.
Hi Brian,
May I know how to make sure whether an animation is running on the compositor?
Flags: needinfo?(cku) → needinfo?(bbirtles)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #95)
> (In reply to Brian Birtles (:birtles) from comment #82)
> > Created attachment 8936659 [details]
> > Test case for compositor animations
> > 
> > It seems like the animations in this test case are not running on the
> > compositor. Let me try to work out a list of the places that need to be
> > fixed.
> Hi Brian,
> May I know how to make sure whether an animation is running on the
> compositor?

You can easily check it on animation inspector in devtools panel.  If the animation is running on the compositor, you will see a lightning bolt icon.
Flags: needinfo?(bbirtles)
Thanks Eric. I've filed [1] for making the values for `scale` consistent. Any comments appreciated!

[1] https://github.com/w3c/csswg-drafts/issues/2109
(In reply to Hiroyuki Ikezoe (:hiro) from comment #96)
> > Hi Brian,
> > May I know how to make sure whether an animation is running on the
> > compositor?
> 
> You can easily check it on animation inspector in devtools panel.  If the
> animation is running on the compositor, you will see a lightning bolt icon.

Got it, thanks, hiro
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213170

> As you mention in comment 79, this means that when Stylo is disabled, we'll get into nsCSSParser::ParsePropertyByFunction and assert that we got an unexpected property.  If we don't want to implement support for these properties in the old style system, we don't have a great way to make the property completely invisible (including e.g. on getComputedStyle objects).
> 
> I guess it's probably fine to just make ParsePropertyByFunction not assert and just return false, and live with the fact that we'd expose the property on getComputedStyle objects.  As long as we put these properties behind a pref and don't enable the pref by default before we remove the old style system.

Instead of remove that assertion, we can modify it from
  MOZ_ASSERT(false, "should not be called");
to
  MOZ_ASSERT(nsLayoutUtils::StyloEnabled(), "should not be called in the old"
                                            "style system.");
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213170

> Instead of remove that assertion, we can modify it from
>   MOZ_ASSERT(false, "should not be called");
> to
>   MOZ_ASSERT(nsLayoutUtils::StyloEnabled(), "should not be called in the old"
>                                             "style system.");

I'd prefer to list the properties that we are deciding not to implement in the old style system explicitly, so like:

  case eCSSPropertyID_translate:
  case eCSSPropertyID_rotate:
  case eCSSPropertyID_scale:
    // These properties aren't implemented in the old style system.
    return false;
  default:
    MOZ_ASSERT(false, ...);

just to avoid weakening the existing assertion for current cases where we're still using the old style system (in chrome windows, and any sites in the stylo blocklist).
Comment on attachment 8935261 [details]
Bug 1207734 - Part 3. Add rotate/translate/scale properties into nsCSSPropList.

https://reviewboard.mozilla.org/r/206154/#review213452

I don't think you need my review on this -- emilio and heycam's review seems sufficient.

The only feedback I have is that if we decide to split supporting animation on the compositor to a separate bug then we should probably drop CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR from this patch.
Attachment #8935261 - Flags: review?(bbirtles)
Comment on attachment 8935263 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/206158/#review213126

> This should be `Option<(Number, Number, Number)>, Angle`, and that way you can remove the `panic!`s.

If I do this change, I need to impl the following trait:
  ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero

Option<()> is not handled in those marcos(I guess)
Attached file Transition test case
This test case animates for me on Chrome Canary (I guess I have enabled experimental web platform features) but with these patches:

* It animated for me once
* Every time since it doesn't animate
* Once the content process crashed

Also, getComputedStyle(elem).scale doesn't seem to produce any output. I guess we need to modify nsComputedDOMStyle.

See: https://developer.mozilla.org/en-US/docs/Mozilla/Adding_a_new_style_property#Computed_Style
Comment on attachment 8935268 [details]
Bug 1207734 - Part 6.a. Update LayerAnimationInfo list according to the new added transform properties.

https://reviewboard.mozilla.org/r/206168/#review213466
Attachment #8935268 - Flags: review?(bbirtles) → review+
(In reply to C.J. Ku[:cjku](UTC+8) from comment #102)
> Comment on attachment 8935263 [details]
> Bug 1207734 - Part 3.a. (stylo) Implement rotate property styling.
> 
> https://reviewboard.mozilla.org/r/206158/#review213126
> 
> > This should be `Option<(Number, Number, Number)>, Angle`, and that way you can remove the `panic!`s.
> 
> If I do this change, I need to impl the following trait:
>   ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero
> 
> Option<()> is not handled in those marcos(I guess)

You should implement those like:

impl<T, U, D> ToComputedValue for (T, U, D)
where
    T: ToComputedValue,
    U: ToComputedValue,
    D: ToComputedValue,
{
    type ComputedValue = (T::ComputedValue, U::ComputedValue, D::ComputedValue);

    fn to_computed_value(&self) -> Self::ComputedValue {
        (self.0.to_computed_value(), self.1.to_computed_value(), self.2.to_computed_value())
    }
}
Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review213480

Clearing review request for now, since we at least need to use animate_multiplicative_factor. It would be helpful to write tests for this too.

If you find that supporting animation is too much work, feel free to split it off into a separate bug.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2364
(Diff revision 2)
> +        Ok(Scale::Specified(from.0.animate(&to.0, procedure)?,
> +                            Some(from.1.animate(&to.1, procedure)?),
> +                            Some(from.2.animate(&to.2, procedure)?)))

I think this needs to use animate_multiplicative_factor.

But we should really add tests for this to confirm.

A good place to start for that would be:

  testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js
Attachment #8935269 - Flags: review?(bbirtles)
Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review213488

Clearing review request for now since it seems like there is a bit of review feedback that needs to be addressed here.

(I also wonder if it makes sense to just invalidate the combined transform -- e.g. make it null -- and then lazily generate it when needed.)
Attachment #8936571 - Flags: review?(bbirtles)
Comment on attachment 8936572 [details]
Bug 1207734 - Part 4.b. (stylo) Update nsStyleDisplay::mCombinedTransform once transform value changed.

https://reviewboard.mozilla.org/r/207328/#review213490

Clearing review request now since it seems at least Emilio's comment needs to be addressed.
Attachment #8936572 - Flags: review?(bbirtles)
Comment on attachment 8936573 [details]
Bug 1207734 - Part 4.b. Use the final combined transform in the nsDisplayTransform.

https://reviewboard.mozilla.org/r/207330/#review213492

This is probably fine (but in reviewing this I start to wonder if we'll need to keep these separate further down the pipeline so we can send independent base values to the compositor for additive animation).
Attachment #8936573 - Flags: review?(bbirtles) → review+
Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review213142

> There's no need for this to addref everything, `const RefPtr<nsCSSValueSharedList>&`?

arrays of references is not legal in c++ standard, so, we will declare it as const RefPtr<nsCSSValueSharedList>* instead.
Comment on attachment 8936575 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

https://reviewboard.mozilla.org/r/207334/#review213846

This probably needs a reftest too for `reset` values, and ideally also the other wide keywords, since reset was broken with the other patches I believe.

r=me with one, or with another patch that adds one.

::: layout/reftests/w3c-css/submitted/transforms/individual-transform-1-ref.html:42
(Diff revision 1)
> +      .row_2 {
> +        top: 250px;
> +      }
> +      .scale_2{
> +        left:100px;
> +        width:50px;

nit: Super minor, but let's use whitespace consistently? That is, always a space after the colon, or never (though always is the most common style I guess?)

::: layout/reftests/w3c-css/submitted/transforms/individual-transform-2d.html:7
(Diff revision 1)
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <title>Individual transform: combine individual transform properties</title>
> +    <link rel="author" title="CJ Ku" href="mailto:cku@mozilla.com">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">

I think these need `<link rel="match">` tags?

::: layout/reftests/w3c-css/submitted/transforms/individual-transform-2e.html:3
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +  <head>

nit: I personally see better what's going on if you remove superfluous markup, like `<html>` / `<head>` / `<body>`, though if you disagree it's not a big deal.

::: layout/reftests/w3c-css/submitted/transforms/individual-transform-2e.html:8
(Diff revision 1)
> +  <head>
> +    <meta charset="utf-8">
> +    <title>Individual transform: combine individual transform properties</title>
> +    <link rel="author" title="CJ Ku" href="mailto:cku@mozilla.com">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> +    <style type="text/css">

nit: not need for `type="text/css"`, here nor anywhere else.
Attachment #8936575 - Flags: review?(emilio) → review+
Comment on attachment 8935263 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/206158/#review213850

::: servo/components/style/values/specified/transform.rs:529
(Diff revision 2)
> +
> +        if let Some(rx) = input.try(|i| Number::parse(context, i)).ok() {
> +            // <number>{3} <angle>
> +            let ry = Number::parse(context, input)?;
> +            let rz = Number::parse(context, input)?;
> +            let angle = specified::Angle::parse_with_unitless(context, input)?;

This needs a link to the spec, why do we allow unitless angles here? I don't see anything like that on the spec.
Comment on attachment 8936574 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

https://reviewboard.mozilla.org/r/207332/#review213848

Clearing because I'm not convinced it's correct.

::: layout/style/test/property_database.js:6300
(Diff revision 1)
> +  gCSSProperties["rotate"] = {
> +    domProp: "rotate",
> +    inherited: false,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "45deg", "45grad", "0",  "72rad", "0.25turn", ".57rad",

Looking at the spec, it looks to me like `0` shouldn't be a valid value? I left a comment on another commit regarding this. If we allow unitless angle here, needs a comment on the code.

::: layout/style/test/property_database.js:6310
(Diff revision 1)
> +  gCSSProperties["translate"] = {
> +    domProp: "translate",
> +    inherited: false,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "-4px", "3px", "4em", "50%", "4px 5px 6px",

Add tests for `calc`? Here and the others.
Attachment #8936574 - Flags: review?(emilio)
Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review213852

Clearing because there seems to be a large amount of animation issues, and the patch may need to change significantly. Birtles is probably a better reviewer than me on this, but happy to take a look too.
Attachment #8935269 - Flags: review?(emilio)
Yeah, we might want to split making these properties animatable in a later bug.  Brian suggested to make these property non-compositor runnable properties in comment 101, but even so I suspect there are bunch of annoying test cases that we need to care about, e.g. transform and scale animations on an element. 

So, I'd suggest that we make these properties non-animatable in this bug and make sure transform animations run on the compositor with various *static* values for these properties properly, then make them animatable in a later bug.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #113)
> Looking at the spec, it looks to me like `0` shouldn't be a valid value? I
> left a comment on another commit regarding this. If we allow unitless angle
> here, needs a comment on the code.

I suspect the spec is wrong here since I believe the rotate() function etc. explicitly allow this and we should be consistent with that. So, I think we should file a spec issue to check/clarify this.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #115)
> So, I'd suggest that we make these properties non-animatable in this bug and
> make sure transform animations run on the compositor with various *static*
> values for these properties properly, then make them animatable in a later
> bug.

Yes, I agree.
(I am a bit concerned that the approach we have taken here might now allow animating correctly--i.e. we might be merging the transforms too soon--but I guess we can fix that in the animation bug.)
(In reply to Brian Birtles (:birtles) from comment #116)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #113)
> > Looking at the spec, it looks to me like `0` shouldn't be a valid value? I
> > left a comment on another commit regarding this. If we allow unitless angle
> > here, needs a comment on the code.
> 
> I suspect the spec is wrong here since I believe the rotate() function etc.
> explicitly allow this and we should be consistent with that. So, I think we
> should file a spec issue to check/clarify this.

Yeah, those are supposed to be exceptions rather than the rule, to avoid web compat issues, but I guess it makes sense to be consistent with transform.

(In reply to Brian Birtles (:birtles) from comment #118)
> (I am a bit concerned that the approach we have taken here might now allow
> animating correctly--i.e. we might be merging the transforms too soon--but I
> guess we can fix that in the animation bug.)

If we can remove that merging code from style I'd feel much more comfortable with the patches.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #119)
> Yeah, those are supposed to be exceptions rather than the rule, to avoid web
> compat issues, but I guess it makes sense to be consistent with transform.

I filed https://github.com/w3c/csswg-drafts/issues/2116... I have mixed feelings about it, so whatever the WG decides would be fine with me.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #120)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #119)
> > Yeah, those are supposed to be exceptions rather than the rule, to avoid web
> > compat issues, but I guess it makes sense to be consistent with transform.
> 
> I filed https://github.com/w3c/csswg-drafts/issues/2116... I have mixed
> feelings about it, so whatever the WG decides would be fine with me.

Not that my personal feeling matters much anyway, though :P

Testing whatever the resolution for that is on WPT would be nice.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #119)
> (In reply to Brian Birtles (:birtles) from comment #118)
> > (I am a bit concerned that the approach we have taken here might now allow
> > animating correctly--i.e. we might be merging the transforms too soon--but I
> > guess we can fix that in the animation bug.)
> 
> If we can remove that merging code from style I'd feel much more comfortable
> with the patches.

In light of that, it might make sense to include animation in this patch.

For example, if we discover that we can't support animation properly so long as we're merging transforms in style, then maybe much of the work in this patch would need to be redone?

For what it's worth, my particular concern is how additive animations of 'scale', 'translate' etc. will work, particularly when we send them to the compositor. I fear they need to be treated as separate channels there (i.e. we can't just send them as a bunch of otherwise transform animations).

It also feels a little unfortunate to have to remember to call "UpdateTransform". If we could generate the combined transform on demand somewhere later in the pipeline that would seem to be simpler, but then I guess that runs the risk of one of the many users of transforms forgetting to include the individual properties.
(In reply to Brian Birtles (:birtles) from comment #122)
> For what it's worth, my particular concern is how additive animations of
> 'scale', 'translate' etc. will work, particularly when we send them to the
> compositor. I fear they need to be treated as separate channels there (i.e.
> we can't just send them as a bunch of otherwise transform animations).

If translate, scale, rotate, transform are animated with different timing functions, then they would need to be separate channels (or not animated on the compositor). (In Chromium, we currently animate on the main thread if two or more of the properties are in effect for an element.)
Yeah, the approach I suggested in comment 84 still sends them to the compositor as independent transform animations which will be interpolated separately and then combined. However, once you have additive scale and additive translate and additive rotate animations (or perhaps even just those without missing to/from keyframes), I think you need to get separate base values to send to the compositor, then interpolate them independently, add their stacks independently, and then combine them, which I think means you need to differentiate between the sources of the different transform animations on the compositor too.
Comment on attachment 8935265 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/206162/#review213136

> This needs to reset the property, not do nothing.

This property is reset by #680(3 lines above) alreay
  unsafe { self.gecko.${gecko_ffi_name}.clear() };
Blocks: 1425837
Comment on attachment 8935269 [details]
Bug 1207734 - Part 8. (stylo) Implement Animate trait for individual transform.

https://reviewboard.mozilla.org/r/206170/#review213480

> I think this needs to use animate_multiplicative_factor.
> 
> But we should really add tests for this to confirm.
> 
> A good place to start for that would be:
> 
>   testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js

Filed bug 1425837 as a follow-up.
Attachment #8935268 - Attachment is obsolete: true
Attachment #8936576 - Attachment is obsolete: true
Attachment #8936577 - Attachment is obsolete: true
Attachment #8936578 - Attachment is obsolete: true
Attachment #8936572 - Attachment is obsolete: true
Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review214454

::: layout/style/nsStyleStruct.cpp:4027
(Diff revision 2)
> +nsStyleDisplay::GetCombinedTransform() const
> +{
> +  // Return mSpecifiedTransform directly if there is no individual transform
> +  // at all.
> +  if (!mSpecifiedTranslate && !mSpecifiedRotate && !mSpecifiedScale) {
> +    return mSpecifiedTransform;

return do_AddRef(mSpecifiedTransform);
Attachment #8935263 - Attachment is obsolete: true
Attachment #8935263 - Flags: review?(emilio)
Attachment #8935264 - Attachment is obsolete: true
Attachment #8935264 - Flags: review?(emilio)
Attachment #8935265 - Attachment is obsolete: true
Attachment #8935265 - Flags: review?(emilio)
Attachment #8935269 - Attachment is obsolete: true
Attachment #8935269 - Flags: review?(emilio)
Attachment #8935269 - Flags: review?(bbirtles)
Attachment #8936574 - Attachment is obsolete: true
Attachment #8936574 - Flags: review?(emilio)
Attachment #8936575 - Attachment is obsolete: true
Attachment #8937972 - Attachment is obsolete: true
Attachment #8937972 - Flags: review?(emilio)
Attachment #8937973 - Attachment is obsolete: true
Attachment #8937973 - Flags: review?(emilio)
Attachment #8936571 - Flags: review?(bbirtles)
Attachment #8938035 - Flags: review?(emilio)
Attachment #8938036 - Flags: review?(emilio)
Attachment #8938037 - Flags: review?(emilio)
Attachment #8938038 - Flags: review?(emilio)
Attachment #8938039 - Flags: review?(emilio)
Attachment #8938041 - Flags: review?(emilio)
Attachment #8938042 - Flags: review?(emilio)
Hi Emilio,
Because of an incorrect push, some review history disappear. Sorry for that. But I did fixed issues that you posted, please have a look again.
Attachment #8938040 - Flags: review?(emilio)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #157)
> Hi Emilio,
> Because of an incorrect push, some review history disappear. Sorry for that.
> But I did fixed issues that you posted, please have a look again.
Update the status before those patched been removed
 Bug 1207734 - Part 7.a. - (r? cleared) The main concern is to replace Specified(Option<Number>, Option<Number>, Option<Number>, Angle) by Specified(Option<(Number, Number, Number)>, Angle) in comment 60
 Bug 1207734 - Part 7.b/c. - (r? cleared)
 Bug 1207734 - Part 9.a. - (r? cleared) You have some concern at  Comment 113
 Bug 1207734 - Part 9.b. - r+ already
 bug 1207734 - Part 9.c./d. - new added
Attachment #8938038 - Flags: review?(emilio) → review?(bbirtles)
Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review214530

Looks good, but could do another look once the comments are addressed.

::: servo/components/style/properties/gecko.mako.rs:546
(Diff revision 1)
>      pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
>          convert_nscolor_to_rgba(${get_gecko_property(gecko_ffi_name)})
>      }
>  </%def>
>  
> +<%def name="impl_rotate(ident, gecko_ffi_name)">

Given this is only used in one place, I'd avoid the mako function, and I'd just implement the functions without all the mako stuff.

You can do that adding `rotate`, etc to `skip_box_longhands` (terrible name, I know), and just defining `set_rotate`, `copy_rotate_from`, etc.

Also, there's no need for the `allow()`s, nor anything like that, that's needed for transform since we share that code for `-moz-window-transform`.

::: servo/components/style/values/animated/mod.rs:455
(Diff revision 1)
> +    U: ToAnimatedZero,
> +    V: ToAnimatedZero,
> +{
> +    #[inline]
> +    fn to_animated_zero(&self) -> Result<Self, ()> {
> +        Ok((self.0.to_animated_zero()?, self.1.to_animated_zero()?,

nit: Use block indentation:

```
Ok((
    self.0.to_animated_zero()?,
    self.1.to_animated_zero()?,
    self.2.to_animated_zero()?,
))
```

Same below.

::: servo/components/style/values/computed/mod.rs:310
(Diff revision 1)
>  
> +impl<A, B, C> ToComputedValue for (A, B, C)
> +    where A: ToComputedValue, B: ToComputedValue, C: ToComputedValue,
> +{
> +    type ComputedValue = (
> +        <A as ToComputedValue>::ComputedValue,

I think you don't need the `<A as ToComputedValue>::ComputedValue`, and only `A::ComputedValue` should be enough.

::: servo/components/style/values/computed/mod.rs:317
(Diff revision 1)
> +        <C as ToComputedValue>::ComputedValue,
> +    );
> +
> +    #[inline]
> +    fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
> +        (self.0.to_computed_value(context), self.1.to_computed_value(context),

nit: indentation.

::: servo/components/style/values/generics/transform.rs:764
(Diff revision 1)
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
> +/// A value of the `Rotate` property
> +///
> +/// <https://drafts.csswg.org/css-transforms-2/#individual-transforms>
> +pub enum Rotate<Number, Angle> {

nit: Maybe `Rotation` is a more descriptive type name?

::: servo/components/style/values/specified/transform.rs:531
(Diff revision 1)
> +            // <number>{3} <angle>
> +            let ry = Number::parse(context, input)?;
> +            let rz = Number::parse(context, input)?;
> +            let angle = specified::Angle::parse(context, input)?;
> +            return Ok(GenericRotate::Specified(Some((rx, ry, rz)), angle));
> +        } else {

nit: No `else` after return.

::: servo/components/style/values/specified/transform.rs:533
(Diff revision 1)
> +            let rz = Number::parse(context, input)?;
> +            let angle = specified::Angle::parse(context, input)?;
> +            return Ok(GenericRotate::Specified(Some((rx, ry, rz)), angle));
> +        } else {
> +            // <angle>
> +            let angle = specified::Angle::parse_with_unitless(context, input)?;

No unitless angle, per https://github.com/w3c/csswg-drafts/issues/2116.
Attachment #8938035 - Flags: review?(emilio)
Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review214532

::: servo/components/style/properties/gecko.mako.rs:604
(Diff revision 1)
>              }
>          }
>      }
>  </%def>
>  
> +<%def name="impl_translate(ident, gecko_ffi_name)">

Same re. the mako stuff than for `rotate`.

::: servo/components/style/values/generics/transform.rs:776
(Diff revision 1)
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
> +/// A value of the `Translate` property
> +///
> +/// <https://drafts.csswg.org/css-transforms-2/#individual-transforms>
> +pub enum Translate<LengthOrPercentage, Length> {

nit: Maybe `Translation`?

::: servo/components/style/values/specified/transform.rs:15
(Diff revision 1)
>  use style_traits::{ParseError, StyleParseErrorKind};
>  use values::computed::{Context, LengthOrPercentage as ComputedLengthOrPercentage};
>  use values::computed::{Percentage as ComputedPercentage, ToComputedValue};
>  use values::computed::transform::TimingFunction as ComputedTimingFunction;
>  use values::generics::transform::{Matrix3D, Transform as GenericTransform,
> -    Rotate as GenericRotate};
> +    Rotate as GenericRotate, Translate as GenericTranslate};

nit: Use multiple `use` lines instead of multiline imports.

::: servo/components/style/values/specified/transform.rs:551
(Diff revision 1)
> +    ) -> Result<Self, ParseError<'i>> {
> +        if input.try(|i| i.expect_ident_matching("none")).is_ok() {
> +            return Ok(GenericTranslate::None);
> +        }
> +        let tx = specified::LengthOrPercentage::parse(context, input)?;
> +        if let Some(ty) = input.try(|i| specified::LengthOrPercentage::parse(context, i)).ok() {

Maybe just `if let Ok(..) = input.try(..) {` instead of `if let Some(..) = input.try(..).ok() {`?

::: servo/components/style/values/specified/transform.rs:555
(Diff revision 1)
> +        let tx = specified::LengthOrPercentage::parse(context, input)?;
> +        if let Some(ty) = input.try(|i| specified::LengthOrPercentage::parse(context, i)).ok() {
> +            if let Some(tz) = input.try(|i| specified::Length::parse(context, i)).ok() {
> +                // <number>{3}
> +                return Ok(GenericTranslate::Specified(tx, Some(ty), Some(tz)));
> +            } else {

nit: no else after `return`.
Attachment #8938036 - Flags: review?(emilio)
Comment on attachment 8938037 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/208766/#review214536

Same as before, I'll take a look at it once the mako stuff is gone, but looks pretty good!
Attachment #8938037 - Flags: review?(emilio)
Comment on attachment 8938039 [details]
Bug 1207734 - Part 9.a. (testing) Update gCSSProperties for the properties of individual transforms.

https://reviewboard.mozilla.org/r/208770/#review214538

r=me
Attachment #8938039 - Flags: review?(emilio) → review+
Comment on attachment 8938040 [details]
Bug 1207734 - Part 9.b. (testing) Add basic reftests for individual transform.

https://reviewboard.mozilla.org/r/208772/#review214542

::: layout/reftests/w3c-css/submitted/transforms/reftest.list:11
(Diff revision 1)
>  == perspective-zero-2.html perspective-zero-2-ref.html
> +
> +default-preferences pref(layout.css.individual-transform.enabled,true)
> +# skip stylo-vs-gecko comparison since we support individual transform on new
> +# style system only.
> +skip-if(!stylo||styloVsGecko) == individual-transform-1.html individual-transform-1-ref.html

Maybe just `fails-if` instead of `skip-if`? That way we can notice if something goes pretty wrong :)
Attachment #8938040 - Flags: review?(emilio) → review+
Comment on attachment 8938041 [details]
bug 1207734 - Part 9.c. (testing) Clone reftests from w3c-css to web-platform-test

https://reviewboard.mozilla.org/r/208774/#review214544

Looks fine, but seems unfortunate to have duplicated tests. Why not just landing the WPT ones? There are some known issues about our WPT stuff, like non-fatal assertions and such, but I think it's not a big deal in this case, since the new style system doesn't use those anyway.
Attachment #8938041 - Flags: review?(emilio) → review+
Comment on attachment 8938042 [details]
Bug 1207734 - Part 9.d. (testing) Update manifest.json for new added wp-tests.

https://reviewboard.mozilla.org/r/208776/#review214546

Shouldn't need to ask for review for this IMO :)
Attachment #8938042 - Flags: review?(emilio) → review+
Attached file Scale tests
These three animations should look the same
Attached file Base value test
This animation should not jump
Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

https://reviewboard.mozilla.org/r/208768/#review214672

Clearing review for now because we should get attachment 8938223 [details] and attachment 8938224 [details] to pass first.

Can you also please add wpt tests for these properties to:

  testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js
  testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js

that cover these cases?

(If you make the changes to a fork of web-platform-tests repository then you can use ./wpt serve to run them as a local web server where you can also run them against Chrome. Unfortunately Chrome doesn't appear to support `composite: 'accumulate'` yet so you won't be able to test that there.)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2398
(Diff revision 1)
> +        Ok(Scale::Specified(animate_multiplicative_factor(from.0, to.0, procedure)?,
> +                            Some(animate_multiplicative_factor(from.1, to.1, procedure)?),
> +                            Some(animate_multiplicative_factor(from.2, to.2, procedure)?)))

I don't think this is right. I think this might be the right behavior when |procedure| is Procedure::Interpolate or Procedure::Accumulate, but not Procedure::Add.

See attachment 8938223 [details].

If you _add_ `scale: 2 2` with `scale: 2 2` the visual result, for consistency with 'transform', should be the same as `scale(2) scale(2)`. It's only when you _accumulate_ that it should be `scale: 3 3`. (And we should add a test for that too.)

e.g. the following should not jump

anim.animate({ scale: [ '1 1', '2 2' ] }, { duration: 1000, iterations: 2, iterationComposite: 'accumulate' })
Attachment #8938038 - Flags: review?(bbirtles)
Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

This looks mostly fine, but clearing review now because if we are *not* supposed to resolve percentages then I guess this will change a bit.

::: layout/style/nsComputedDOMStyle.cpp:1746
(Diff revision 2)
> +static already_AddRefed<CSSValue>
> +ReadIndividualTransformValue(nsCSSValueSharedList* aList,
> +                             const std::function<void(const nsCSSValue::Array*,
> +                                                      nsString&)>& aCallback)

(This indentation seems a little bit odd, but if that's what ./mach clang-format does it must be ok.)

::: layout/style/nsComputedDOMStyle.cpp:1773
(Diff revision 2)
> +already_AddRefed<CSSValue>
> +nsComputedDOMStyle::DoGetTranslate()
> +{
> +  typedef nsStyleTransformMatrix::TransformReferenceBox TransformReferenceBox;
> +
> +  RefPtr<nsComputedDOMStyle> self(this);

(It's a pity we need this, but our static analysis insists on it.)

::: layout/style/nsComputedDOMStyle.cpp:1785
(Diff revision 2)
> +
> +      switch (nsStyleTransformMatrix::TransformFunctionOf(aData)) {
> +        /* translate : <length-percentage> */
> +        case eCSSKeyword_translatex: {
> +          NS_PRECONDITION(aData->Count() == 2, "Invalid array!");
> +          float tx = ProcessTranslatePart(aData->Item(1),

Out of curiouslty, how does this work? This function is defined in the nsStyleTransformMatrix namespace. How come we don't need to add the namespace prefix here?

This is mostly just my curiosity but I'm also wondering if this only works because of a stray use/typedef in the unified build, although I can't find one.

::: layout/style/nsComputedDOMStyle.cpp:1792
(Diff revision 2)
> +          // According to the spec, unspecified translations default to 0px.
> +          aResult.AppendLiteral("px 0px 0px");

I think the "shortest form"[1] means we should return just the one value here.

Also, are we supposed to resolve percentages to pixels here? I thought normally we didn't? (Except for the exceptional values like width, height etc. whose used values we return.) e.g. see the transform-origin property[2]

But, then again, we are proposing to make the serialization of 'transform' resolve to a single matrix[3] so it sounds like we resolve percentages there.

Do you happen to know if this is defined/discussed anywhere? If not then as the spec stands, I think we are supposed to preserve percentages--but we should clarify that.

(For what it's worth, if we *don't* need to resolve percentages, then I suspect we can drop the 'self' capture from the lambda and no longer need to use std::function either since a lambda with no captures has a conversion operator to a concrete type.)

[1] https://github.com/w3c/csswg-drafts/issues/1564
[2] https://drafts.csswg.org/css-transforms/#transform-origin-property
[3] https://drafts.csswg.org/css-transforms-2/#serialization-of-the-computed-value

::: layout/style/nsComputedDOMStyle.cpp:1806
(Diff revision 2)
> +          aResult.AppendLiteral("px ");
> +          float ty = ProcessTranslatePart(aData->Item(2),
> +                                          contextIfGecko,
> +                                          self->mStyleContext->PresContext(),
> +                                          dummy,
> +                                          &refBox,
> +                                          &TransformReferenceBox::Height);
> +          aResult.AppendFloat(ty);
> +          // According to the spec, unspecified translations default to 0px.
> +          aResult.AppendLiteral("px 0px");
> +          break;

The "shortest form" here would also mean that if ty == 0, we don't serialize it. And we also don't add the final "0px".

::: layout/style/nsComputedDOMStyle.cpp:1829
(Diff revision 2)
> +          float ty = ProcessTranslatePart(aData->Item(2),
> +                                          contextIfGecko,
> +                                          self->mStyleContext->PresContext(),
> +                                          dummy,
> +                                          &refBox,
> +                                          &TransformReferenceBox::Height);
> +          aResult.AppendFloat(ty);
> +          aResult.AppendLiteral("px ");
> +          float tz = ProcessTranslatePart(aData->Item(3),
> +                                          contextIfGecko,
> +                                          self->mStyleContext->PresContext(),
> +                                          dummy,
> +                                          &refBox,
> +                                          nullptr);
> +          aResult.AppendFloat(tz);
> +          aResult.AppendLiteral("px");

Likewise here, we need to omit from the end any arguments that are 0.

::: layout/style/nsComputedDOMStyle.cpp:1864
(Diff revision 2)
> +        /* scale : <number> */
> +        case eCSSKeyword_scalex:
> +        NS_PRECONDITION(aData->Count() == 2, "Invalid array!");
> +          aResult.AppendFloat(aData->Item(1).GetFloatValue());
> +          // According to the spec, unspecified scales default to 1.
> +          aResult.AppendLiteral(" 1.0 1.0");

Once again, short form here means we should not append the 1.0 1.0.

(And if other browsers are appending that, we should make sure there are web-platform-tests covering it so they have a chance to update their implementations.)

::: layout/style/nsComputedDOMStyle.cpp:1873
(Diff revision 2)
> +        NS_PRECONDITION(aData->Count() == 3, "Invalid array!");
> +          aResult.AppendFloat(aData->Item(1).GetFloatValue());
> +          aResult.AppendLiteral(" ");
> +          aResult.AppendFloat(aData->Item(2).GetFloatValue());
> +          // According to the spec, unspecified scales default to 1.
> +          aResult.AppendLiteral(" 1.0");

Here too, plus we should omit the second argument if it is 1.0.

::: layout/style/nsComputedDOMStyle.cpp:1879
(Diff revision 2)
> +          aResult.AppendLiteral(" ");
> +          aResult.AppendFloat(aData->Item(2).GetFloatValue());
> +          aResult.AppendLiteral(" ");
> +          aResult.AppendFloat(aData->Item(3).GetFloatValue());

Likewise, we should omit the 3rd argument if it is 1.0, and omit both 2nd and 3rd arguments if they are 1.0.

::: layout/style/nsComputedDOMStyle.cpp:1900
(Diff revision 2)
> +          // According to the spec, if the axis is unspecified, it defaults to
> +          // '0 0 1'
> +          aResult.AppendLiteral("0 0 1 ");

Again, shortest form says we don't need this.
Attachment #8937970 - Flags: review?(bbirtles)
Comment on attachment 8937971 [details]
Bug 1207734 - Part 6. Fix an assertion from the early pref access checking.

https://reviewboard.mozilla.org/r/208698/#review214678
Attachment #8937971 - Flags: review?(wmccloskey) → review+
Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review214530

> nit: Maybe `Rotation` is a more descriptive type name?

Although it more descriptive, I think it's clearer to just match the keyword in spec.
Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

> Out of curiouslty, how does this work? This function is defined in the nsStyleTransformMatrix namespace. How come we don't need to add the namespace prefix here?
> 
> This is mostly just my curiosity but I'm also wondering if this only works because of a stray use/typedef in the unified build, although I can't find one.

that's because there is a typedef in the beginning of this function:
  typedef nsStyleTransformMatrix::TransformReferenceBox TransformReferenceBox;
Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

> I think the "shortest form"[1] means we should return just the one value here.
> 
> Also, are we supposed to resolve percentages to pixels here? I thought normally we didn't? (Except for the exceptional values like width, height etc. whose used values we return.) e.g. see the transform-origin property[2]
> 
> But, then again, we are proposing to make the serialization of 'transform' resolve to a single matrix[3] so it sounds like we resolve percentages there.
> 
> Do you happen to know if this is defined/discussed anywhere? If not then as the spec stands, I think we are supposed to preserve percentages--but we should clarify that.
> 
> (For what it's worth, if we *don't* need to resolve percentages, then I suspect we can drop the 'self' capture from the lambda and no longer need to use std::function either since a lambda with no captures has a conversion operator to a concrete type.)
> 
> [1] https://github.com/w3c/csswg-drafts/issues/1564
> [2] https://drafts.csswg.org/css-transforms/#transform-origin-property
> [3] https://drafts.csswg.org/css-transforms-2/#serialization-of-the-computed-value

No idea, issue filed:
https://github.com/w3c/csswg-drafts/issues/2124

Since both chromium and edge resolve percentages to pxs, I think we may keep current behavior unless we get a conclusion from w3c that we shouldn't. how do you think?
Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

> Once again, short form here means we should not append the 1.0 1.0.
> 
> (And if other browsers are appending that, we should make sure there are web-platform-tests covering it so they have a chance to update their implementations.)

chromiun returns shortest form whereas edge returns longest form. will create a test case for this
Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review214674

> chromiun returns shortest form whereas edge returns longest form. will create a test case for this

Create test cases in Part 9.f
Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

https://reviewboard.mozilla.org/r/208768/#review214672

Done in Part 9.f
Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

https://reviewboard.mozilla.org/r/208768/#review214672

attachment 8938223 [details] was fixed in this patch. attachment 8938224 [details] was fixed(temp workaround) in Part 4.c
Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review217118

This looks _way_ more consistent :).

r=me

::: servo/components/style/properties/gecko.mako.rs:3122
(Diff revision 2)
> +        }
> +
> +        let list = unsafe { (*self.gecko.${gecko_ffi_name}.to_safe().get()).mHead.as_ref() };
> +
> +        let mut transform = clone_transform_from_list(list);
> +        debug_assert!(transform.0.len() == 1);

nit: `debug_assert_eq!`.

::: servo/components/style/properties/longhand/box.mako.rs:417
(Diff revision 2)
>                            gecko_ffi_name="mSpecifiedTransform",
>                            flags="CREATES_STACKING_CONTEXT FIXPOS_CB",
>                            spec="https://drafts.csswg.org/css-transforms/#propdef-transform")}
>  
> +${helpers.predefined_type("rotate", "Rotate",
> +                          "generics::transform::Rotate(generics::transform::IndividualTransform::None)",

Maybe create a `fn none() -> Self` on rotate and call it, much like `Transform`? I think it reads a bit nicer.

::: servo/components/style/values/generics/transform.rs:762
(Diff revision 2)
>  }
> +
> +#[derive(ToComputedValue, Animate, ComputeSquaredDistance, ToAnimatedZero)]
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
> +/// A base component for each individual transform.
> +pub enum IndividualTransform<T> {

s/T/TransformOperation/?

Also maybe a comment that this is needed to get serialization right for the `None` variant instead of just using `Option` would be nice.

::: servo/components/style/values/specified/transform.rs:537
(Diff revision 2)
> +            return Ok(GenericRotate(IndividualTransform::Specified(GenericTransformOperation::Rotate3D(rx, ry, rz, angle))));
> +        }
> +
> +        // 'rotate: <angle>'
> +        let angle = specified::Angle::parse(context, input)?;
> +        Ok(GenericRotate(IndividualTransform::Specified(GenericTransformOperation::Rotate(angle))))

I think you can avoid using `GenericRotate` and just use `Rotate`? `GenericTransformOperation` is needed due to a long-standing rust issue with enums and associated types.
Attachment #8938035 - Flags: review?(emilio) → review+
Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review217126

r=me, with the same comments as the previous patch, and this extra comment addressed in both.

::: servo/components/style/values/specified/transform.rs:555
(Diff revision 2)
> +        if input.try(|i| i.expect_ident_matching("none")).is_ok() {
> +            return Ok(GenericTranslate(IndividualTransform::None));
> +        }
> +
> +        let tx = specified::LengthOrPercentage::parse(context, input)?;
> +        if let Some(ty) = input.try(|i| specified::LengthOrPercentage::parse(context, i)).ok() {

Why not `if let Ok(..)`, and remove the `.ok()`? Here and below, and also on the previous patch.
Attachment #8938036 - Flags: review?(emilio) → review+
Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review217132

Sorry, one last comment I overlooked.

::: servo/components/style/values/specified/transform.rs:557
(Diff revision 2)
> +        }
> +
> +        let tx = specified::LengthOrPercentage::parse(context, input)?;
> +        if let Some(ty) = input.try(|i| specified::LengthOrPercentage::parse(context, i)).ok() {
> +            if let Some(tz) = input.try(|i| specified::Length::parse(context, i)).ok() {
> +                // 'translate : <number>{3}'

Hmm, this comments need fixing, this is `<length-percentage> <length-percentage> <length>`, no `<number> {..}`, ditto below.
Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review217136

::: servo/components/style/values/computed/transform.rs:299
(Diff revision 2)
>              .collect::<Result<Vec<_>, _>>()?))
>      }
>  }
> +
> +/// A computed CSS `rotate`
> +pub type Rotate = GenericRotate<TransformOperation>;

Hmm, so another comment I initially overlooked and realised when looking at the third patch (whoops).

TransformOperation::to_css serializes the functions, so how is this right?

Wouldn't this patch cause this behavior?

```
element.style.rotate = "10deg";
element.style.rotate; // rotate(10deg)
```

I think this deserves an explanation before I can grant r+ on this patch.

The rest of the patches are equally affected, but maybe I'm misunderstanding something, this should be tested.

Whatever solution we have for rotate we need to adopt for the rest.
Attachment #8938035 - Flags: review+
Comment on attachment 8938037 [details]
Bug 1207734 - Part 7.c. (stylo) Implement scale property styling.

https://reviewboard.mozilla.org/r/208766/#review217138

Same comments as the rest, and need to clarify that serialization issue.

::: servo/components/style/values/specified/transform.rs:586
(Diff revision 2)
> +        }
> +
> +        let sx = Number::parse(context, input)?;
> +        if let Some(sy) = input.try(|i| Number::parse(context, i)).ok() {
> +            if let Some(sz) = input.try(|i| Number::parse(context, i)).ok() {
> +                // 'scale : <length-percentage><length-percentage><length>'

Comment needs fixing, these are numbers.
Attachment #8938037 - Flags: review?(emilio)
Comment on attachment 8938036 [details]
Bug 1207734 - Part 7.b. (stylo) Implement translate property styling.

https://reviewboard.mozilla.org/r/208764/#review217140

The serialization issue needs to be addressed, so clearing review until that is clarified, because I'm either misunderstanding something, or the type representation may need to change to be more specific.

Other than that it looks fine to me.
Attachment #8938036 - Flags: review+
Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review217368

::: layout/style/nsStyleStruct.h:2600
(Diff revision 4)
>    RefPtr<nsCSSValueSharedList> mSpecifiedScale; // [reset]
> +
> +  // Used to store the final combination of mSpecifiedTranslate,
> +  // mSpecifiedRotate, mSpecifiedScale and mSpecifiedTransform.
> +  // Use GetCombinedTransform() to get the final transform, instead of
> +  // accessing mCombinedTransform directly, since this value is initiated

s/initiated/initialized/

or simply "calculated"

::: layout/style/nsStyleStruct.cpp:4034
(Diff revision 4)
> +  const RefPtr<nsCSSValueSharedList>* shareLists[] = { &mSpecifiedTranslate,
> +                                                       &mSpecifiedRotate,
> +                                                       &mSpecifiedScale,
> +                                                       &mSpecifiedTransform };
> +
> +  // Generally, we will have 3 transform functions(one rotate, one trasnlate
> +  // and one scale) plus one rotate, once translate and one scale property
> +  // at most. That's where 6 comes from.
> +  AutoTArray<nsCSSValueList*, 6> valueLists;
> +  for (auto list: shareLists) {
> +    if (*list) {
> +      valueLists.AppendElement((*list)->mHead->Clone());

I don't quite follow the comment. shareLists only has 4 elements? We don't seem to traverse mSpecifiedTransform, so why are we allocating 6 slots of inline storage? Is it because of how mCombinedTransform will be used elsewhere?

If this is correct, I think this could be more clear.

Also, any reason to prefer pointers to RefPtrs? Wouldn't

  const nsCSSValueSharedList* sharedLists[] = { mSpecifiedTransform.get(), ... };

be simpler?

::: layout/style/nsStyleStruct.cpp:4039
(Diff revision 4)
> +  const RefPtr<nsCSSValueSharedList>* shareLists[] = { &mSpecifiedTranslate,
> +                                                       &mSpecifiedRotate,
> +                                                       &mSpecifiedScale,
> +                                                       &mSpecifiedTransform };
> +
> +  // Generally, we will have 3 transform functions(one rotate, one trasnlate

s/trasnlate/translate/

::: layout/style/nsStyleStruct.cpp:4040
(Diff revision 4)
> +                                                       &mSpecifiedRotate,
> +                                                       &mSpecifiedScale,
> +                                                       &mSpecifiedTransform };
> +
> +  // Generally, we will have 3 transform functions(one rotate, one trasnlate
> +  // and one scale) plus one rotate, once translate and one scale property

s/once/one/

::: layout/style/nsStyleStruct.cpp:4048
(Diff revision 4)
> +  MOZ_ASSERT(valueLists.Length());
> +
> +  for (uint32_t i = 0; i < valueLists.Length() - 1; i++) {

It might be worth adding a comment here:

  // Check we have at least one list or else valueLists.Length() - 1 below will
  // underflow.
Attachment #8936571 - Flags: review?(bbirtles) → review+
Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review217380

I'm not sure if it's related to this patch, but if I open up attachment 8938224 [details] in DevTools and inspect the box I see:

  translate: translateX(100px);

which seems wrong.

::: layout/style/nsComputedDOMStyle.h:519
(Diff revision 3)
> +  already_AddRefed<CSSValue> DoGetTranslate();
> +  already_AddRefed<CSSValue> DoGetRotate();
> +  already_AddRefed<CSSValue> DoGetScale();

(Should these go above TransformBox and after Transform? It seems odd to put them between TransformBox and TransformOrigin.)

::: layout/style/nsComputedDOMStyle.cpp:1753
(Diff revision 3)
> +
> +    /* Set it to "none." */

(Not sure the comment and blank line are necessary.)

::: layout/style/nsComputedDOMStyle.cpp:1781
(Diff revision 3)
> +      GeckoStyleContext* contextIfGecko =
> +        self->mStyleContext ? self->mStyleContext->GetAsGecko() : nullptr;
> +      TransformReferenceBox refBox(self->mInnerFrame, nsSize(0, 0));
> +      RuleNodeCacheConditions dummy;
> +
> +      switch (nsStyleTransformMatrix::TransformFunctionOf(aData)) {

Can you add a comment here saying that even though the spec doesn't say to resolve percentage values, Blink and Edge do and so until that is clarified we do as well:

  https://github.com/w3c/csswg-drafts/issues/2124

(Personally I think we should *not* resolve percentages here. It's simpler not to and more common and by resolving percentages here we make it harder to spec editors to argue for preserving percentages since, "all three implementations resolve them". But I'll leave it up to you.)
Attachment #8937970 - Flags: review?(bbirtles) → review+
Comment on attachment 8936571 [details]
Bug 1207734 - Part 4.a. Store the final combined transform in nsStyleDisplay::mCombinedTransform.

https://reviewboard.mozilla.org/r/207326/#review217390

::: layout/style/nsStyleStruct.h:2602
(Diff revision 4)
> +  // Used to store the final combination of mSpecifiedTranslate,
> +  // mSpecifiedRotate, mSpecifiedScale and mSpecifiedTransform.
> +  // Use GetCombinedTransform() to get the final transform, instead of
> +  // accessing mCombinedTransform directly, since this value is initiated
> +  // lazily in GetCombinedTransform().
> +  mutable RefPtr<nsCSSValueSharedList> mCombinedTransform;

I'm somewhat concerned about this `mutable` member, since we do poke at style structs off-main-thread.

At the very least, this needs a `MOZ_ASSERT(NS_IsMainThread())` in everything that reads or writes from it.

Even with that, I'd probably try to avoid it.
Comment on attachment 8940969 [details]
Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

https://reviewboard.mozilla.org/r/211238/#review217386

::: commit-message-d0eb5:1
(Diff revision 1)
> +Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.
> +
> +Since we do not support async-transform for individual-transform yet.

Is this patch supposed to be blocking transform animations whenever we have an individual transform property specified, or when we are trying to animate one of them, or both?

It would be nice to have a few tests to check this, if possible.

If we only really care about individual transforms being animated then we should probably put the check in KeyframeEffectReadOnly::ShouldBlockAsyncTransformAnimations instead.

::: layout/generic/nsFrame.cpp:2827
(Diff revision 1)
>        }
>      }
>      inTransform = true;
>    }
>  
> +  // XXX cku temporary disable async-animation when this frame has any

(nit: s/temporary/temporarily/)

::: layout/style/nsStyleStruct.h:2810
(Diff revision 1)
> +    return mSpecifiedRotate != nullptr ||
> +           mSpecifiedTranslate != nullptr ||
> +           mSpecifiedScale != nullptr;

I believe this should be:

  return !mSpecifiedRotate || !mSpecifiedTranslate || !mSpecifiedScale;

According to our coding style:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

I know HasTransformStyle() does this but I think it's wrong too.
Attachment #8940969 - Flags: review?(bbirtles) → review+
Comment on attachment 8940969 [details]
Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

https://reviewboard.mozilla.org/r/211238/#review217396

::: layout/style/nsStyleStruct.h:2810
(Diff revision 1)
> +    return mSpecifiedRotate != nullptr ||
> +           mSpecifiedTranslate != nullptr ||
> +           mSpecifiedScale != nullptr;

Err, obviously that should be:

  return mSpecifiedRotate || mSpecifiedTranslate || mSpecifiedScale;
Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review217136

> Hmm, so another comment I initially overlooked and realised when looking at the third patch (whoops).
> 
> TransformOperation::to_css serializes the functions, so how is this right?
> 
> Wouldn't this patch cause this behavior?
> 
> ```
> element.style.rotate = "10deg";
> element.style.rotate; // rotate(10deg)
> ```
> 
> I think this deserves an explanation before I can grant r+ on this patch.
> 
> The rest of the patches are equally affected, but maybe I'm misunderstanding something, this should be tested.
> 
> Whatever solution we have for rotate we need to adopt for the rest.

hmm... property serialization is not done, will do it. And will add test cases in test_sepcified_value_serialization.html
Comment on attachment 8940970 [details]
Bug 1207734 - Part 9.g. (testing) Update animation wpt for individual transform.

https://reviewboard.mozilla.org/r/211240/#review217402

This is pretty good but needs a few additional tests as indicated below.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1588
(Diff revision 1)
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      const animation = target.animate(
> +        {
> +          [idlName]: ['45deg', '135deg'],
> +        },
> +        1000
> +      );
> +
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,  expected: '90deg' }]);
> +    }, `${property} without rotation axes`);
> +
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      const animation =
> +        target.animate({ [idlName]: [ '0 1 0 0deg',
> +                                      '0 1 0 90deg'] },
> +                       1000);
> +
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500, expected: '0 1 0 45deg' }]);
> +    }, `${property}  with rotation axes`);

This looks good but could we add one more test for, say 0deg -> 720deg, and then check that at 250 we are at 180deg?

(The reason is just to catch implementations that internally represent rotations as matrices.)

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1606
(Diff revision 1)
> +        target.animate({ [idlName]: [ '0 1 0 0deg',
> +                                      '0 1 0 90deg'] },

Don't we need to test the interpolation/addition/accumulation of the axis too?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1618
(Diff revision 1)
> +  },
> +  testAddition: function(property, setup) {
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      target.style[idlName] = '45deg';

Would it be worth setting target.style.transform = 'rotate(23deg)' just to check that the underlying transform is not reflected? (assuming that property !== 'transform').

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1631
(Diff revision 1)
> +    }, `${property} without rotation axes`);
> +
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      target.style.transform = 'translateX(100px)';

Oh wait, I see you've done that!

Would it be more interesting to add a rotate() value, however?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1676
(Diff revision 1)
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      const animation = target.animate(
> +        {
> +          [idlName]: ['200px', '400px'],
> +        },
> +        1000
> +      );
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,  expected: '300px' }]);
> +    }, `${property} with two unspecified values`);

I think we need a test for interpolating between lengths and percentages.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1834
(Diff revision 1)
> +
> +    test(t => {
> +      const idlName = propertyToIDL(property);
> +      const target = createTestElement(t, setup);
> +      target.style[idlName] = '2 2 2';
> +      const animation = target.animate({ [idlName]: ['-3 -3 -3', '5 5 5'] },

We should test with different values (e.g. '1 2 3' -> '4 5 6') to make sure we are interpolating the correct channels.
Attachment #8940970 - Flags: review?(bbirtles)
Comment on attachment 8938038 [details]
Bug 1207734 - Part 8.b. (stylo) Implement Animate trait for individual transforms.

https://reviewboard.mozilla.org/r/208768/#review217394

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2323
(Diff revision 2)
> +        let (fx, fy, fz, fa) =
> +            transform::get_normalized_vector_and_angle(from.0, from.1, from.2,
> +                                                       from.3);
> +        let (tx, ty, tz, ta) =
> +            transform::get_normalized_vector_and_angle(to.0, to.1, to.2, to.3);
> +        if (fx, fy, fz) == (tx, ty, tz) {
> +            return Ok(Rotate(IndividualTransform::Specified(TransformOperation::Rotate3D(fx, fy, fz, fa.animate(&ta, procedure)?))));
> +        }

Is this the formatting rustfmt gives you?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2339
(Diff revision 2)
> +        
> +        /*let (rx, ry, rz, angle) =
> +            ComputedRotate::matrix3d_to_rotation(&matrix_f.animate(&matrix_t, procedure)?);
> +        Ok(Rotate(IndividualTransform::Specified(TransformOperation::Rotate3D(rx, ry, rz, angle))))*/

Pretty sure this is not supposed to be here?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2432
(Diff revision 2)
> +            // scale(x1,y1,z1)*scale(x2,y2,z2) = scale(x1*x2, y1*y2, z1*z2)
> +            return Ok(Scale(IndividualTransform::Specified(TransformOperation::Scale3D(from.0 * to.0, from.1 * to.1, from.2 * to.2))));

I'm curious where this comes from?

I mean, it makes sense: presumably if you have an additive animation with a value of 'scale: 2' and the underlying value is 'scale: 10', you expect the result to be 'scale: 20' (while accumulate should give you 'scale: 12').

However, is this specified anywhere? Does blink do this? (I guess it's not possible to test Edge since it doesn't have Web Animations yet.)

I think this is ok but we should specify it somewhere and make sure that there is a test that fails without this behavior.
Attachment #8938038 - Flags: review?(bbirtles) → review+
I believe animations for these individual transform properties also create a stacking context, so I think we should modify nsIFrame::HasAnimationOfTransform() as well.
Comment on attachment 8937970 [details]
Bug 1207734 - Part 5. Implement the getComputedStyle API for individual transform.

https://reviewboard.mozilla.org/r/208696/#review217380

Emilio also found this issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1207734#c200

It will be fixed in Part 7.a/b/c
Comment on attachment 8940969 [details]
Bug 1207734 - Part 4.c. Temporary disable async-transform for individual-transform.

https://reviewboard.mozilla.org/r/211238/#review217570

::: layout/generic/nsFrame.cpp:2831
(Diff revision 1)
>  
> +  // XXX cku temporary disable async-animation when this frame has any
> +  // individual transforms before bug 1425837 been fixed.
> +  if (allowAsyncAnimation && disp->HasIndividualTransform()) {
> +    allowAsyncAnimation = false;
> +  }

Fix birtles's comment:
If we only really care about individual transforms being animated then we should probably put the check in KeyframeEffectReadOnly::ShouldBlockAsyncTransformAnimations instead.
Comment on attachment 8938035 [details]
Bug 1207734 - Part 7.a. (stylo) Implement rotate property styling.

https://reviewboard.mozilla.org/r/208762/#review217118

> I think you can avoid using `GenericRotate` and just use `Rotate`? `GenericTransformOperation` is needed due to a long-standing rust issue with enums and associated types.

Will get a compile error by doing this change:
error[E0423]: expected function, found type alias `Rotate`
Attachment #8942583 - Flags: review?(bbirtles)
Attachment #8938035 - Flags: review?(emilio)
Attachment #8938036 - Flags: review?(emilio)
Attachment #8938037 - Flags: review?(emilio)
Attachment #8942584 - Flags: review?(boris.chiou)
Attachment #8942585 - Flags: review?(emilio)
Attachment #8942586 - Flags: review?(bbirtles)
Attachment #8940970 - Flags: review?(bbirtles)
Comment on attachment 8942583 [details]
Bug 1207734 - Part 1.c. Fix coding style of HasTransformStyle.

https://reviewboard.mozilla.org/r/212840/#review218462
Attachment #8942583 - Flags: review?(bbirtles) → review+
Comment on attachment 8942586 [details]
Bug 1207734 - Part 9.f. (testing) Implement testAnimationSampleRotate3d.

https://reviewboard.mozilla.org/r/212846/#review218466
Attachment #8942586 - Flags: review?(bbirtles) → review+
Comment on attachment 8942584 [details]
Bug 1207734 - Part 8.a. (stylo) Implement MatrixDecomposed3D::slerp.

https://reviewboard.mozilla.org/r/212842/#review218464

r=me with the following comment addressed.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2072
(Diff revision 1)
> +impl Animate for MatrixDecomposed3D {
> +    /// <https://drafts.csswg.org/css-transforms/#interpolation-of-decomposed-3d-matrix-values>
> +    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
> +        use std::f64;
> +
> +        let (this_weight, other_weight) = procedure.weights();

`this_weight` and `other_weight` are only used in `debug_assert!(...)`, so let's move this into it, too.

e.g.
```
debug_assert!(
    (procedure.weights().this_weight +
        procedure.weights().other_weight - 1.0f64).abs() <= f64::EPSILON ||
    procedure.weights().other_weight == 1.0f64 ||
    procedure.weights().other_weight == 0.0f64,
    "..."
);
```

or you could add some inline functions into `Precedure` [1], so we don't need to call `prodecure.weigths()` so many times.

or you might just add a function: `is_valid_for_animation()` in `Procedure`.

[1] https://searchfox.org/mozilla-central/rev/137f1b2f434346a0c3756ebfcbdbee4069e15dc8/servo/components/style/values/animated/mod.rs#101
Attachment #8942584 - Flags: review?(boris.chiou) → review+