From e74f7792f5550cd74def97fd789cb94f06ce887a Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:19:48 +0900 Subject: [PATCH] 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). --- components/style/gecko/wrapper.rs | 37 +++++++--------- components/style/properties/data.py | 12 +++++ .../helpers/animated_properties.mako.rs | 44 ++++++++----------- 3 files changed, 47 insertions(+), 46 deletions(-) diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 8aa229f7d26..b74fc684825 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1040,7 +1040,6 @@ impl<'le> TElement for GeckoElement<'le> { after_change_style: &ComputedValues) -> bool { use gecko_bindings::structs::nsCSSPropertyID; - use properties::{PropertyId, animated_properties}; use std::collections::HashSet; debug_assert!(self.might_need_transitions_update(Some(before_change_style), @@ -1075,6 +1074,8 @@ impl<'le> TElement for GeckoElement<'le> { continue; } + let transition_property: TransitionProperty = property.into(); + let mut property_check_helper = |property: &TransitionProperty| -> bool { if self.needs_transitions_update_per_property(property, combined_duration, @@ -1091,26 +1092,25 @@ impl<'le> TElement for GeckoElement<'le> { } false }; - if property == nsCSSPropertyID::eCSSPropertyExtra_all_properties { - if TransitionProperty::any(property_check_helper) { - return true; - } - } else { - let is_shorthand = PropertyId::from_nscsspropertyid(property).ok().map_or(false, |p| { - p.as_shorthand().is_ok() - }); - if is_shorthand { - let shorthand: TransitionProperty = property.into(); + + match transition_property { + TransitionProperty::All => { + if TransitionProperty::any(property_check_helper) { + return true; + } + }, + TransitionProperty::Unsupported(_) => { }, + ref shorthand if shorthand.is_shorthand() => { if shorthand.longhands().iter().any(|p| property_check_helper(p)) { return true; } - } else { - if animated_properties::nscsspropertyid_is_animatable(property) && - property_check_helper(&property.into()) { + }, + ref longhand => { + if property_check_helper(longhand) { return true; } - } - } + }, + }; } // Check if we have to cancel the running transition because this is not a matching @@ -1133,11 +1133,6 @@ impl<'le> TElement for GeckoElement<'le> { // |property| should be an animatable longhand let animatable_longhand = AnimatableLonghand::from_transition_property(property).unwrap(); - // We don't allow transitions on properties that are not interpolable. - if animatable_longhand.is_discrete() { - return false; - } - if existing_transitions.contains_key(property) { // If there is an existing transition, update only if the end value differs. // If the end value has not changed, we should leave the currently running diff --git a/components/style/properties/data.py b/components/style/properties/data.py index 0822e25e320..c0510d0c13f 100644 --- a/components/style/properties/data.py +++ b/components/style/properties/data.py @@ -194,12 +194,15 @@ class Longhand(object): self.animation_value_type = animation_value_type self.animatable = animation_value_type != "none" + self.transitionable = animation_value_type != "none" \ + and animation_value_type != "discrete" self.is_animatable_with_computed_value = animation_value_type == "ComputedValue" \ or animation_value_type == "discrete" if self.logical: # Logical properties will be animatable (i.e. the animation type is # discrete). For now, it is still non-animatable. self.animatable = False + self.transitionable = False self.animation_type = None # NB: Animatable implies clone because a property animation requires a # copy of the computed value. @@ -242,7 +245,16 @@ class Shorthand(object): break return animatable + def get_transitionable(self): + transitionable = False + for sub in self.sub_properties: + if sub.transitionable: + transitionable = True + break + return transitionable + animatable = property(get_animatable) + transitionable = property(get_transitionable) class Method(object): diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 315673c591f..e6a1b8985ba 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -95,13 +95,7 @@ impl AnimatableLonghand { pub fn from_transition_property(transition_property: &TransitionProperty) -> Option { match *transition_property { % for prop in data.longhands: - <% - # TODO: Later in this patch series, once we introduce the 'transitionable' - # definition, we will need to make the below test: - # - # if prop.transitionable and prop.animatable: - %> - % if prop.animatable: + % if prop.transitionable and prop.animatable: TransitionProperty::${prop.camel_case} => Some(AnimatableLonghand::${prop.camel_case}), % endif @@ -169,22 +163,22 @@ impl<'a> From for PropertyDeclarationId<'a> { } } -/// A given transition property, that is either `All`, an animatable longhand property, -/// a shorthand with at least one animatable longhand component, or an unsupported property. +/// A given transition property, that is either `All`, a transitionable longhand property, +/// a shorthand with at least one transitionable longhand component, or an unsupported property. // NB: This needs to be here because it needs all the longhands generated // beforehand. #[derive(Clone, Debug, PartialEq, Eq, Hash)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub enum TransitionProperty { - /// All, any animatable property changing should generate a transition. + /// All, any transitionable property changing should generate a transition. All, % for prop in data.longhands + data.shorthands_except_all(): - % if prop.animatable: + % if prop.transitionable: /// ${prop.name} ${prop.camel_case}, % endif % endfor - /// Unrecognized property which could be any non-animatable, custom property, or + /// Unrecognized property which could be any non-transitionable, custom property, or /// unknown property. Unsupported(Atom) } @@ -197,17 +191,17 @@ impl TransitionProperty { /// Iterates over each longhand property. pub fn each ()>(mut cb: F) { % for prop in data.longhands: - % if prop.animatable: + % if prop.transitionable: cb(&TransitionProperty::${prop.camel_case}); % endif % endfor } - /// Iterates over every property that is not TransitionProperty::All, stopping and returning - /// true when the provided callback returns true for the first time. + /// Iterates over every longhand property that is not TransitionProperty::All, stopping and + /// returning true when the provided callback returns true for the first time. pub fn any bool>(mut cb: F) -> bool { % for prop in data.longhands: - % if prop.animatable: + % if prop.transitionable: if cb(&TransitionProperty::${prop.camel_case}) { return true; } @@ -222,7 +216,7 @@ impl TransitionProperty { (match_ignore_ascii_case! { &ident, "all" => Ok(TransitionProperty::All), % for prop in data.longhands + data.shorthands_except_all(): - % if prop.animatable: + % if prop.transitionable: "${prop.name}" => Ok(TransitionProperty::${prop.camel_case}), % endif % endfor @@ -236,13 +230,13 @@ impl TransitionProperty { }).map_err(|()| SelectorParseError::UnexpectedIdent(ident.into()).into()) } - /// Return animatable longhands of this shorthand TransitionProperty, except for "all". + /// Return transitionable longhands of this shorthand TransitionProperty, except for "all". pub fn longhands(&self) -> &'static [TransitionProperty] { % for prop in data.shorthands_except_all(): - % if prop.animatable: + % if prop.transitionable: static ${prop.ident.upper()}: &'static [TransitionProperty] = &[ % for sub in prop.sub_properties: - % if sub.animatable: + % if sub.transitionable: TransitionProperty::${sub.camel_case}, % endif % endfor @@ -251,7 +245,7 @@ impl TransitionProperty { % endfor match *self { % for prop in data.shorthands_except_all(): - % if prop.animatable: + % if prop.transitionable: TransitionProperty::${prop.camel_case} => ${prop.ident.upper()}, % endif % endfor @@ -263,7 +257,7 @@ impl TransitionProperty { pub fn is_shorthand(&self) -> bool { match *self { % for prop in data.shorthands_except_all(): - % if prop.animatable: + % if prop.transitionable: TransitionProperty::${prop.camel_case} => true, % endif % endfor @@ -292,7 +286,7 @@ impl ToCss for TransitionProperty { match *self { TransitionProperty::All => dest.write_str("all"), % for prop in data.longhands + data.shorthands_except_all(): - % if prop.animatable: + % if prop.transitionable: TransitionProperty::${prop.camel_case} => dest.write_str("${prop.name}"), % endif % endfor @@ -312,7 +306,7 @@ impl<'a> From< &'a TransitionProperty> for nsCSSPropertyID { fn from(transition_property: &'a TransitionProperty) -> nsCSSPropertyID { match *transition_property { % for prop in data.longhands + data.shorthands_except_all(): - % if prop.animatable: + % if prop.transitionable: TransitionProperty::${prop.camel_case} => ${helpers.to_nscsspropertyid(prop.ident)}, % endif @@ -330,7 +324,7 @@ impl From for TransitionProperty { fn from(property: nsCSSPropertyID) -> TransitionProperty { match property { % for prop in data.longhands + data.shorthands_except_all(): - % if prop.animatable: + % if prop.transitionable: ${helpers.to_nscsspropertyid(prop.ident)} => TransitionProperty::${prop.camel_case}, % else: