From a06ce4a6aaa1e4027ecc5cb930fdbfbcef7c28be Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Fri, 14 Apr 2017 14:56:04 +0800 Subject: [PATCH 1/4] Bug 1353628 - Part 1: Add shorthand properties into TransitionProperty. In order to make TransitionProperty::parse() also work on shorthands, we should add shorthands into TransitionProperty, and add the arms in other functions which match TransitionProperty. MozReview-Commit-ID: KFd26KcQf3N --- components/style/gecko/wrapper.rs | 20 +++++--- .../helpers/animated_properties.mako.rs | 48 +++++++++++++++++-- .../style/properties/properties.mako.rs | 4 +- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index f64703dfd9c..b598bb0a799 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -775,7 +775,7 @@ impl<'le> TElement for GeckoElement<'le> { after_change_style: &Arc, pseudo: Option<&PseudoElement>) -> bool { use gecko_bindings::structs::nsCSSPropertyID; - use properties::animated_properties; + use properties::{PropertyId, animated_properties}; use std::collections::HashSet; debug_assert!(self.might_need_transitions_update(&Some(before_change_style), @@ -825,16 +825,24 @@ impl<'le> TElement for GeckoElement<'le> { } false }; - // FIXME: Bug 1353628: Shorthand properties are parsed failed now, so after fixing - // that, we have to handle shorthand. if property == nsCSSPropertyID::eCSSPropertyExtra_all_properties { if TransitionProperty::any(property_check_helper) { return true; } } else { - if animated_properties::nscsspropertyid_is_animatable(property) && - property_check_helper(property.into()) { - return true; + 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(); + 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()) { + return true; + } } } } diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 7c6031ca09c..721b858e59b 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -54,10 +54,16 @@ pub enum TransitionProperty { ${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 } impl TransitionProperty { - /// Iterates over each property that is not `All`. + /// Iterates over each longhand property. pub fn each ()>(mut cb: F) { % for prop in data.longhands: % if prop.animatable: @@ -88,6 +94,9 @@ impl TransitionProperty { "${prop.name}" => Ok(TransitionProperty::${prop.camel_case}), % endif % endfor + % for prop in data.shorthands_except_all(): + "${prop.name}" => Ok(TransitionProperty::${prop.camel_case}), + % endfor _ => Err(()) } } @@ -118,7 +127,8 @@ impl TransitionProperty { } } - /// Returns true if this TransitionProperty is one of the discrete animatable properties. + /// 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: @@ -129,6 +139,25 @@ impl TransitionProperty { _ => false } } + + /// 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 + ]; + % endfor + match *self { + % for prop in data.shorthands_except_all(): + TransitionProperty::${prop.camel_case} => ${prop.ident.upper()}, + % endfor + _ => panic!("Not allowed to call longhands() for this TransitionProperty") + } + } } /// Returns true if this nsCSSPropertyID is one of the animatable properties. @@ -155,6 +184,9 @@ impl ToCss for TransitionProperty { 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 } } } @@ -171,6 +203,10 @@ impl From for nsCSSPropertyID { => ${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, } } @@ -188,6 +224,10 @@ impl From for TransitionProperty { => TransitionProperty::${prop.camel_case}, % 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!("Unsupported Servo transition property: {:?}", property), } @@ -206,7 +246,7 @@ impl<'a> From for PropertyDeclarationId<'a> { => PropertyDeclarationId::Longhand(LonghandId::${prop.camel_case}), % endif % endfor - TransitionProperty::All => panic!(), + _ => panic!(), } } } @@ -304,6 +344,7 @@ impl AnimatedProperty { } % endif % endfor + other => panic!("Can't use TransitionProperty::{:?} here", other), } } } @@ -459,6 +500,7 @@ impl AnimationValue { } % endif % endfor + other => panic!("Can't use TransitionProperty::{:?} here.", other), } } } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index d3d4d807659..767bb39385c 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -257,7 +257,7 @@ impl LonghandIdSet { TransitionProperty::${prop.camel_case} => self.insert(LonghandId::${prop.camel_case}), % endif % endfor - TransitionProperty::All => unreachable!("Tried to set TransitionProperty::All in a PropertyBitfield"), + other => unreachable!("Tried to set TransitionProperty::{:?} in a PropertyBitfield", other), } } @@ -270,7 +270,7 @@ impl LonghandIdSet { TransitionProperty::${prop.camel_case} => self.contains(LonghandId::${prop.camel_case}), % endif % endfor - TransitionProperty::All => unreachable!("Tried to get TransitionProperty::All in a PropertyBitfield"), + other => unreachable!("Tried to get TransitionProperty::{:?} in a PropertyBitfield", other), } } } From 3a1421491b8eafc893d69a6d0758bec69d823507 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Tue, 18 Apr 2017 18:12:25 +0800 Subject: [PATCH 2/4] Bug 1353628 - Part 2: Add unit tests of parsing transition properties. MozReview-Commit-ID: K5thA20ORPk --- tests/unit/style/parsing/mod.rs | 1 + .../unit/style/parsing/transition_property.rs | 67 +++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 tests/unit/style/parsing/transition_property.rs diff --git a/tests/unit/style/parsing/mod.rs b/tests/unit/style/parsing/mod.rs index bf641e625d6..79efb4a3484 100644 --- a/tests/unit/style/parsing/mod.rs +++ b/tests/unit/style/parsing/mod.rs @@ -99,6 +99,7 @@ mod selectors; mod supports; mod text; mod text_overflow; +mod transition_property; mod transition_timing_function; mod ui; mod value; diff --git a/tests/unit/style/parsing/transition_property.rs b/tests/unit/style/parsing/transition_property.rs new file mode 100644 index 00000000000..5283f173afc --- /dev/null +++ b/tests/unit/style/parsing/transition_property.rs @@ -0,0 +1,67 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use parsing::parse; +use style::properties::animated_properties::TransitionProperty; +use style::properties::longhands::transition_property; +use style::properties::shorthands::transition; +use style_traits::ToCss; + +#[test] +fn test_longhand_properties() { + assert_roundtrip_with_context!(transition_property::parse, "margin-left"); + assert_roundtrip_with_context!(transition_property::parse, "background-color"); + assert_roundtrip_with_context!(transition_property::parse, "width"); + + assert_eq!(parse_longhand!(transition_property, "margin-left, width"), + transition_property::SpecifiedValue( + vec![TransitionProperty::MarginLeft, + TransitionProperty::Width])); + + // TODO: If one of the identifiers listed is not a recognized property name or is not an + // animatable property, the implementation must still start transitions on the animatable + // properties. Therefore, the parser shouldn't return Err for non-animatable property. + assert!(parse(transition_property::parse, "transition-duration").is_err()); +} + +#[test] +fn test_shorthand_properties() { + assert_roundtrip_with_context!(transition_property::parse, "margin"); + assert_roundtrip_with_context!(transition_property::parse, "background"); + assert_roundtrip_with_context!(transition_property::parse, "border-bottom"); + + assert_eq!(parse_longhand!(transition_property, "margin, background"), + transition_property::SpecifiedValue( + vec![TransitionProperty::Margin, + TransitionProperty::Background])); +} + +#[test] +fn test_keywords() { + assert_roundtrip_with_context!(transition_property::parse, "all"); + assert_roundtrip_with_context!(transition_property::parse, "none"); + + assert_eq!(parse_longhand!(transition_property, "all"), + transition_property::SpecifiedValue(vec![TransitionProperty::All])); + assert_eq!(parse_longhand!(transition_property, "none"), + transition_property::SpecifiedValue(vec![])); + + assert!(parse(transition_property::parse, "inherit").is_err()); + assert!(parse(transition_property::parse, "initial").is_err()); +} + +#[test] +fn test_transition_shorthand() { + let result = parse(transition::parse_value, "2s margin-left, 4s background").unwrap(); + assert_eq!(result.transition_property, + parse_longhand!(transition_property, "margin-left, background")); + + let result = parse(transition::parse_value, "2s margin, 4s all").unwrap(); + assert_eq!(result.transition_property, + parse_longhand!(transition_property, "margin, all")); + + let result = parse(transition::parse_value, "none").unwrap(); + assert_eq!(result.transition_property, + parse_longhand!(transition_property, "none")); +} From 43f9f8ca957a41f0e4dada0c33bf15d870be50a9 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 19 Apr 2017 10:44:30 +0800 Subject: [PATCH 3/4] Bug 1353628 - Part 3: Create PropertyAnimation for shorthands. Therefore, we can start transitions on shorthand properties properly. MozReview-Commit-ID: Ev2u2tEalnK --- components/style/animation.rs | 11 +++++++++++ .../properties/helpers/animated_properties.mako.rs | 10 ++++++++++ 2 files changed, 21 insertions(+) diff --git a/components/style/animation.rs b/components/style/animation.rs index aeff683f407..63998083d6d 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -272,6 +272,15 @@ impl PropertyAnimation { let timing_function = box_style.transition_timing_function_mod(transition_index); let duration = box_style.transition_duration_mod(transition_index); + if transition_property.is_shorthand() { + return transition_property.longhands().iter().filter_map(|transition_property| { + PropertyAnimation::from_transition_property(*transition_property, + timing_function, + duration, + old_style, + new_style) + }).collect(); + } if transition_property != TransitionProperty::All { if let Some(property_animation) = @@ -305,6 +314,8 @@ impl PropertyAnimation { old_style: &ComputedValues, new_style: &ComputedValues) -> Option { + debug_assert!(!transition_property.is_shorthand() && + transition_property != TransitionProperty::All); let animated_property = AnimatedProperty::from_transition_property(&transition_property, old_style, new_style); diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 721b858e59b..5e49b263605 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -158,6 +158,16 @@ impl TransitionProperty { _ => 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(): + TransitionProperty::${prop.camel_case} => true, + % endfor + _ => false + } + } } /// Returns true if this nsCSSPropertyID is one of the animatable properties. From 8e90a2a287844b510a39f3d5c745c79cab795ed6 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 19 Apr 2017 20:32:16 +0800 Subject: [PATCH 4/4] Bug 1353628 - Part 4: Update metadata for css-transition-1_dev. 1. All tests in events-001.htm are passed. 2. Parsing 'inherit' in transition-property-002.htm is passed. --- .../css-transitions-1_dev/html/events-001.htm.ini | 11 ----------- .../html/transition-property-002.htm.ini | 3 --- 2 files changed, 14 deletions(-) delete mode 100644 tests/wpt/metadata-css/css-transitions-1_dev/html/events-001.htm.ini diff --git a/tests/wpt/metadata-css/css-transitions-1_dev/html/events-001.htm.ini b/tests/wpt/metadata-css/css-transitions-1_dev/html/events-001.htm.ini deleted file mode 100644 index 69679779896..00000000000 --- a/tests/wpt/metadata-css/css-transitions-1_dev/html/events-001.htm.ini +++ /dev/null @@ -1,11 +0,0 @@ -[events-001.htm] - type: testharness - [transition:padding, changing padding-left / events] - expected: FAIL - - [transition:padding, changing padding / events] - expected: FAIL - - [transition:padding, changing padding but not padding-bottom / events] - expected: FAIL - diff --git a/tests/wpt/metadata-css/css-transitions-1_dev/html/transition-property-002.htm.ini b/tests/wpt/metadata-css/css-transitions-1_dev/html/transition-property-002.htm.ini index 95904351714..d32a55f8927 100644 --- a/tests/wpt/metadata-css/css-transitions-1_dev/html/transition-property-002.htm.ini +++ b/tests/wpt/metadata-css/css-transitions-1_dev/html/transition-property-002.htm.ini @@ -15,9 +15,6 @@ [parse 'foobar, all'] expected: FAIL - [parse 'inherit'] - expected: FAIL - [parse 'initial'] expected: FAIL