From f8e2f85cdc63b94e6393aacd4a5e65cf43e64aa2 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:10:51 +0900 Subject: [PATCH 01/11] Drop Servo_AnimationValueMap_Push --- components/style/gecko/generated/bindings.rs | 7 ------- ports/geckolib/glue.rs | 12 ------------ 2 files changed, 19 deletions(-) diff --git a/components/style/gecko/generated/bindings.rs b/components/style/gecko/generated/bindings.rs index 38b86d87c3b..96f71df09e7 100644 --- a/components/style/gecko/generated/bindings.rs +++ b/components/style/gecko/generated/bindings.rs @@ -2238,13 +2238,6 @@ extern "C" { result: RawGeckoComputedKeyframeValuesListBorrowedMut); } -extern "C" { - pub fn Servo_AnimationValueMap_Push(arg1: - RawServoAnimationValueMapBorrowedMut, - property: nsCSSPropertyID, - value: - RawServoAnimationValueBorrowed); -} extern "C" { pub fn Servo_ComputedValues_ExtractAnimationValue(computed_values: ServoComputedValuesBorrowed, diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index b51a4e85b96..96c05f6601a 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -379,18 +379,6 @@ pub extern "C" fn Servo_AnimationValues_ComputeDistance(from: RawServoAnimationV from_value.compute_distance(to_value).unwrap_or(0.0) } -#[no_mangle] -pub extern "C" fn Servo_AnimationValueMap_Push(value_map: RawServoAnimationValueMapBorrowedMut, - property: nsCSSPropertyID, - value: RawServoAnimationValueBorrowed) -{ - use style::properties::animated_properties::AnimationValueMap; - - let value_map = AnimationValueMap::from_ffi_mut(value_map); - let value = AnimationValue::as_arc(&value).as_ref(); - value_map.insert(property.into(), value.clone()); -} - #[no_mangle] pub extern "C" fn Servo_AnimationCompose(raw_value_map: RawServoAnimationValueMapBorrowedMut, base_values: *mut ::std::os::raw::c_void, From 09c2b1b93035041287e36696ee0e42f8c3abb865 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:11:34 +0900 Subject: [PATCH 02/11] 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. --- components/style/properties/data.py | 10 ++++++++++ .../properties/helpers/animated_properties.mako.rs | 12 +----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/components/style/properties/data.py b/components/style/properties/data.py index 15d9f2fc89a..0822e25e320 100644 --- a/components/style/properties/data.py +++ b/components/style/properties/data.py @@ -234,6 +234,16 @@ class Shorthand(object): self.allowed_in_keyframe_block = allowed_in_keyframe_block \ and allowed_in_keyframe_block != "False" + def get_animatable(self): + animatable = False + for sub in self.sub_properties: + if sub.animatable: + animatable = True + break + return animatable + + animatable = property(get_animatable) + class Method(object): def __init__(self, name, return_type=None, arg_types=None, is_mut=False): diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 857c0b10491..0ac3e4704cf 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -195,21 +195,11 @@ impl TransitionProperty { #[cfg(feature = "gecko")] pub fn nscsspropertyid_is_animatable(property: nsCSSPropertyID) -> bool { match property { - % for prop in data.longhands: + % for prop in data.longhands + data.shorthands_except_all(): % if prop.animatable: ${helpers.to_nscsspropertyid(prop.ident)} => true, % endif % endfor - % for prop in data.shorthands_except_all(): - <% - animatable = "false" - for sub in prop.sub_properties: - if sub.animatable: - animatable = "true" - break - %> - ${helpers.to_nscsspropertyid(prop.ident)} => ${animatable}, - % endfor _ => false } } From c1bf6d3efcb9181e933ede2a6114c54cc520ed95 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:12:03 +0900 Subject: [PATCH 03/11] 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. --- .../helpers/animated_properties.mako.rs | 58 +++++++------------ 1 file changed, 22 insertions(+), 36 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 0ac3e4704cf..136d787532d 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -46,8 +46,8 @@ use values::generics::border::BorderCornerRadius as GenericBorderCornerRadius; use values::generics::position as generic_position; -/// A given transition property, that is either `All`, or an animatable -/// property. +/// 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. // NB: This needs to be here because it needs all the longhands generated // beforehand. #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -55,18 +55,12 @@ use values::generics::position as generic_position; pub enum TransitionProperty { /// All, any animatable property changing should generate a transition. All, - % for prop in data.longhands: + % for prop in data.longhands + data.shorthands_except_all(): % if prop.animatable: /// ${prop.name} ${prop.camel_case}, % endif % endfor - // Shorthand properties may or may not contain any animatable property. Either should still be - // parsed properly. - % for prop in data.shorthands_except_all(): - /// ${prop.name} - ${prop.camel_case}, - % endfor /// Unrecognized property which could be any non-animatable, custom property, or /// unknown property. Unsupported(Atom) @@ -104,14 +98,11 @@ impl TransitionProperty { let ident = try!(input.expect_ident()); (match_ignore_ascii_case! { &ident, "all" => Ok(TransitionProperty::All), - % for prop in data.longhands: + % for prop in data.longhands + data.shorthands_except_all(): % if prop.animatable: "${prop.name}" => Ok(TransitionProperty::${prop.camel_case}), % endif % endfor - % for prop in data.shorthands_except_all(): - "${prop.name}" => Ok(TransitionProperty::${prop.camel_case}), - % endfor "none" => Err(()), _ => { match CSSWideKeyword::from_ident(&ident) { @@ -164,17 +155,21 @@ impl TransitionProperty { /// Return animatable longhands of this shorthand TransitionProperty, except for "all". pub fn longhands(&self) -> &'static [TransitionProperty] { % for prop in data.shorthands_except_all(): - static ${prop.ident.upper()}: &'static [TransitionProperty] = &[ - % for sub in prop.sub_properties: - % if sub.animatable: - TransitionProperty::${sub.camel_case}, - % endif - % endfor - ]; + % if prop.animatable: + static ${prop.ident.upper()}: &'static [TransitionProperty] = &[ + % for sub in prop.sub_properties: + % if sub.animatable: + TransitionProperty::${sub.camel_case}, + % endif + % endfor + ]; + % endif % endfor match *self { % for prop in data.shorthands_except_all(): - TransitionProperty::${prop.camel_case} => ${prop.ident.upper()}, + % if prop.animatable: + TransitionProperty::${prop.camel_case} => ${prop.ident.upper()}, + % endif % endfor _ => panic!("Not allowed to call longhands() for this TransitionProperty") } @@ -184,7 +179,9 @@ impl TransitionProperty { pub fn is_shorthand(&self) -> bool { match *self { % for prop in data.shorthands_except_all(): - TransitionProperty::${prop.camel_case} => true, + % if prop.animatable: + TransitionProperty::${prop.camel_case} => true, + % endif % endfor _ => false } @@ -210,14 +207,11 @@ impl ToCss for TransitionProperty { { match *self { TransitionProperty::All => dest.write_str("all"), - % for prop in data.longhands: + % for prop in data.longhands + data.shorthands_except_all(): % if prop.animatable: TransitionProperty::${prop.camel_case} => dest.write_str("${prop.name}"), % endif % endfor - % for prop in data.shorthands_except_all(): - TransitionProperty::${prop.camel_case} => dest.write_str("${prop.name}"), - % endfor #[cfg(feature = "gecko")] TransitionProperty::Unsupported(ref atom) => serialize_identifier(&atom.to_string(), dest), @@ -233,16 +227,12 @@ impl ToCss for TransitionProperty { impl<'a> From< &'a TransitionProperty> for nsCSSPropertyID { fn from(transition_property: &'a TransitionProperty) -> nsCSSPropertyID { match *transition_property { - % for prop in data.longhands: + % for prop in data.longhands + data.shorthands_except_all(): % if prop.animatable: TransitionProperty::${prop.camel_case} => ${helpers.to_nscsspropertyid(prop.ident)}, % endif % endfor - % for prop in data.shorthands_except_all(): - TransitionProperty::${prop.camel_case} - => ${helpers.to_nscsspropertyid(prop.ident)}, - % endfor TransitionProperty::All => nsCSSPropertyID::eCSSPropertyExtra_all_properties, _ => panic!("Unconvertable Servo transition property: {:?}", transition_property), } @@ -255,7 +245,7 @@ impl<'a> From< &'a TransitionProperty> for nsCSSPropertyID { impl From for TransitionProperty { fn from(property: nsCSSPropertyID) -> TransitionProperty { match property { - % for prop in data.longhands: + % for prop in data.longhands + data.shorthands_except_all(): % if prop.animatable: ${helpers.to_nscsspropertyid(prop.ident)} => TransitionProperty::${prop.camel_case}, @@ -264,10 +254,6 @@ impl From for TransitionProperty { => TransitionProperty::Unsupported(Atom::from("${prop.ident}")), % endif % endfor - % for prop in data.shorthands_except_all(): - ${helpers.to_nscsspropertyid(prop.ident)} - => TransitionProperty::${prop.camel_case}, - % endfor nsCSSPropertyID::eCSSPropertyExtra_all_properties => TransitionProperty::All, _ => panic!("Unconvertable nsCSSPropertyID: {:?}", property), } From 9c3c954aa2aaf895b1c2eacea1c3b634cd11c310 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:13:04 +0900 Subject: [PATCH 04/11] 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. --- .../properties/helpers/animated_properties.mako.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 136d787532d..723c9301573 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -46,6 +46,20 @@ use values::generics::border::BorderCornerRadius as GenericBorderCornerRadius; use values::generics::position as generic_position; +/// A longhand property whose animation type is not "none". +/// +/// NOTE: This includes the 'display' property since it is animatable from SMIL even though it is +/// not animatable from CSS animations or Web Animations. CSS transitions also does not allow +/// animating 'display', but for CSS transitions we have the separate TransitionProperty type. +pub enum AnimatableLonghand { + % for prop in data.longhands: + % if prop.animatable: + /// ${prop.name} + ${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. // NB: This needs to be here because it needs all the longhands generated From 8f3dad598f623f2eab90b6c7da7ffb02eca5c610 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:18:18 +0900 Subject: [PATCH 05/11] 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. --- components/style/animation.rs | 34 +++- components/style/gecko/wrapper.rs | 8 +- .../style/properties/declaration_block.rs | 10 +- .../helpers/animated_properties.mako.rs | 150 ++++++++++++------ .../style/properties/properties.mako.rs | 18 +-- .../style/stylesheets/keyframes_rule.rs | 14 +- ports/geckolib/glue.rs | 32 ++-- tests/unit/style/keyframes.rs | 8 +- 8 files changed, 175 insertions(+), 99 deletions(-) diff --git a/components/style/animation.rs b/components/style/animation.rs index 3ee17b1872c..f8fde047bf1 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -12,7 +12,7 @@ use dom::OpaqueNode; use euclid::Point2D; use font_metrics::FontMetricsProvider; use properties::{self, CascadeFlags, ComputedValues, Importance}; -use properties::animated_properties::{AnimatedProperty, TransitionProperty}; +use properties::animated_properties::{AnimatableLonghand, AnimatedProperty, TransitionProperty}; use properties::longhands::animation_direction::computed_value::single_value::T as AnimationDirection; use properties::longhands::animation_iteration_count::single_value::computed_value::T as AnimationIterationCount; use properties::longhands::animation_play_state::computed_value::single_value::T as AnimationPlayState; @@ -330,7 +330,27 @@ impl PropertyAnimation { -> Option { debug_assert!(!transition_property.is_shorthand() && transition_property != &TransitionProperty::All); - let animated_property = AnimatedProperty::from_transition_property(transition_property, + + // We're not expecting |transition_property| to be a shorthand (including 'all') and + // all other transitionable properties should be animatable longhands (since transitionable + // is a subset of animatable). + let animatable_longhand = + AnimatableLonghand::from_transition_property(transition_property).unwrap(); + + PropertyAnimation::from_animatable_longhand(&animatable_longhand, + timing_function, + duration, + old_style, + new_style) + } + + fn from_animatable_longhand(animatable_longhand: &AnimatableLonghand, + timing_function: TimingFunction, + duration: Time, + old_style: &ComputedValues, + new_style: &ComputedValues) + -> Option { + let animated_property = AnimatedProperty::from_animatable_longhand(animatable_longhand, old_style, new_style); @@ -743,22 +763,22 @@ pub fn update_style_for_animation(context: &SharedStyleContext, let mut new_style = (*style).clone(); - for transition_property in &animation.properties_changed { + for property in &animation.properties_changed { debug!("update_style_for_animation: scanning prop {:?} for animation \"{}\"", - transition_property, name); - match PropertyAnimation::from_transition_property(transition_property, + property, name); + match PropertyAnimation::from_animatable_longhand(property, timing_function, Time::from_seconds(relative_duration as f32), &from_style, &target_style) { Some(property_animation) => { - debug!("update_style_for_animation: got property animation for prop {:?}", transition_property); + debug!("update_style_for_animation: got property animation for prop {:?}", property); debug!("update_style_for_animation: {:?}", property_animation); property_animation.update(Arc::make_mut(&mut new_style), relative_progress); } None => { debug!("update_style_for_animation: property animation {:?} not animating", - transition_property); + property); } } } diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 961ed2d45f7..539cb492adc 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -68,7 +68,8 @@ use logical_geometry::WritingMode; use media_queries::Device; use properties::{ComputedValues, parse_style_attribute}; use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock}; -use properties::animated_properties::{AnimationValue, AnimationValueMap, TransitionProperty}; +use properties::animated_properties::{AnimatableLonghand, AnimationValue, AnimationValueMap}; +use properties::animated_properties::TransitionProperty; use properties::style_structs::Font; use rule_tree::CascadeLevel as ServoCascadeLevel; use selector_parser::{AttrValue, ElementExt, PseudoClassStringArg}; @@ -1143,8 +1144,11 @@ impl<'le> TElement for GeckoElement<'le> { return existing_transitions.get(property).unwrap() != &after_value; } + // |property| should be an animatable longhand + let animatable_longhand = AnimatableLonghand::from_transition_property(property).unwrap(); + combined_duration > 0.0f32 && - AnimatedProperty::from_transition_property(property, + AnimatedProperty::from_animatable_longhand(&animatable_longhand, before_change_style, after_change_style).does_animate() } diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index efbccd9f991..bcbb26771a9 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -126,7 +126,7 @@ impl<'a, 'cx, 'cx_a:'cx> AnimationValueIterator<'a, 'cx, 'cx_a> { } impl<'a, 'cx, 'cx_a:'cx> Iterator for AnimationValueIterator<'a, 'cx, 'cx_a> { - type Item = (TransitionProperty, AnimationValue); + type Item = (AnimatableLonghand, AnimationValue); #[inline] fn next(&mut self) -> Option { use properties::Importance; @@ -136,11 +136,11 @@ impl<'a, 'cx, 'cx_a:'cx> Iterator for AnimationValueIterator<'a, 'cx, 'cx_a> { match next { Some(&(ref decl, importance)) => { if importance == Importance::Normal { - let property = TransitionProperty::from_declaration(decl); + let property = AnimatableLonghand::from_declaration(decl); let animation = AnimationValue::from_declaration(decl, &mut self.context, self.default_values); debug_assert!(property.is_none() == animation.is_none(), - "The failure condition of TransitionProperty::from_declaration \ + "The failure condition of AnimatableLonghand::from_declaration \ and AnimationValue::from_declaration should be the same"); // Skip the property if either ::from_declaration fails. match (property, animation) { @@ -201,7 +201,7 @@ impl PropertyDeclarationBlock { } } - /// Return an iterator of (TransitionProperty, AnimationValue). + /// Return an iterator of (AnimatableLonghand, AnimationValue). pub fn to_animation_value_iter<'a, 'cx, 'cx_a:'cx>(&'a self, context: &'cx mut Context<'cx_a>, default_values: &'a Arc) @@ -554,7 +554,7 @@ impl PropertyDeclarationBlock { let mut longhands = LonghandIdSet::new(); for (property, animation_value) in animation_value_map.iter() { - longhands.set_transition_property_bit(property); + longhands.set_animatable_longhand_bit(property); declarations.push((animation_value.uncompute(), Importance::Normal)); } diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 723c9301573..1f03bd4c071 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -51,6 +51,8 @@ use values::generics::position as generic_position; /// NOTE: This includes the 'display' property since it is animatable from SMIL even though it is /// not animatable from CSS animations or Web Animations. CSS transitions also does not allow /// animating 'display', but for CSS transitions we have the separate TransitionProperty type. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +#[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub enum AnimatableLonghand { % for prop in data.longhands: % if prop.animatable: @@ -60,6 +62,101 @@ pub enum AnimatableLonghand { % endfor } +impl AnimatableLonghand { + /// Converts from an nsCSSPropertyID. Returns None if nsCSSPropertyID is not an animatable + /// longhand in Servo. + #[cfg(feature = "gecko")] + pub fn from_nscsspropertyid(css_property: nsCSSPropertyID) -> Option { + match css_property { + % for prop in data.longhands: + % if prop.animatable: + ${helpers.to_nscsspropertyid(prop.ident)} + => Some(AnimatableLonghand::${prop.camel_case}), + % endif + % endfor + _ => None + } + } + + /// Converts from TransitionProperty. Returns None if the property is not an animatable + /// longhand. + 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: + TransitionProperty::${prop.camel_case} + => Some(AnimatableLonghand::${prop.camel_case}), + % endif + % endfor + _ => None + } + } + + /// Get an animatable longhand property from a property declaration. + pub fn from_declaration(declaration: &PropertyDeclaration) -> Option { + use properties::LonghandId; + match *declaration { + % for prop in data.longhands: + % if prop.animatable: + PropertyDeclaration::${prop.camel_case}(..) + => Some(AnimatableLonghand::${prop.camel_case}), + % endif + % endfor + PropertyDeclaration::CSSWideKeyword(id, _) | + PropertyDeclaration::WithVariables(id, _) => { + match id { + % for prop in data.longhands: + % if prop.animatable: + LonghandId::${prop.camel_case} => + Some(AnimatableLonghand::${prop.camel_case}), + % endif + % endfor + _ => None, + } + }, + _ => None, + } + } +} + +/// Convert to nsCSSPropertyID. +#[cfg(feature = "gecko")] +#[allow(non_upper_case_globals)] +impl<'a> From< &'a AnimatableLonghand> for nsCSSPropertyID { + fn from(property: &'a AnimatableLonghand) -> nsCSSPropertyID { + match *property { + % for prop in data.longhands: + % if prop.animatable: + AnimatableLonghand::${prop.camel_case} + => ${helpers.to_nscsspropertyid(prop.ident)}, + % endif + % endfor + } + } +} + +/// Convert to PropertyDeclarationId. +#[cfg(feature = "gecko")] +#[allow(non_upper_case_globals)] +impl<'a> From for PropertyDeclarationId<'a> { + fn from(property: AnimatableLonghand) -> PropertyDeclarationId<'a> { + match property { + % for prop in data.longhands: + % if prop.animatable: + 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. // NB: This needs to be here because it needs all the longhands generated @@ -127,32 +224,6 @@ impl TransitionProperty { }).map_err(|()| SelectorParseError::UnexpectedIdent(ident.into()).into()) } - /// Get a transition property from a property declaration. - pub fn from_declaration(declaration: &PropertyDeclaration) -> Option { - use properties::LonghandId; - match *declaration { - % for prop in data.longhands: - % if prop.animatable: - PropertyDeclaration::${prop.camel_case}(..) - => Some(TransitionProperty::${prop.camel_case}), - % endif - % endfor - PropertyDeclaration::CSSWideKeyword(id, _) | - PropertyDeclaration::WithVariables(id, _) => { - match id { - % for prop in data.longhands: - % if prop.animatable: - LonghandId::${prop.camel_case} => - Some(TransitionProperty::${prop.camel_case}), - % endif - % endfor - _ => None, - } - }, - _ => None, - } - } - /// Returns true if this TransitionProperty is one of the discrete animatable properties and /// this TransitionProperty should be a longhand property. pub fn is_discrete(&self) -> bool { @@ -274,23 +345,6 @@ impl From for TransitionProperty { } } -/// Convert to PropertyDeclarationId. -#[cfg(feature = "gecko")] -#[allow(non_upper_case_globals)] -impl<'a> From for PropertyDeclarationId<'a> { - fn from(transition_property: TransitionProperty) -> PropertyDeclarationId<'a> { - match transition_property { - % for prop in data.longhands: - % if prop.animatable: - TransitionProperty::${prop.camel_case} - => PropertyDeclarationId::Longhand(LonghandId::${prop.camel_case}), - % endif - % endfor - _ => panic!(), - } - } -} - /// An animated property interpolation between two computed values for that /// property. #[derive(Clone, Debug, PartialEq)] @@ -377,22 +431,20 @@ impl AnimatedProperty { /// Get an animatable value from a transition-property, an old style, and a /// new style. - pub fn from_transition_property(transition_property: &TransitionProperty, + pub fn from_animatable_longhand(property: &AnimatableLonghand, old_style: &ComputedValues, new_style: &ComputedValues) -> AnimatedProperty { - match *transition_property { - TransitionProperty::All => panic!("Can't use TransitionProperty::All here."), + match *property { % for prop in data.longhands: % if prop.animatable: - TransitionProperty::${prop.camel_case} => { + AnimatableLonghand::${prop.camel_case} => { AnimatedProperty::${prop.camel_case}( old_style.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}().into(), new_style.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}().into()) } % endif % endfor - ref other => panic!("Can't use TransitionProperty::{:?} here", other), } } } @@ -401,7 +453,7 @@ impl AnimatedProperty { /// This HashMap stores the values that are the last AnimationValue to be /// composed for each TransitionProperty. #[cfg(feature = "gecko")] -pub type AnimationValueMap = HashMap; +pub type AnimationValueMap = HashMap; #[cfg(feature = "gecko")] unsafe impl HasFFI for AnimationValueMap { type FFIType = RawServoAnimationValueMap; diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index d307a3833c5..72af0a2d6d7 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -32,7 +32,7 @@ use font_metrics::FontMetricsProvider; use logical_geometry::WritingMode; use media_queries::Device; use parser::{Parse, ParserContext}; -use properties::animated_properties::TransitionProperty; +use properties::animated_properties::AnimatableLonghand; #[cfg(feature = "gecko")] use properties::longhands::system_font::SystemFont; use selectors::parser::SelectorParseError; #[cfg(feature = "servo")] use servo_config::prefs::PREFS; @@ -300,29 +300,25 @@ impl LonghandIdSet { } } - /// Set the corresponding bit of TransitionProperty. - /// This function will panic if TransitionProperty::All is given. - pub fn set_transition_property_bit(&mut self, property: &TransitionProperty) { + /// Set the corresponding bit of AnimatableLonghand. + pub fn set_animatable_longhand_bit(&mut self, property: &AnimatableLonghand) { match *property { % for prop in data.longhands: % if prop.animatable: - TransitionProperty::${prop.camel_case} => self.insert(LonghandId::${prop.camel_case}), + AnimatableLonghand::${prop.camel_case} => self.insert(LonghandId::${prop.camel_case}), % endif % endfor - ref other => unreachable!("Tried to set TransitionProperty::{:?} in a PropertyBitfield", other), } } - /// Return true if the corresponding bit of TransitionProperty is set. - /// This function will panic if TransitionProperty::All is given. - pub fn has_transition_property_bit(&self, property: &TransitionProperty) -> bool { + /// Return true if the corresponding bit of AnimatableLonghand is set. + pub fn has_animatable_longhand_bit(&self, property: &AnimatableLonghand) -> bool { match *property { % for prop in data.longhands: % if prop.animatable: - TransitionProperty::${prop.camel_case} => self.contains(LonghandId::${prop.camel_case}), + AnimatableLonghand::${prop.camel_case} => self.contains(LonghandId::${prop.camel_case}), % endif % endfor - ref other => unreachable!("Tried to get TransitionProperty::{:?} in a PropertyBitfield", other), } } } diff --git a/components/style/stylesheets/keyframes_rule.rs b/components/style/stylesheets/keyframes_rule.rs index c03240ecafc..dcb8902ae2a 100644 --- a/components/style/stylesheets/keyframes_rule.rs +++ b/components/style/stylesheets/keyframes_rule.rs @@ -11,7 +11,7 @@ use parser::{ParserContext, log_css_error}; use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId}; use properties::{PropertyDeclarationId, LonghandId, SourcePropertyDeclaration}; use properties::LonghandIdSet; -use properties::animated_properties::TransitionProperty; +use properties::animated_properties::AnimatableLonghand; use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction; use selectors::parser::SelectorParseError; use shared_lock::{DeepCloneWithLock, SharedRwLock, SharedRwLockReadGuard, Locked, ToCssWithGuard}; @@ -337,14 +337,14 @@ pub struct KeyframesAnimation { /// The difference steps of the animation. pub steps: Vec, /// The properties that change in this animation. - pub properties_changed: Vec, + pub properties_changed: Vec, /// Vendor prefix type the @keyframes has. pub vendor_prefix: Option, } /// Get all the animated properties in a keyframes animation. fn get_animated_properties(keyframes: &[Arc>], guard: &SharedRwLockReadGuard) - -> Vec { + -> Vec { let mut ret = vec![]; let mut seen = LonghandIdSet::new(); // NB: declarations are already deduplicated, so we don't have to check for @@ -355,9 +355,9 @@ fn get_animated_properties(keyframes: &[Arc>], guard: &SharedRw for &(ref declaration, importance) in block.declarations().iter() { assert!(!importance.important()); - if let Some(property) = TransitionProperty::from_declaration(declaration) { - if !seen.has_transition_property_bit(&property) { - seen.set_transition_property_bit(&property); + if let Some(property) = AnimatableLonghand::from_declaration(declaration) { + if !seen.has_animatable_longhand_bit(&property) { + seen.set_animatable_longhand_bit(&property); ret.push(property); } } @@ -372,7 +372,7 @@ impl KeyframesAnimation { /// /// This will return a keyframe animation with empty steps and /// properties_changed if the list of keyframes is empty, or there are no - // animated properties obtained from the keyframes. + /// animated properties obtained from the keyframes. /// /// Otherwise, this will compute and sort the steps used for the animation, /// and return the animation object. diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 96c05f6601a..81445b52022 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -95,7 +95,8 @@ use style::parser::ParserContext; use style::properties::{CascadeFlags, ComputedValues, Importance, SourcePropertyDeclaration}; use style::properties::{LonghandIdSet, PropertyDeclaration, PropertyDeclarationBlock, PropertyId, StyleBuilder}; use style::properties::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP; -use style::properties::animated_properties::{Animatable, AnimationValue, TransitionProperty}; +use style::properties::animated_properties::{Animatable, AnimatableLonghand, AnimationValue}; +use style::properties::animated_properties::TransitionProperty; use style::properties::parse_one_declaration_into; use style::rule_tree::StyleSource; use style::selector_parser::PseudoElementCascadeType; @@ -393,7 +394,10 @@ pub extern "C" fn Servo_AnimationCompose(raw_value_map: RawServoAnimationValueMa use style::gecko_bindings::bindings::Gecko_GetProgressFromComputedTiming; use style::properties::animated_properties::AnimationValueMap; - let property: TransitionProperty = css_property.into(); + let property = match AnimatableLonghand::from_nscsspropertyid(css_property) { + Some(longhand) => longhand, + None => { return (); } + }; let value_map = AnimationValueMap::from_ffi_mut(raw_value_map); // We will need an underlying value if either of the endpoints is null... @@ -2683,10 +2687,10 @@ pub extern "C" fn Servo_GetComputedKeyframeValues(keyframes: RawGeckoKeyframeLis let guard = declarations.read_with(&guard); for anim in guard.to_animation_value_iter(&mut context, &default_values) { - if !seen.has_transition_property_bit(&anim.0) { + if !seen.has_animatable_longhand_bit(&anim.0) { // This is safe since we immediately write to the uninitialized values. unsafe { animation_values.set_len((property_index + 1) as u32) }; - seen.set_transition_property_bit(&anim.0); + seen.set_animatable_longhand_bit(&anim.0); animation_values[property_index].mProperty = (&anim.0).into(); // We only make sure we have enough space for this variable, // but didn't construct a default value for StyleAnimationValue, @@ -2783,7 +2787,7 @@ pub extern "C" fn Servo_AssertTreeIsClean(root: RawGeckoElementBorrowed) { fn append_computed_property_value(keyframe: *mut structs::Keyframe, style: &ComputedValues, - property: &TransitionProperty, + property: &AnimatableLonghand, shared_lock: &SharedRwLock) { let block = style.to_declaration_block(property.clone().into()); unsafe { @@ -2802,7 +2806,7 @@ enum Offset { One } -fn fill_in_missing_keyframe_values(all_properties: &[TransitionProperty], +fn fill_in_missing_keyframe_values(all_properties: &[AnimatableLonghand], timing_function: nsTimingFunctionBorrowed, style: &ComputedValues, properties_set_at_offset: &LonghandIdSet, @@ -2810,7 +2814,7 @@ fn fill_in_missing_keyframe_values(all_properties: &[TransitionProperty], keyframes: RawGeckoKeyframeListBorrowedMut, shared_lock: &SharedRwLock) { let needs_filling = all_properties.iter().any(|ref property| { - !properties_set_at_offset.has_transition_property_bit(property) + !properties_set_at_offset.has_animatable_longhand_bit(property) }); // Return earli if all animated properties are already set. @@ -2829,7 +2833,7 @@ fn fill_in_missing_keyframe_values(all_properties: &[TransitionProperty], // Append properties that have not been set at this offset. for ref property in all_properties.iter() { - if !properties_set_at_offset.has_transition_property_bit(property) { + if !properties_set_at_offset.has_animatable_longhand_bit(property) { append_computed_property_value(keyframe, style, property, @@ -2921,17 +2925,17 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(raw_data: RawServoStyleSetB let mut index = unsafe { (*keyframe).mPropertyValues.len() }; for &(ref declaration, _) in animatable { - let property = TransitionProperty::from_declaration(declaration).unwrap(); - if !properties_set_at_current_offset.has_transition_property_bit(&property) { - properties_set_at_current_offset.set_transition_property_bit(&property); + let property = AnimatableLonghand::from_declaration(declaration).unwrap(); + if !properties_set_at_current_offset.has_animatable_longhand_bit(&property) { + properties_set_at_current_offset.set_animatable_longhand_bit(&property); if current_offset == 0.0 { - properties_set_at_start.set_transition_property_bit(&property); + properties_set_at_start.set_animatable_longhand_bit(&property); } else if current_offset == 1.0 { - properties_set_at_end.set_transition_property_bit(&property); + properties_set_at_end.set_animatable_longhand_bit(&property); } unsafe { - let property = TransitionProperty::from_declaration(declaration).unwrap(); + let property = AnimatableLonghand::from_declaration(declaration).unwrap(); (*keyframe).mPropertyValues.set_len((index + 1) as u32); (*keyframe).mPropertyValues[index].mProperty = (&property).into(); (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky( diff --git a/tests/unit/style/keyframes.rs b/tests/unit/style/keyframes.rs index 8f179fc4999..b546b28ad53 100644 --- a/tests/unit/style/keyframes.rs +++ b/tests/unit/style/keyframes.rs @@ -3,7 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use style::properties::{PropertyDeclaration, PropertyDeclarationBlock, Importance}; -use style::properties::animated_properties::TransitionProperty; +use style::properties::animated_properties::AnimatableLonghand; use style::shared_lock::SharedRwLock; use style::stylearc::Arc; use style::stylesheets::keyframes_rule::{Keyframe, KeyframesAnimation, KeyframePercentage, KeyframeSelector}; @@ -100,7 +100,7 @@ fn test_missing_property_in_initial_keyframe() { declared_timing_function: false, }, ], - properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height], + properties_changed: vec![AnimatableLonghand::Width, AnimatableLonghand::Height], vendor_prefix: None, }; @@ -160,7 +160,7 @@ fn test_missing_property_in_final_keyframe() { declared_timing_function: false, }, ], - properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height], + properties_changed: vec![AnimatableLonghand::Width, AnimatableLonghand::Height], vendor_prefix: None, }; @@ -222,7 +222,7 @@ fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() { declared_timing_function: false, } ], - properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height], + properties_changed: vec![AnimatableLonghand::Width, AnimatableLonghand::Height], vendor_prefix: None, }; From a2307adf46e92c2364f50110aa88a206747b900d Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:18:55 +0900 Subject: [PATCH 06/11] Convert AnimationValue::from_computed_values to take an AnimatableLonghand --- components/style/gecko/wrapper.rs | 9 +++++---- .../properties/helpers/animated_properties.mako.rs | 10 ++++------ ports/geckolib/glue.rs | 7 ++++++- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 539cb492adc..f1e84c2129c 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1135,18 +1135,19 @@ impl<'le> TElement for GeckoElement<'le> { return false; } + // |property| should be an animatable longhand + let animatable_longhand = AnimatableLonghand::from_transition_property(property).unwrap(); + 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(property, after_change_style)); + Arc::new(AnimationValue::from_computed_values(&animatable_longhand, + after_change_style)); return existing_transitions.get(property).unwrap() != &after_value; } - // |property| should be an animatable longhand - let animatable_longhand = AnimatableLonghand::from_transition_property(property).unwrap(); - combined_duration > 0.0f32 && AnimatedProperty::from_animatable_longhand(&animatable_longhand, before_change_style, diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 1f03bd4c071..57b5cd1138e 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -621,15 +621,14 @@ impl AnimationValue { } } - /// Get an AnimationValue for a TransitionProperty from a given computed values. - pub fn from_computed_values(transition_property: &TransitionProperty, + /// Get an AnimationValue for an AnimatableLonghand from a given computed values. + pub fn from_computed_values(property: &AnimatableLonghand, computed_values: &ComputedValues) -> Self { - match *transition_property { - TransitionProperty::All => panic!("Can't use TransitionProperty::All here."), + match *property { % for prop in data.longhands: % if prop.animatable: - TransitionProperty::${prop.camel_case} => { + AnimatableLonghand::${prop.camel_case} => { AnimationValue::${prop.camel_case}( % if prop.is_animatable_with_computed_value: computed_values.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}()) @@ -640,7 +639,6 @@ impl AnimationValue { } % endif % endfor - ref other => panic!("Can't use TransitionProperty::{:?} here.", other), } } } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 81445b52022..dc9d7417266 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -688,8 +688,13 @@ pub extern "C" fn Servo_ComputedValues_ExtractAnimationValue(computed_values: Se property_id: nsCSSPropertyID) -> RawServoAnimationValueStrong { + let property = match AnimatableLonghand::from_nscsspropertyid(property_id) { + Some(longhand) => longhand, + None => { return Strong::null(); } + }; + let computed_values = ComputedValues::as_arc(&computed_values); - Arc::new(AnimationValue::from_computed_values(&property_id.into(), computed_values)).into_strong() + Arc::new(AnimationValue::from_computed_values(&property, computed_values)).into_strong() } #[no_mangle] From 5ce7b1cc5576ceb3286cacf0f9d2a9dd87984925 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:19:22 +0900 Subject: [PATCH 07/11] Move is_discrete from TransitionProperty to AnimatableLonghand --- components/style/gecko/wrapper.rs | 10 ++++---- .../helpers/animated_properties.mako.rs | 25 +++++++++---------- ports/geckolib/glue.rs | 7 +++--- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index f1e84c2129c..8aa229f7d26 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1130,14 +1130,14 @@ impl<'le> TElement for GeckoElement<'le> { -> bool { use properties::animated_properties::AnimatedProperty; - // We don't allow transitions on properties that are not interpolable. - if property.is_discrete() { - return false; - } - // |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/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 57b5cd1138e..315673c591f 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -63,6 +63,18 @@ pub enum AnimatableLonghand { } impl AnimatableLonghand { + /// Returns true if this AnimatableLonghand is one of the discretely animatable properties. + pub fn is_discrete(&self) -> bool { + match *self { + % for prop in data.longhands: + % if prop.animation_value_type == "discrete": + AnimatableLonghand::${prop.camel_case} => true, + % endif + % endfor + _ => false + } + } + /// Converts from an nsCSSPropertyID. Returns None if nsCSSPropertyID is not an animatable /// longhand in Servo. #[cfg(feature = "gecko")] @@ -224,19 +236,6 @@ impl TransitionProperty { }).map_err(|()| SelectorParseError::UnexpectedIdent(ident.into()).into()) } - /// Returns true if this TransitionProperty is one of the discrete animatable properties and - /// this TransitionProperty should be a longhand property. - pub fn is_discrete(&self) -> bool { - match *self { - % for prop in data.longhands: - % if prop.animation_value_type == "discrete": - TransitionProperty::${prop.camel_case} => true, - % endif - % endfor - _ => false - } - } - /// Return animatable longhands of this shorthand TransitionProperty, except for "all". pub fn longhands(&self) -> &'static [TransitionProperty] { % for prop in data.shorthands_except_all(): diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index dc9d7417266..4f997459cf2 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -96,7 +96,6 @@ use style::properties::{CascadeFlags, ComputedValues, Importance, SourceProperty use style::properties::{LonghandIdSet, PropertyDeclaration, PropertyDeclarationBlock, PropertyId, StyleBuilder}; use style::properties::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP; use style::properties::animated_properties::{Animatable, AnimatableLonghand, AnimationValue}; -use style::properties::animated_properties::TransitionProperty; use style::properties::parse_one_declaration_into; use style::rule_tree::StyleSource; use style::selector_parser::PseudoElementCascadeType; @@ -705,8 +704,10 @@ pub extern "C" fn Servo_Property_IsAnimatable(property: nsCSSPropertyID) -> bool #[no_mangle] pub extern "C" fn Servo_Property_IsDiscreteAnimatable(property: nsCSSPropertyID) -> bool { - let property: TransitionProperty = property.into(); - property.is_discrete() + match AnimatableLonghand::from_nscsspropertyid(property) { + Some(longhand) => longhand.is_discrete(), + None => false + } } #[no_mangle] From e74f7792f5550cd74def97fd789cb94f06ce887a Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:19:48 +0900 Subject: [PATCH 08/11] 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: From 479c3e452848c34a11a8a7918f71d54e487609f5 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:20:16 +0900 Subject: [PATCH 09/11] 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. --- .../helpers/animated_properties.mako.rs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index e6a1b8985ba..f8695e000fc 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -163,6 +163,19 @@ impl<'a> From for PropertyDeclarationId<'a> { } } +/// Returns true if this nsCSSPropertyID is one of the animatable properties. +#[cfg(feature = "gecko")] +pub fn nscsspropertyid_is_animatable(property: nsCSSPropertyID) -> bool { + match property { + % for prop in data.longhands + data.shorthands_except_all(): + % if prop.animatable: + ${helpers.to_nscsspropertyid(prop.ident)} => true, + % endif + % endfor + _ => false + } +} + /// 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 @@ -266,19 +279,6 @@ impl TransitionProperty { } } -/// Returns true if this nsCSSPropertyID is one of the animatable properties. -#[cfg(feature = "gecko")] -pub fn nscsspropertyid_is_animatable(property: nsCSSPropertyID) -> bool { - match property { - % for prop in data.longhands + data.shorthands_except_all(): - % if prop.animatable: - ${helpers.to_nscsspropertyid(prop.ident)} => true, - % endif - % endfor - _ => false - } -} - impl ToCss for TransitionProperty { fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write, From d249c61c0d9a2603748f9389764cde52df7eafb2 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:20:46 +0900 Subject: [PATCH 10/11] Add IsTransitionable FFI --- components/style/gecko/generated/bindings.rs | 3 +++ .../properties/helpers/animated_properties.mako.rs | 13 +++++++++++++ ports/geckolib/glue.rs | 6 ++++++ 3 files changed, 22 insertions(+) diff --git a/components/style/gecko/generated/bindings.rs b/components/style/gecko/generated/bindings.rs index 96f71df09e7..8ccd98dd2cc 100644 --- a/components/style/gecko/generated/bindings.rs +++ b/components/style/gecko/generated/bindings.rs @@ -2248,6 +2248,9 @@ extern "C" { extern "C" { pub fn Servo_Property_IsAnimatable(property: nsCSSPropertyID) -> bool; } +extern "C" { + pub fn Servo_Property_IsTransitionable(property: nsCSSPropertyID) -> bool; +} extern "C" { pub fn Servo_Property_IsDiscreteAnimatable(property: nsCSSPropertyID) -> bool; diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index f8695e000fc..dfc55107435 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -338,6 +338,19 @@ impl From for TransitionProperty { } } +/// Returns true if this nsCSSPropertyID is one of the transitionable properties. +#[cfg(feature = "gecko")] +pub fn nscsspropertyid_is_transitionable(property: nsCSSPropertyID) -> bool { + match property { + % for prop in data.longhands + data.shorthands_except_all(): + % if prop.transitionable: + ${helpers.to_nscsspropertyid(prop.ident)} => true, + % endif + % endfor + _ => false + } +} + /// An animated property interpolation between two computed values for that /// property. #[derive(Clone, Debug, PartialEq)] diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 4f997459cf2..50cb50f81ed 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -702,6 +702,12 @@ pub extern "C" fn Servo_Property_IsAnimatable(property: nsCSSPropertyID) -> bool animated_properties::nscsspropertyid_is_animatable(property) } +#[no_mangle] +pub extern "C" fn Servo_Property_IsTransitionable(property: nsCSSPropertyID) -> bool { + use style::properties::animated_properties; + animated_properties::nscsspropertyid_is_transitionable(property) +} + #[no_mangle] pub extern "C" fn Servo_Property_IsDiscreteAnimatable(property: nsCSSPropertyID) -> bool { match AnimatableLonghand::from_nscsspropertyid(property) { From e6bd75070270803443d1395a4261fd4907788566 Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 15 Jun 2017 10:21:08 +0900 Subject: [PATCH 11/11] Make 'display' animatable But not by CSS animations or Web Animations. --- components/style/properties/longhand/box.mako.rs | 2 +- components/style/stylesheets/keyframes_rule.rs | 5 ++++- ports/geckolib/glue.rs | 5 ++++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/components/style/properties/longhand/box.mako.rs b/components/style/properties/longhand/box.mako.rs index b3ada208939..551b10f96ae 100644 --- a/components/style/properties/longhand/box.mako.rs +++ b/components/style/properties/longhand/box.mako.rs @@ -12,7 +12,7 @@ // TODO(SimonSapin): don't parse `inline-table`, since we don't support it <%helpers:longhand name="display" need_clone="True" - animation_value_type="none" + animation_value_type="discrete" custom_cascade="${product == 'servo'}" spec="https://drafts.csswg.org/css-display/#propdef-display"> <% diff --git a/components/style/stylesheets/keyframes_rule.rs b/components/style/stylesheets/keyframes_rule.rs index dcb8902ae2a..da5311f3da7 100644 --- a/components/style/stylesheets/keyframes_rule.rs +++ b/components/style/stylesheets/keyframes_rule.rs @@ -356,7 +356,10 @@ fn get_animated_properties(keyframes: &[Arc>], guard: &SharedRw assert!(!importance.important()); if let Some(property) = AnimatableLonghand::from_declaration(declaration) { - if !seen.has_animatable_longhand_bit(&property) { + // Skip the 'display' property because although it is animatable from SMIL, + // it should not be animatable from CSS Animations or Web Animations. + if property != AnimatableLonghand::Display && + !seen.has_animatable_longhand_bit(&property) { seen.set_animatable_longhand_bit(&property); ret.push(property); } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 50cb50f81ed..19950db47cf 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -2938,7 +2938,10 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(raw_data: RawServoStyleSetB let mut index = unsafe { (*keyframe).mPropertyValues.len() }; for &(ref declaration, _) in animatable { let property = AnimatableLonghand::from_declaration(declaration).unwrap(); - if !properties_set_at_current_offset.has_animatable_longhand_bit(&property) { + // Skip the 'display' property because although it is animatable from SMIL, + // it should not be animatable from CSS Animations. + if property != AnimatableLonghand::Display && + !properties_set_at_current_offset.has_animatable_longhand_bit(&property) { properties_set_at_current_offset.set_animatable_longhand_bit(&property); if current_offset == 0.0 { properties_set_at_start.set_animatable_longhand_bit(&property);