Bug 1371518 - Make TransitionProperty treat all properties that are not transitionable as unsupported; r?hiro draft
authorBrian Birtles <birtles@gmail.com>
Wed, 14 Jun 2017 15:00:00 +0900
changeset 593857 273a82324c9aa35eb3efa84bc6ae53c7fb330a84
parent 593856 88bb0120ce4ffeacda44ccf22903aa92943774b0
child 593858 a23a9f3e1bb42bbef5ac8d8a86b6ab180be5337b
push id63840
push userbbirtles@mozilla.com
push dateWed, 14 Jun 2017 07:29:38 +0000
reviewershiro
bugs1371518
milestone56.0a1
Bug 1371518 - Make TransitionProperty treat all properties that are not transitionable as unsupported; r?hiro 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). MozReview-Commit-ID: 17NZjhyLq4B
servo/components/style/gecko/wrapper.rs
servo/components/style/properties/data.py
servo/components/style/properties/helpers/animated_properties.mako.rs
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -1030,17 +1030,16 @@ impl<'le> TElement for GeckoElement<'le>
     // update.
     //
     // https://drafts.csswg.org/css-transitions/#starting
     fn needs_transitions_update(&self,
                                 before_change_style: &ComputedValues,
                                 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),
                                                          after_change_style),
                       "We should only call needs_transitions_update if \
                        might_need_transitions_update returns true");
 
         let after_change_box_style = after_change_style.get_box();
@@ -1065,52 +1064,53 @@ impl<'le> TElement for GeckoElement<'le>
             let property = after_change_box_style.transition_nscsspropertyid_at(i);
             let combined_duration = after_change_box_style.transition_combined_duration_at(i);
 
             // We don't need to update transition for none/custom properties.
             if is_none_or_custom_property(property) {
                 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,
                                                               before_change_style,
                                                               after_change_style,
                                                               &existing_transitions) {
                     return true;
                 }
 
                 if let Some(set) = transitions_to_keep.as_mut() {
                     // The TransitionProperty here must be animatable, so cloning it is cheap
                     // because it is an integer-like enum.
                     set.insert(property.clone());
                 }
                 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
         // transition-property value.
         transitions_to_keep.map_or(false, |set| {
             existing_transitions.keys().any(|property| !set.contains(property))
         })
     }
@@ -1123,21 +1123,16 @@ impl<'le> TElement for GeckoElement<'le>
                                              existing_transitions: &HashMap<TransitionProperty,
                                                                             Arc<AnimationValue>>)
                                              -> bool {
         use properties::animated_properties::AnimatedProperty;
 
         // |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
             // transition as-is since we don't want to interrupt its timing function.
             let after_value =
                 Arc::new(AnimationValue::from_computed_values(&animatable_longhand,
                                                               after_change_style));
             return existing_transitions.get(property).unwrap() != &after_value;
--- a/servo/components/style/properties/data.py
+++ b/servo/components/style/properties/data.py
@@ -189,22 +189,25 @@ class Longhand(object):
 
         # This is done like this since just a plain bool argument seemed like
         # really random.
         if animation_value_type is None:
             raise TypeError("animation_value_type should be specified for (" + name + ")")
         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.
         #
         # See components/style/helpers/animated_properties.mako.rs.
         self.need_clone = need_clone or self.animatable
 
 
@@ -237,17 +240,26 @@ class Shorthand(object):
     def get_animatable(self):
         animatable = False
         for sub in self.sub_properties:
             if sub.animatable:
                 animatable = True
                 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):
     def __init__(self, name, return_type=None, arg_types=None, is_mut=False):
         self.name = name
         self.return_type = return_type
         self.arg_types = arg_types or []
         self.is_mut = is_mut
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -90,23 +90,17 @@ impl AnimatableLonghand {
         }
     }
 
     /// Converts from TransitionProperty. Returns None if the property is not an animatable
     /// longhand.
     pub fn from_transition_property(transition_property: &TransitionProperty) -> Option<Self> {
         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
             % endfor
             _ => None
         }
     }
 
@@ -164,111 +158,111 @@ impl<'a> From<AnimatableLonghand> for Pr
                     AnimatableLonghand::${prop.camel_case}
                         => PropertyDeclarationId::Longhand(LonghandId::${prop.camel_case}),
                 % endif
             % endfor
         }
     }
 }
 
-/// 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)
 }
 
 no_viewport_percentage!(TransitionProperty);
 
 impl ComputedValueAsSpecified for TransitionProperty {}
 
 impl TransitionProperty {
     /// Iterates over each longhand property.
     pub fn each<F: FnMut(&TransitionProperty) -> ()>(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<F: FnMut(&TransitionProperty) -> bool>(mut cb: F) -> bool {
         % for prop in data.longhands:
-            % if prop.animatable:
+            % if prop.transitionable:
                 if cb(&TransitionProperty::${prop.camel_case}) {
                     return true;
                 }
             % endif
         % endfor
         false
     }
 
     /// Parse a transition-property value.
     pub fn parse<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
         let ident = try!(input.expect_ident());
         (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
             "none" => Err(()),
             _ => {
                 match CSSWideKeyword::from_ident(&ident) {
                     Some(_) => Err(()),
                     None => Ok(TransitionProperty::Unsupported((&*ident).into()))
                 }
             }
         }).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
                 ];
             % endif
         % 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
             _ => panic!("Not allowed to call longhands() for this TransitionProperty")
         }
     }
 
     /// Returns true if this TransitionProperty is a shorthand.
     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
             _ => false
         }
     }
 }
 
@@ -287,17 +281,17 @@ pub fn nscsspropertyid_is_animatable(pro
 
 impl ToCss for TransitionProperty {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result
         where W: fmt::Write,
     {
         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
             #[cfg(feature = "gecko")]
             TransitionProperty::Unsupported(ref atom) => serialize_identifier(&atom.to_string(),
                                                                               dest),
             #[cfg(feature = "servo")]
             TransitionProperty::Unsupported(ref atom) => serialize_identifier(atom, dest),
@@ -307,17 +301,17 @@ impl ToCss for TransitionProperty {
 
 /// Convert to nsCSSPropertyID.
 #[cfg(feature = "gecko")]
 #[allow(non_upper_case_globals)]
 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
             % endfor
             TransitionProperty::All => nsCSSPropertyID::eCSSPropertyExtra_all_properties,
             _ => panic!("Unconvertable Servo transition property: {:?}", transition_property),
         }
     }
@@ -325,17 +319,17 @@ impl<'a> From< &'a TransitionProperty> f
 
 /// Convert nsCSSPropertyID to TransitionProperty
 #[cfg(feature = "gecko")]
 #[allow(non_upper_case_globals)]
 impl From<nsCSSPropertyID> 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:
                     ${helpers.to_nscsspropertyid(prop.ident)}
                         => TransitionProperty::Unsupported(Atom::from("${prop.ident}")),
                 % endif
             % endfor
             nsCSSPropertyID::eCSSPropertyExtra_all_properties => TransitionProperty::All,