Closed Bug 1371518 Opened 3 years ago Closed 3 years ago

stylo: Support animation of the display property from SMIL

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 10 obsolete files)

327 bytes, text/html
Details
367 bytes, text/html
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
41 bytes, text/x-github-pull-request
Details | Review
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.
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'.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
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.
Attached patch WIP patch (obsolete) — Splinter Review
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.
Priority: -- → P2
Attachment #8876588 - Attachment is obsolete: true
Comment on attachment 8877445 [details]
Bug 1371518 - Drop Servo_AnimationValueMap_Push;

https://reviewboard.mozilla.org/r/148854/#review153346
Attachment #8877445 - Flags: review?(hikezoe) → review+
Comment on attachment 8877446 [details]
Bug 1371518 - Move definition of animatable for shorthands to Shorthand object;

https://reviewboard.mozilla.org/r/148856/#review153348
Attachment #8877446 - Flags: review?(hikezoe) → review+
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
Attachment #8877447 - Flags: review?(hikezoe) → review+
Comment on attachment 8877448 [details]
Bug 1371518 - Introduce AnimatableLonghand type;

https://reviewboard.mozilla.org/r/148860/#review153354
Attachment #8877448 - Flags: review?(hikezoe) → review+
Comment on attachment 8877449 [details]
Bug 1371518 - Use AnimatableLonghand for AnimationValueMap and related code;

https://reviewboard.mozilla.org/r/148862/#review153364
Attachment #8877449 - Flags: review?(hikezoe) → review+
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?
Attachment #8877450 - Flags: review?(hikezoe) → review+
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?
Attachment #8877451 - Flags: review?(hikezoe) → review+
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
Attachment #8877452 - Flags: review?(hikezoe) → review+
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
Attachment #8877453 - Flags: review?(hikezoe) → review+
Comment on attachment 8877454 [details]
Bug 1371518 - Use Servo backend to determine if a property is transitionable;

https://reviewboard.mozilla.org/r/148872/#review153394
Attachment #8877454 - Flags: review?(hikezoe) → review+
Comment on attachment 8877455 [details]
Bug 1371518 - Make KeyframeUtils::IsAnimatable consult the Servo backend;

https://reviewboard.mozilla.org/r/148874/#review153402
Attachment #8877455 - Flags: review?(hikezoe) → review+
Comment on attachment 8877456 [details]
Bug 1371518 - Make 'display' animatable;

https://reviewboard.mozilla.org/r/148876/#review153406
Attachment #8877456 - Flags: review?(hikezoe) → review+
Comment on attachment 8877457 [details]
Bug 1371518 - Update test expectations;

https://reviewboard.mozilla.org/r/148878/#review153408
Attachment #8877457 - Flags: review?(hikezoe) → review+
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 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?
(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.
(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.
(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()?
(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.
Attachment #8877446 - Attachment is obsolete: true
Attachment #8877447 - Attachment is obsolete: true
Attachment #8877448 - Attachment is obsolete: true
Attachment #8877449 - Attachment is obsolete: true
Attachment #8877450 - Attachment is obsolete: true
Attachment #8877451 - Attachment is obsolete: true
Attachment #8877452 - Attachment is obsolete: true
Attachment #8877453 - Attachment is obsolete: true
Attachment #8877456 - Attachment is obsolete: true
(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.
Attached file Servo PR #17331
Pushed by bbirtles@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.