1371518 - stylo: Support animation of the display property from SMIL

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles)]

Normally we don't allow animating the display property from CSS or Web Animations (we tried and all sorts of things started failing which we never got around to investigating). SMIL, however, allows this and it's quite useful and common.

In Stylo we don't yet support this because we treat 'display' as not animatable. As a result a number of tests fail.

In Gecko, it seems like this works because StyleAnimationValue::ComputeValue doesn't check if the property is animatable. Instead it has this logic:

  if (nsCSSProps::IsShorthand(aProperty) ||
      nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_None) {
    // Just capture the specified value
    aComputedValue.SetUnparsedStringValue(nsString(aSpecifiedValue));
    ...
    return true;
  }

Then, in SMIL (specifically nsSMILCSSProperty::IsPropertyAnimatable) we filter out properties that should not be animatable.

We need to add some way to represent animation values for display in Servo so we can fix these tests.

Comment 1

[Brian Birtles (:birtles)]

I've been deliberating if it is better to do:

a) Treat 'display' as not animatable and extra exceptions to create AnimationValues for it as-needed (e.g. when called for SMIL).
b) Treat 'display' as animatable and add checks in CSS animations/transitions/Web animations to make sure we skip it.
c) Somewhere in between (a) and (b) where we mark 'display' as animatable in the properties file so that we create AnimationValues as usual, but then override all the functions that test if a property is animatable (e.g. nscsspropertyid_is_animatable) to make them return false so that it appears to be not animatable from that point of view.

Gecko roughly does (c) but I'm leaning towards (b). It seems more honest and simple (and I suspect we will want to make Web Animations be able to animate 'display' in future). I need to be careful, however, to not regress Servo by making it start transitioning/animating 'display'.

Comment 2

[Brian Birtles (:birtles)]

Attachment: CSS animation test (test that display is not animatable)

Comment 3

[Brian Birtles (:birtles)]

Attachment: CSS transition test (test that display is not animatable)

Comment 4

[Brian Birtles (:birtles)]

I did some investigation into this. It's easy to make 'display' animatable; my
main concern is not to make it accidentally animatable from CSS animations, CSS
transitions, or Web Animations.

Looking into each of those cases...


A) Web Animations where display is set in a property-indexed keyframe

Do we have a test for this?

  Yes: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html

How should it work?

  Currently this happens to work since KeyframeUtils::IsAnimatableProperty still uses

    nsCSSProps::kAnimTypeTable[aProperty] != eStyleAnimType_None

  We should probably do the same thing as IsAnimatable in
  nsTransitionManager.cpp (see below) and, in the Servo case, call
  Servo_Property_IsAnimatable(aProperty) and then also explicitly skip
  declarations where the property is 'display'.


B) Web Animations where display is set in a keyframes array

Do we have a test for this?
  Yes: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html

How should it work?

  As with (A).


C) CSS Animations where display is set in a keyframe

Do we have a test for this?

  Yes: layout/style/test/test_animations.html (which fails if we simply set the
  animation type to discrete)

  (Output could be improved, however. It currently doesn't mention the property
  name. It just says:
  non-animatable properties should be ignored (linear, 0s) - got "none", expected "block")

How should it work in Stylo?

  Servo_StyleSet_GetKeyframesForName only iterates through declarations that are
  animatable

  We should make this check for animatable and not display.

How should it work in Servo?

  I'm still looking into this but see (D). If, for example, we can make
  TransitionProperty not include display that would be a start.


D) CSS Transitions where display is set in a keyframe

Do we have a test for this?

  Yes: layout/style/test/test_transitions_per_property.html

How should it work in Stylo?

  It currently happens to work because we don't support discrete animations
  (i.e. interpolate will fail and we won't generate the transition).

  However, we should extend IsAnimatable in nsTransitionManager.cpp to
  explicitly check for display and return false in that case.

How should it work in Servo?

  Ideally I'd like to make TransitionProperty only support
  transitionable-properties (i.e. ignore discrete properties) but I'm not sure
  yet if that is possible. (Ideally I think I'd like to have AnimationProperty
  and TransitionProperty where TransitionProperty excludes discretely animatable
  properties, both exclude display, and then have some conversion between the
  two.)

  I'm pretty sure there's already a bug in Servo where it is currently
  transitioning discrete properties where it shouldn't so we need to change
  something here but this will take some investigation.

Comment 5

[Brian Birtles (:birtles)]

Attachment: WIP patch

WIP attempt at splitting off a separate AnimatableLonghand type from TransitionProperty. Surprisingly this doesn't appear to have completely broken everything and in a number of cases seems to make the code simpler, clearer, and more robust. It's going to be hard to split this up into small pieces though.

Comment 6

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Drop Servo_AnimationValueMap_Push;

This is no longer needed as of bug 1311257 since we now compose animations on
the Servo side.

Review commit: https://reviewboard.mozilla.org/r/148854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148854/

Comment 7

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Move definition of animatable for shorthands to Shorthand object;

By moving this definition to the Shorthand object we can more easily re-use it
in subsequent patches in this series.

Review commit: https://reviewboard.mozilla.org/r/148856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148856/

Comment 8

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Only include shorthands with at least one animatable component in TransitionProperty;

This allows simplifying the code somewhat and means we can ignore unanimated
shorthands a little sooner. Furthermore, it removes the odd inconsistency where
TransitionProperty only included animatable longhands but allowed all
shorthands regardless of whether or not they were animatable.

Review commit: https://reviewboard.mozilla.org/r/148858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148858/

Comment 9

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Introduce AnimatableLonghand type;

This type, which we will use in the next patch in this series, can represent
only longhands whose animation type is not "none". By introducing this type,
we can later restrict the meaning of TransitionProperty to only cover properties
whose animation type is not "none" OR "discrete" (since currently CSS
transitions should not animate properties whose animation type is discrete).
Doing so will also mean that CSS transitions ignore the 'display' property by
default.

Furthermore, introducing this type will allow the animation code to clearly
document when a property is allowed to be a shorthand or unanimatable property
and when it is expected to be an animatable longhand. This, in turn, will allow
us to remove a few no-longer-necessary checks and simplify the code.

Review commit: https://reviewboard.mozilla.org/r/148860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148860/

Comment 10

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Use AnimatableLonghand for AnimationValueMap and related code;

In the next few patches we move all non-transition related code over to using
AnimatableLonghand instead of TransitionProperty. This will allow us to
re-purpose TransitionProperty to represent only properties that can be
transitioned (i.e. excluding discrete properties) as well as simplifying the
code by removing the need to deal with shorthands and the "all" value in places
that do not need to handle those values.

Review commit: https://reviewboard.mozilla.org/r/148862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148862/

Comment 11

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Convert AnimationValue::from_computed_values to take an AnimatableLonghand;

Review commit: https://reviewboard.mozilla.org/r/148864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148864/

Comment 12

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Move is_discrete from TransitionProperty to AnimatableLonghand;

Review commit: https://reviewboard.mozilla.org/r/148866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148866/

Comment 13

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Make TransitionProperty treat all properties that are not transitionable as unsupported;

Currently properties that are discretely animated cannot be transitioned. Now
that TransitionProperty should only be used for transitions, we can redefine it
to treat non-transitionable properties as unsupported. This should allow us to
simplify the code and make it more self-documenting (e.g. making
TransitionProperty actually relate to transitions).

Review commit: https://reviewboard.mozilla.org/r/148868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148868/

Comment 14

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Move nscssproperty_id_is_animatable together with the other animatable-related code;

Now we have AnimatableLonghand (to do with animatability) and
TransitionProperty (to do with transitionability), we should move
nscssproperty_id_is_animatable to be part of the former group.

Review commit: https://reviewboard.mozilla.org/r/148870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148870/

Comment 15

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Use Servo backend to determine if a property is transitionable;

This will allow Gecko to use the same criteria as Servo for evaluating if
a property should be transitioned and will allow Gecko to exclude discretely
animated properties, including, in future, the 'display' property.

Review commit: https://reviewboard.mozilla.org/r/148872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148872/

Comment 16

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Make KeyframeUtils::IsAnimatable consult the Servo backend;

When styling with the Servo backend, we should also use the Servo backend to
determine if a property is animatable or not. However, if we do this,
Servo_Property_IsAnimatable will start returning true for the 'display' property
once we mark that as animatable in Servo (for SMIL).

Even if we later fail to actually animate 'display' (due checks added to Servo
in the next patch) we still need to treat 'display' as un-animatable in
KeyframeUtils so that we don't *read* the 'display' property of Keyframe objects
passed to the Animations API, since that is observable.

This patch makes us consult Servo_Property_IsAnimatable when using the Servo
backend and also explicitly treat the 'display' property as not animatable.

Review commit: https://reviewboard.mozilla.org/r/148874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148874/

Comment 17

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Make 'display' animatable;

But not be CSS animations or Web Animations.

Review commit: https://reviewboard.mozilla.org/r/148876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148876/

Comment 18

[Brian Birtles (:birtles)]

Attachment: Bug 1371518 - Update test expectations;

Review commit: https://reviewboard.mozilla.org/r/148878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148878/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877445 [details]
Bug 1371518 - Drop Servo_AnimationValueMap_Push;

https://reviewboard.mozilla.org/r/148854/#review153346

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877446 [details]
Bug 1371518 - Move definition of animatable for shorthands to Shorthand object;

https://reviewboard.mozilla.org/r/148856/#review153348

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877447 [details]
Bug 1371518 - Only include shorthands with at least one animatable component in TransitionProperty;

https://reviewboard.mozilla.org/r/148858/#review153352

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877448 [details]
Bug 1371518 - Introduce AnimatableLonghand type;

https://reviewboard.mozilla.org/r/148860/#review153354

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877449 [details]
Bug 1371518 - Use AnimatableLonghand for AnimationValueMap and related code;

https://reviewboard.mozilla.org/r/148862/#review153364

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877450 [details]
Bug 1371518 - Convert AnimationValue::from_computed_values to take an AnimatableLonghand;

https://reviewboard.mozilla.org/r/148864/#review153366

::: servo/components/style/properties/helpers/animated_properties.mako.rs:624
(Diff revision 1)
>              },
>              _ => None // non animatable properties will get included because of shorthands. ignore.
>          }
>      }
>  
> -    /// Get an AnimationValue for a TransitionProperty from a given computed values.
> +    /// Get an AnimationValue for a AnimatableLonghand from a given computed values.

Nit: an AnimatableLonghand.

::: servo/ports/geckolib/glue.rs:691
(Diff revision 1)
> +    let property = match AnimatableLonghand::from_nscsspropertyid(property_id) {
> +        Some(longhand) => longhand,
> +        None => { return Strong::null(); }
> +    };

I saw similar setup in a previous patch, should we add a macro for this setup such as get_property_id_from_nscsspropertyid?

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877451 [details]
Bug 1371518 - Move is_discrete from TransitionProperty to AnimatableLonghand;

https://reviewboard.mozilla.org/r/148866/#review153370

::: servo/ports/geckolib/glue.rs:707
(Diff revision 1)
> -    let property: TransitionProperty = property.into();
> -    property.is_discrete()
> +    match AnimatableLonghand::from_nscsspropertyid(property) {
> +        Some(longhand) => longhand.is_discrete(),
> +        None => false
> +    }
>  }

Nit: Should rewrite this with the macro?

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877452 [details]
Bug 1371518 - Make TransitionProperty treat all properties that are not transitionable as unsupported;

https://reviewboard.mozilla.org/r/148868/#review153374

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877453 [details]
Bug 1371518 - Move nscssproperty_id_is_animatable together with the other animatable-related code;

https://reviewboard.mozilla.org/r/148870/#review153376

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877454 [details]
Bug 1371518 - Use Servo backend to determine if a property is transitionable;

https://reviewboard.mozilla.org/r/148872/#review153394

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877455 [details]
Bug 1371518 - Make KeyframeUtils::IsAnimatable consult the Servo backend;

https://reviewboard.mozilla.org/r/148874/#review153402

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877456 [details]
Bug 1371518 - Make 'display' animatable;

https://reviewboard.mozilla.org/r/148876/#review153406

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8877457 [details]
Bug 1371518 - Update test expectations;

https://reviewboard.mozilla.org/r/148878/#review153408

Comment 32

[Brian Birtles (:birtles)]

Comment on attachment 8877450 [details]
Bug 1371518 - Convert AnimationValue::from_computed_values to take an AnimatableLonghand;

https://reviewboard.mozilla.org/r/148864/#review153672

::: servo/ports/geckolib/glue.rs:691
(Diff revision 1)
> +    let property = match AnimatableLonghand::from_nscsspropertyid(property_id) {
> +        Some(longhand) => longhand,
> +        None => { return Strong::null(); }
> +    };

I thought I saw it somewhere too, but I can't find it.

Comment 33

[Brian Birtles (:birtles)]

Comment on attachment 8877451 [details]
Bug 1371518 - Move is_discrete from TransitionProperty to AnimatableLonghand;

https://reviewboard.mozilla.org/r/148866/#review153682

::: servo/ports/geckolib/glue.rs:707
(Diff revision 1)
> -    let property: TransitionProperty = property.into();
> -    property.is_discrete()
> +    match AnimatableLonghand::from_nscsspropertyid(property) {
> +        Some(longhand) => longhand.is_discrete(),
> +        None => false
> +    }
>  }

Which macro?

Comment 34

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #33)
> Comment on attachment 8877451 [details]
> Bug 1371518 - Move is_discrete from TransitionProperty to AnimatableLonghand;
> 
> https://reviewboard.mozilla.org/r/148866/#review153682
> 
> ::: servo/ports/geckolib/glue.rs:707
> (Diff revision 1)
> > -    let property: TransitionProperty = property.into();
> > -    property.is_discrete()
> > +    match AnimatableLonghand::from_nscsspropertyid(property) {
> > +        Some(longhand) => longhand.is_discrete(),
> > +        None => false
> > +    }
> >  }
> 
> Which macro?

The one in comment 24.

Comment 35

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> (In reply to Brian Birtles (:birtles) from comment #33)
> > Comment on attachment 8877451 [details]
> > Bug 1371518 - Move is_discrete from TransitionProperty to AnimatableLonghand;
> > 
> > https://reviewboard.mozilla.org/r/148866/#review153682
> > 
> > ::: servo/ports/geckolib/glue.rs:707
> > (Diff revision 1)
> > > -    let property: TransitionProperty = property.into();
> > > -    property.is_discrete()
> > > +    match AnimatableLonghand::from_nscsspropertyid(property) {
> > > +        Some(longhand) => longhand.is_discrete(),
> > > +        None => false
> > > +    }
> > >  }
> > 
> > Which macro?
> 
> The one in comment 24.

The two bits of code in question are:

    let property = match AnimatableLonghand::from_nscsspropertyid(property_id) {
        Some(longhand) => longhand,
        None => { return Strong::null(); }
    };

and:

    match AnimatableLonghand::from_nscsspropertyid(property) {
        Some(longhand) => longhand.is_discrete(),
        None => false
    }

They seem pretty different to me.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #35)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> > (In reply to Brian Birtles (:birtles) from comment #33)
> > > Comment on attachment 8877451 [details]
> > > Bug 1371518 - Move is_discrete from TransitionProperty to AnimatableLonghand;
> > > 
> > > https://reviewboard.mozilla.org/r/148866/#review153682
> > > 
> > > ::: servo/ports/geckolib/glue.rs:707
> > > (Diff revision 1)
> > > > -    let property: TransitionProperty = property.into();
> > > > -    property.is_discrete()
> > > > +    match AnimatableLonghand::from_nscsspropertyid(property) {
> > > > +        Some(longhand) => longhand.is_discrete(),
> > > > +        None => false
> > > > +    }
> > > >  }
> > > 
> > > Which macro?
> > 
> > The one in comment 24.
> 
> The two bits of code in question are:
> 
>     let property = match
> AnimatableLonghand::from_nscsspropertyid(property_id) {
>         Some(longhand) => longhand,
>         None => { return Strong::null(); }
>     };
> 
> and:
> 
>     match AnimatableLonghand::from_nscsspropertyid(property) {
>         Some(longhand) => longhand.is_discrete(),
>         None => false
>     }
> 
> They seem pretty different to me.

Given that the macro looks like this:

macro_rules! get_animatable_longhand_from_nscsspropertyid {
    ($property_id: ident, $ret: expr) => {{
        match AnimatableLonghand::from_nscsspropertyid($property_id) {
            Some(longhand) => longhand,
            None => { return $ret; }
        }
    }}
}

I guessed we can do with a method chain for the is_discrete type? No? Something like this;

get_animatable_longhand_from_nscsspropertyid!(property, false).is_discrete()?

Comment 37

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> Given that the macro looks like this:
> 
> macro_rules! get_animatable_longhand_from_nscsspropertyid {
>     ($property_id: ident, $ret: expr) => {{
>         match AnimatableLonghand::from_nscsspropertyid($property_id) {
>             Some(longhand) => longhand,
>             None => { return $ret; }
>         }
>     }}
> }
> 
> I guessed we can do with a method chain for the is_discrete type? No?
> Something like this;
> 
> get_animatable_longhand_from_nscsspropertyid!(property, false).is_discrete()?

Oh, you meant, in this case, we need another if check there. yeah, right.

Comment 38

[Brian Birtles (:birtles)]

Comment on attachment 8877445 [details]
Bug 1371518 - Drop Servo_AnimationValueMap_Push;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148854/diff/1-2/

Comment 39

[Brian Birtles (:birtles)]

Comment on attachment 8877454 [details]
Bug 1371518 - Use Servo backend to determine if a property is transitionable;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148872/diff/1-2/

Comment 40

[Brian Birtles (:birtles)]

Comment on attachment 8877455 [details]
Bug 1371518 - Make KeyframeUtils::IsAnimatable consult the Servo backend;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148874/diff/1-2/

Comment 41

[Brian Birtles (:birtles)]

Comment on attachment 8877457 [details]
Bug 1371518 - Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148878/diff/1-2/

Comment 42

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #37)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> > Given that the macro looks like this:
> > 
> > macro_rules! get_animatable_longhand_from_nscsspropertyid {
> >     ($property_id: ident, $ret: expr) => {{
> >         match AnimatableLonghand::from_nscsspropertyid($property_id) {
> >             Some(longhand) => longhand,
> >             None => { return $ret; }
> >         }
> >     }}
> > }
> > 
> > I guessed we can do with a method chain for the is_discrete type? No?
> > Something like this;
> > 
> > get_animatable_longhand_from_nscsspropertyid!(property, false).is_discrete()?
> 
> Oh, you meant, in this case, we need another if check there. yeah, right.

Yeah, this just seems to be making the code harder to follow in my opinion.

Comment 43

[Brian Birtles (:birtles)]

Attachment: Servo PR #17331

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8652aa453785
Drop Servo_AnimationValueMap_Push; r=hiro
https://hg.mozilla.org/integration/autoland/rev/56723b1e3e8e
Use Servo backend to determine if a property is transitionable; r=hiro
https://hg.mozilla.org/integration/autoland/rev/7978f1be994b
Make KeyframeUtils::IsAnimatable consult the Servo backend; r=hiro
https://hg.mozilla.org/integration/autoland/rev/183ff6627fd3
Update test expectations; r=hiro

Comment 45

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8652aa453785
https://hg.mozilla.org/mozilla-central/rev/56723b1e3e8e
https://hg.mozilla.org/mozilla-central/rev/7978f1be994b
https://hg.mozilla.org/mozilla-central/rev/183ff6627fd3