Make parsing of keyframe declaration blocks spec-compliant.

Exclude animation properties.

components/style/keyframes.rs

@@ -2,10 +2,12 @@
  * 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 cssparser::{AtRuleParser, Delimiter, Parser, QualifiedRuleParser, RuleListParser};
+use cssparser::{AtRuleParser, Parser, QualifiedRuleParser, RuleListParser};
+use cssparser::{DeclarationListParser, DeclarationParser};
 use parser::{ParserContext, log_css_error};
+use properties::PropertyDeclaration;
+use properties::PropertyDeclarationParseResult;
 use properties::animated_properties::TransitionProperty;
-use properties::{PropertyDeclaration, parse_property_declaration_list};
 use std::sync::Arc;
 
 /// A number from 1 to 100, indicating the percentage of the animation where
@@ -238,12 +240,49 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
 
     fn parse_block(&self, prelude: Self::Prelude, input: &mut Parser)
                    -> Result<Self::QualifiedRule, ()> {
+        let mut declarations = Vec::new();
+        let parser = KeyframeDeclarationParser {
+            context: self.context,
+        };
+        let mut iter = DeclarationListParser::new(input, parser);
+        while let Some(declaration) = iter.next() {
+            match declaration {
+                Ok(d) => declarations.extend(d),
+                Err(range) => {
+                    let pos = range.start;
+                    let message = format!("Unsupported keyframe property declaration: '{}'",
+                                          iter.input.slice(range));
+                    log_css_error(iter.input, pos, &*message, self.context);
+                }
+            }
+            // `parse_important` is not called here, `!important` is not allowed in keyframe blocks.
+        }
         Ok(Keyframe {
             selector: prelude,
-            // FIXME: needs parsing different from parse_property_declaration_list:
-            // https://drafts.csswg.org/css-animations/#keyframes
-            // Paragraph "The <declaration-list> inside of <keyframe-block> ..."
-            declarations: parse_property_declaration_list(self.context, input).normal,
+            declarations: Arc::new(declarations),
         })
     }
 }
+
+struct KeyframeDeclarationParser<'a, 'b: 'a> {
+    context: &'a ParserContext<'b>,
+}
+
+/// Default methods reject all at rules.
+impl<'a, 'b> AtRuleParser for KeyframeDeclarationParser<'a, 'b> {
+    type Prelude = ();
+    type AtRule = Vec<PropertyDeclaration>;
+}
+
+impl<'a, 'b> DeclarationParser for KeyframeDeclarationParser<'a, 'b> {
+    type Declaration = Vec<PropertyDeclaration>;
+
+    fn parse_value(&self, name: &str, input: &mut Parser) -> Result<Vec<PropertyDeclaration>, ()> {
+        let mut results = Vec::new();
+        match PropertyDeclaration::parse(name, self.context, input, &mut results, true) {
+            PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => {}
+            _ => return Err(())
+        }
+        Ok(results)
+    }
+}

components/style/properties/data.py

@@ -65,7 +65,8 @@ def maybe_cast(self, type_str):
 class Longhand(object):
     def __init__(self, style_struct, name, animatable=None, derived_from=None, keyword=None,
                  predefined_type=None, custom_cascade=False, experimental=False, internal=False,
-                 need_clone=False, need_index=False, gecko_ffi_name=None, depend_on_viewport_size=False):
+                 need_clone=False, need_index=False, gecko_ffi_name=None, depend_on_viewport_size=False,
+                 allowed_in_keyframe_block=True):
         self.name = name
         self.keyword = keyword
         self.predefined_type = predefined_type
@@ -80,6 +81,13 @@ def __init__(self, style_struct, name, animatable=None, derived_from=None, keywo
         self.depend_on_viewport_size = depend_on_viewport_size
         self.derived_from = (derived_from or "").split()
 
+        # https://drafts.csswg.org/css-animations/#keyframes
+        # > The <declaration-list> inside of <keyframe-block> accepts any CSS property
+        # > except those defined in this specification,
+        # > but does accept the `animation-play-state` property and interprets it specially.
+        self.allowed_in_keyframe_block = allowed_in_keyframe_block \
+            and allowed_in_keyframe_block != "False"
+
         # This is done like this since just a plain bool argument seemed like
         # really random.
         if animatable is None:
@@ -98,7 +106,8 @@ def __init__(self, style_struct, name, animatable=None, derived_from=None, keywo
 
 
 class Shorthand(object):
-    def __init__(self, name, sub_properties, experimental=False, internal=False):
+    def __init__(self, name, sub_properties, experimental=False, internal=False,
+                 allowed_in_keyframe_block=True):
         self.name = name
         self.ident = to_rust_ident(name)
         self.camel_case = to_camel_case(self.ident)
@@ -107,6 +116,13 @@ def __init__(self, name, sub_properties, experimental=False, internal=False):
         self.sub_properties = sub_properties
         self.internal = internal
 
+        # https://drafts.csswg.org/css-animations/#keyframes
+        # > The <declaration-list> inside of <keyframe-block> accepts any CSS property
+        # > except those defined in this specification,
+        # > but does accept the `animation-play-state` property and interprets it specially.
+        self.allowed_in_keyframe_block = allowed_in_keyframe_block \
+            and allowed_in_keyframe_block != "False"
+
 
 class Method(object):
     def __init__(self, name, return_type=None, arg_types=None, is_mut=False):

components/style/properties/longhand/box.mako.rs

@@ -636,7 +636,8 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto",
 
 <%helpers:longhand name="animation-name"
                    need_index="True"
-                   animatable="False">
+                   animatable="False",
+                   allowed_in_keyframe_block="False">
     use values::computed::ComputedValueAsSpecified;
     use values::NoViewportPercentage;
 
@@ -699,7 +700,8 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto",
 
 <%helpers:longhand name="animation-duration"
                    need_index="True"
-                   animatable="False">
+                   animatable="False",
+                   allowed_in_keyframe_block="False">
     pub use super::transition_duration::computed_value;
     pub use super::transition_duration::{get_initial_value, get_initial_single_value};
     pub use super::transition_duration::{parse, parse_one};
@@ -709,7 +711,8 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto",
 
 <%helpers:longhand name="animation-timing-function"
                    need_index="True"
-                   animatable="False">
+                   animatable="False",
+                   allowed_in_keyframe_block="False">
     pub use super::transition_timing_function::computed_value;
     pub use super::transition_timing_function::{get_initial_value, get_initial_single_value};
     pub use super::transition_timing_function::{parse, parse_one};
@@ -719,7 +722,8 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto",
 
 <%helpers:longhand name="animation-iteration-count"
                    need_index="True"
-                   animatable="False">
+                   animatable="False",
+                   allowed_in_keyframe_block="False">
     use values::computed::ComputedValueAsSpecified;
     use values::NoViewportPercentage;
 
@@ -804,22 +808,28 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto",
 ${helpers.keyword_list("animation-direction",
                        "normal reverse alternate alternate-reverse",
                        need_index=True,
-                       animatable=False)}
+                       animatable=False,
+                       allowed_in_keyframe_block=False)}
 
+// animation-play-state is the exception to the rule for allowed_in_keyframe_block:
+// https://drafts.csswg.org/css-animations/#keyframes
 ${helpers.keyword_list("animation-play-state",
                        "running paused",
                        need_clone=True,
                        need_index=True,
-                       animatable=False)}
+                       animatable=False,
+                       allowed_in_keyframe_block=True)}
 
 ${helpers.keyword_list("animation-fill-mode",
                        "none forwards backwards both",
                        need_index=True,
-                       animatable=False)}
+                       animatable=False,
+                       allowed_in_keyframe_block=False)}
 
 <%helpers:longhand name="animation-delay"
                    need_index="True"
-                   animatable="False">
+                   animatable="False",
+                   allowed_in_keyframe_block="False">
     pub use super::transition_duration::computed_value;
     pub use super::transition_duration::{get_initial_value, get_initial_single_value};
     pub use super::transition_duration::{parse, parse_one};

components/style/properties/properties.mako.rs

@@ -517,7 +517,7 @@ pub fn parse_one_declaration(name: &str, input: &str, base_url: &Url, error_repo
                              -> Result<Vec<PropertyDeclaration>, ()> {
     let context = ParserContext::new_with_extra_data(Origin::Author, base_url, error_reporter, extra_data);
     let mut results = vec![];
-    match PropertyDeclaration::parse(name, &context, &mut Parser::new(input), &mut results) {
+    match PropertyDeclaration::parse(name, &context, &mut Parser::new(input), &mut results, false) {
         PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => Ok(results),
         _ => Err(())
     }
@@ -542,7 +542,7 @@ impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> {
                    -> Result<(Vec<PropertyDeclaration>, Importance), ()> {
         let mut results = vec![];
         try!(input.parse_until_before(Delimiter::Bang, |input| {
-            match PropertyDeclaration::parse(name, self.context, input, &mut results) {
+            match PropertyDeclaration::parse(name, self.context, input, &mut results, false) {
                 PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => Ok(()),
                 _ => Err(())
             }
@@ -832,6 +832,7 @@ pub enum PropertyDeclarationParseResult {
     UnknownProperty,
     ExperimentalProperty,
     InvalidValue,
+    AnimationPropertyInKeyframeBlock,
     ValidOrIgnoredDeclaration,
 }
 
@@ -984,8 +985,16 @@ impl PropertyDeclaration {
         }
     }
 
+    /// The `in_keyframe_block` parameter controls this:
+    ///
+    /// https://drafts.csswg.org/css-animations/#keyframes
+    /// > The <declaration-list> inside of <keyframe-block> accepts any CSS property
+    /// > except those defined in this specification,
+    /// > but does accept the `animation-play-state` property and interprets it specially.
     pub fn parse(name: &str, context: &ParserContext, input: &mut Parser,
-                 result_list: &mut Vec<PropertyDeclaration>) -> PropertyDeclarationParseResult {
+                 result_list: &mut Vec<PropertyDeclaration>,
+                 in_keyframe_block: bool)
+                 -> PropertyDeclarationParseResult {
         if let Ok(name) = ::custom_properties::parse_name(name) {
             let value = match input.try(CSSWideKeyword::parse) {
                 Ok(CSSWideKeyword::UnsetKeyword) |  // Custom properties are alawys inherited
@@ -1003,6 +1012,11 @@ impl PropertyDeclaration {
             % for property in data.longhands:
                 % if not property.derived_from:
                     "${property.name}" => {
+                        % if not property.allowed_in_keyframe_block:
+                            if in_keyframe_block {
+                                return PropertyDeclarationParseResult::AnimationPropertyInKeyframeBlock
+                            }
+                        % endif
                         % if property.internal:
                             if context.stylesheet_origin != Origin::UserAgent {
                                 return PropertyDeclarationParseResult::UnknownProperty
@@ -1028,6 +1042,11 @@ impl PropertyDeclaration {
             % endfor
             % for shorthand in data.shorthands:
                 "${shorthand.name}" => {
+                    % if not shorthand.allowed_in_keyframe_block:
+                        if in_keyframe_block {
+                            return PropertyDeclarationParseResult::AnimationPropertyInKeyframeBlock
+                        }
+                    % endif
                     % if shorthand.internal:
                         if context.stylesheet_origin != Origin::UserAgent {
                             return PropertyDeclarationParseResult::UnknownProperty

components/style/properties/shorthand/box.mako.rs

@@ -152,7 +152,8 @@ macro_rules! try_parse_one {
                     sub_properties="animation-name animation-duration
                                     animation-timing-function animation-delay
                                     animation-iteration-count animation-direction
-                                    animation-fill-mode animation-play-state">
+                                    animation-fill-mode animation-play-state"
+                    allowed_in_keyframe_block="False">
     use properties::longhands::{animation_name, animation_duration, animation_timing_function};
     use properties::longhands::{animation_delay, animation_iteration_count, animation_direction};
     use properties::longhands::{animation_fill_mode, animation_play_state};

tests/unit/style/stylesheets.rs

@@ -13,6 +13,7 @@ use style::error_reporting::ParseErrorReporter;
 use style::keyframes::{Keyframe, KeyframeSelector, KeyframePercentage};
 use style::parser::ParserContextExtraData;
 use style::properties::{PropertyDeclaration, PropertyDeclarationBlock, DeclaredValue, longhands};
+use style::properties::longhands::animation_play_state;
 use style::stylesheets::{Stylesheet, CSSRule, StyleRule, KeyframesRule, Origin};
 use style::values::specified::{LengthOrPercentageOrAuto, Percentage};
 use url::Url;
@@ -27,7 +28,12 @@ fn test_parse_stylesheet() {
         #d1 > .ok { background: blue; }
         @keyframes foo {
             from { width: 0% }
-            to { width: 100%}
+            to {
+                width: 100%;
+                width: 50% !important; /* !important not allowed here */
+                animation-name: 'foo'; /* animation properties not allowed here */
+                animation-play-state: running; /* … except animation-play-state */
+            }
         }";
     let url = Url::parse("about::test").unwrap();
     let stylesheet = Stylesheet::from_str(css, url, Origin::UserAgent,
@@ -187,6 +193,9 @@ fn test_parse_stylesheet() {
                         declarations: Arc::new(vec![
                             PropertyDeclaration::Width(DeclaredValue::Value(
                                 LengthOrPercentageOrAuto::Percentage(Percentage(1.)))),
+                            PropertyDeclaration::AnimationPlayState(DeclaredValue::Value(
+                                animation_play_state::SpecifiedValue(
+                                    vec![animation_play_state::SingleSpecifiedValue::running]))),
                         ]),
                     },
                 ]