Bug 1911353 - Unify how we reject !important in keyframe and @position-try. r=dshin

This was more the kind of thing I meant, and allows us to get rid of the
keyframe-rule-specific parser.

Differential Revision: https://phabricator.services.mozilla.com/D218488

dom/locales/en-US/chrome/layout/css.properties

@@ -47,7 +47,7 @@ PEExpectedNoneOrURL=Expected ‘none’ or URL but found ‘%1$S’.
 PEExpectedNoneOrURLOrFilterFunction=Expected ‘none’, URL, or filter function but found ‘%1$S’.
 PEDisallowedImportRule=@import rules are not yet valid in constructed stylesheets.
 PENeverMatchingHostSelector=:host selector in ‘%S’ is not featureless and will never match. Maybe you intended to use :host()?
-PEPositionTryImportantDeclError=Property cannot be declared as !important in a @position-try rule.
+PEImportantDeclError=Property cannot be declared as !important in this context.
 
 TooLargeDashedRadius=Border radius is too large for ‘dashed’ style (the limit is 100000px). Rendering as solid.
 TooLargeDottedRadius=Border radius is too large for ‘dotted’ style (the limit is 100000px). Rendering as solid.

layout/style/test/test_css_parse_error_smoketest.html

@@ -127,7 +127,7 @@
     },
     {
       css: "@position-try --foo { width: 10px !important; }",
-      error: "Property cannot be declared as !important in a @position-try rule.  Declaration dropped."
+      error: "Property cannot be declared as !important in this context.  Declaration dropped."
     },
   ];
 

servo/components/style/error_reporting.rs

@@ -32,8 +32,6 @@ pub enum ContextualParseError<'a> {
     InvalidKeyframeRule(&'a str, ParseError<'a>),
     /// A font feature values rule was not valid.
     InvalidFontFeatureValuesRule(&'a str, ParseError<'a>),
-    /// A keyframe property declaration was not recognized.
-    UnsupportedKeyframePropertyDeclaration(&'a str, ParseError<'a>),
     /// A rule was invalid for some reason.
     InvalidRule(&'a str, ParseError<'a>),
     /// A rule was not recognized.
@@ -175,10 +173,6 @@ impl<'a> fmt::Display for ContextualParseError<'a> {
                 write!(f, "Invalid font feature value rule: '{}', ", rule)?;
                 parse_error_to_str(err, f)
             },
-            ContextualParseError::UnsupportedKeyframePropertyDeclaration(decl, ref err) => {
-                write!(f, "Unsupported keyframe property declaration: '{}', ", decl)?;
-                parse_error_to_str(err, f)
-            },
             ContextualParseError::InvalidRule(rule, ref err) => {
                 write!(f, "Invalid rule: '{}', ", rule)?;
                 parse_error_to_str(err, f)

servo/components/style/parser.rs

@@ -131,6 +131,12 @@ impl<'a> ParserContext<'a> {
         self.nesting_context.rule_types.contains(CssRuleType::Page)
     }
 
+    /// Returns whether !important declarations are forbidden.
+    #[inline]
+    pub fn allows_important_declarations(&self) -> bool {
+        !self.nesting_context.rule_types.intersects(CssRuleTypes::IMPORTANT_FORBIDDEN)
+    }
+
     /// Get the rule type, which assumes that one is available.
     pub fn rule_types(&self) -> CssRuleTypes {
         self.nesting_context.rule_types

servo/components/style/properties/declaration_block.rs

@@ -1455,18 +1455,14 @@ impl<'i> DeclarationParserState<'i> {
             PropertyDeclaration::parse_into(&mut self.declarations, id, context, input)
         })?;
         self.importance = match input.try_parse(parse_important) {
-            Ok(()) => Importance::Important,
+            Ok(()) => {
+                if !context.allows_important_declarations() {
+                    return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedImportantDeclaration));
+                }
+                Importance::Important
+            },
             Err(_) => Importance::Normal,
         };
-        if context
-            .nesting_context
-            .rule_types
-            .contains(CssRuleType::PositionTry) &&
-            matches!(self.importance, Importance::Important)
-        {
-            return Err(input
-                .new_custom_error(StyleParseErrorKind::PositionTryUnexpectedImportantDeclaration));
-        }
         // In case there is still unparsed text in the declaration, we should roll back.
         input.expect_exhausted()?;
         self.output_block
@@ -1623,7 +1619,7 @@ fn report_one_css_error<'i>(
         // the error to be more specific.
         if !matches!(
             error.kind,
-            ParseErrorKind::Custom(StyleParseErrorKind::PositionTryUnexpectedImportantDeclaration)
+            ParseErrorKind::Custom(StyleParseErrorKind::UnexpectedImportantDeclaration)
         ) {
             error = match *property {
                 PropertyId::Custom(ref c) => {

servo/components/style/stylesheets/font_feature_values_rule.rs

@@ -422,9 +422,9 @@ macro_rules! font_feature_values_blocks {
                             while let Some(declaration) = iter.next() {
                                 if let Err((error, slice)) = declaration {
                                     let location = error.location;
-                                    let error = ContextualParseError::UnsupportedKeyframePropertyDeclaration(
-                                        slice, error
-                                    );
+                                    // TODO(emilio): Maybe add a more specific error kind for
+                                    // font-feature-values descriptors.
+                                    let error = ContextualParseError::UnsupportedPropertyDeclaration(slice, error, &[]);
                                     self.context.log_css_error(location, error);
                                 }
                             }

servo/components/style/stylesheets/keyframes_rule.rs

@@ -11,8 +11,8 @@ use crate::properties::{
         animation_composition::single_value::SpecifiedValue as SpecifiedComposition,
         transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction,
     },
-    Importance, LonghandId, PropertyDeclaration, PropertyDeclarationBlock, PropertyDeclarationId,
-    PropertyDeclarationIdSet, PropertyId, SourcePropertyDeclaration,
+    parse_property_declaration_list, LonghandId, PropertyDeclaration, PropertyDeclarationBlock,
+    PropertyDeclarationId, PropertyDeclarationIdSet,
 };
 use crate::shared_lock::{DeepCloneParams, DeepCloneWithLock, SharedRwLock, SharedRwLockReadGuard};
 use crate::shared_lock::{Locked, ToCssWithGuard};
@@ -21,7 +21,7 @@ use crate::stylesheets::rule_parser::VendorPrefix;
 use crate::stylesheets::{CssRuleType, StylesheetContents};
 use crate::values::{serialize_percentage, KeyframesName};
 use cssparser::{
-    parse_one_rule, AtRuleParser, CowRcStr, DeclarationParser, Parser, ParserInput, ParserState,
+    parse_one_rule, AtRuleParser, DeclarationParser, Parser, ParserInput, ParserState,
     QualifiedRuleParser, RuleBodyItemParser, RuleBodyParser, SourceLocation, Token,
 };
 use servo_arc::Arc;
@@ -227,11 +227,9 @@ impl Keyframe {
         let mut input = ParserInput::new(css);
         let mut input = Parser::new(&mut input);
 
-        let mut declarations = SourcePropertyDeclaration::default();
         let mut rule_parser = KeyframeListParser {
             context: &mut context,
             shared_lock: &lock,
-            declarations: &mut declarations,
         };
         parse_one_rule(&mut input, &mut rule_parser)
     }
@@ -529,7 +527,6 @@ impl KeyframesAnimation {
 struct KeyframeListParser<'a, 'b> {
     context: &'a mut ParserContext<'b>,
     shared_lock: &'a SharedRwLock,
-    declarations: &'a mut SourcePropertyDeclaration,
 }
 
 /// Parses a keyframe list from CSS input.
@@ -538,11 +535,9 @@ pub fn parse_keyframe_list<'a>(
     input: &mut Parser,
     shared_lock: &SharedRwLock,
 ) -> Vec<Arc<Locked<Keyframe>>> {
-    let mut declarations = SourcePropertyDeclaration::default();
     let mut parser = KeyframeListParser {
         context,
         shared_lock,
-        declarations: &mut declarations,
     };
     RuleBodyParser::new(input, &mut parser)
         .filter_map(Result::ok)
@@ -587,32 +582,8 @@ impl<'a, 'b, 'i> QualifiedRuleParser<'i> for KeyframeListParser<'a, 'b> {
         start: &ParserState,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self::QualifiedRule, ParseError<'i>> {
-        let mut block = PropertyDeclarationBlock::new();
-        let declarations = &mut self.declarations;
-        self.context
-            .nest_for_rule(CssRuleType::Keyframe, |context| {
-                let mut parser = KeyframeDeclarationParser {
-                    context: &context,
-                    declarations,
-                };
-                let mut iter = RuleBodyParser::new(input, &mut parser);
-                while let Some(declaration) = iter.next() {
-                    match declaration {
-                        Ok(()) => {
-                            block.extend(iter.parser.declarations.drain(), Importance::Normal);
-                        },
-                        Err((error, slice)) => {
-                            iter.parser.declarations.clear();
-                            let location = error.location;
-                            let error =
-                                ContextualParseError::UnsupportedKeyframePropertyDeclaration(
-                                    slice, error,
-                                );
-                            context.log_css_error(location, error);
-                        },
-                    }
-                    // `parse_important` is not called here, `!important` is not allowed in keyframe blocks.
-                }
+        let block = self.context.nest_for_rule(CssRuleType::Keyframe, |p| {
+            parse_property_declaration_list(&p, input, &[])
         });
         Ok(Arc::new(self.shared_lock.wrap(Keyframe {
             selector,
@@ -632,59 +603,3 @@ impl<'a, 'b, 'i> RuleBodyItemParser<'i, Arc<Locked<Keyframe>>, StyleParseErrorKi
         false
     }
 }
-
-struct KeyframeDeclarationParser<'a, 'b: 'a> {
-    context: &'a ParserContext<'b>,
-    declarations: &'a mut SourcePropertyDeclaration,
-}
-
-/// Default methods reject all at rules.
-impl<'a, 'b, 'i> AtRuleParser<'i> for KeyframeDeclarationParser<'a, 'b> {
-    type Prelude = ();
-    type AtRule = ();
-    type Error = StyleParseErrorKind<'i>;
-}
-
-impl<'a, 'b, 'i> QualifiedRuleParser<'i> for KeyframeDeclarationParser<'a, 'b> {
-    type Prelude = ();
-    type QualifiedRule = ();
-    type Error = StyleParseErrorKind<'i>;
-}
-
-impl<'a, 'b, 'i> DeclarationParser<'i> for KeyframeDeclarationParser<'a, 'b> {
-    type Declaration = ();
-    type Error = StyleParseErrorKind<'i>;
-
-    fn parse_value<'t>(
-        &mut self,
-        name: CowRcStr<'i>,
-        input: &mut Parser<'i, 't>,
-    ) -> Result<(), ParseError<'i>> {
-        let id = match PropertyId::parse(&name, self.context) {
-            Ok(id) => id,
-            Err(()) => {
-                return Err(input.new_custom_error(StyleParseErrorKind::UnknownProperty(name)));
-            },
-        };
-
-        // TODO(emilio): Shouldn't this use parse_entirely?
-        PropertyDeclaration::parse_into(self.declarations, id, self.context, input)?;
-
-        // In case there is still unparsed text in the declaration, we should
-        // roll back.
-        input.expect_exhausted()?;
-
-        Ok(())
-    }
-}
-
-impl<'a, 'b, 'i> RuleBodyItemParser<'i, (), StyleParseErrorKind<'i>>
-    for KeyframeDeclarationParser<'a, 'b>
-{
-    fn parse_qualified(&self) -> bool {
-        false
-    }
-    fn parse_declarations(&self) -> bool {
-        true
-    }
-}

servo/components/style/stylesheets/mod.rs

@@ -424,6 +424,9 @@ impl From<CssRuleType> for CssRuleTypes {
 }
 
 impl CssRuleTypes {
+    /// Rules where !important declarations are forbidden.
+    pub const IMPORTANT_FORBIDDEN: Self = Self(CssRuleType::PositionTry.bit() | CssRuleType::Keyframe.bit());
+
     /// Returns whether the rule is in the current set.
     #[inline]
     pub fn contains(self, ty: CssRuleType) -> bool {

servo/components/style_traits/lib.rs

@@ -163,15 +163,8 @@ pub enum StyleParseErrorKind<'i> {
     InvalidFilter(CowRcStr<'i>, Token<'i>),
     /// The property declaration contained an invalid value.
     OtherInvalidValue(CowRcStr<'i>),
-    /// The declaration contained an animation property, and we were parsing
-    /// this as a keyframe block (so that property should be ignored).
-    ///
-    /// See: https://drafts.csswg.org/css-animations/#keyframes
-    AnimationPropertyInKeyframeBlock,
-    /// The property is not allowed within a page rule.
-    NotAllowedInPageRule,
-    /// `!important` declarations are disallowed in `@position-try`.
-    PositionTryUnexpectedImportantDeclaration,
+    /// `!important` declarations are disallowed in `@position-try` or keyframes.
+    UnexpectedImportantDeclaration,
 }
 
 impl<'i> From<ValueParseErrorKind<'i>> for StyleParseErrorKind<'i> {

servo/ports/geckolib/error_reporter.rs

@@ -196,7 +196,6 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
             ContextualParseError::UnsupportedFontPaletteValuesDescriptor(s, err) |
             ContextualParseError::InvalidKeyframeRule(s, err) |
             ContextualParseError::InvalidFontFeatureValuesRule(s, err) |
-            ContextualParseError::UnsupportedKeyframePropertyDeclaration(s, err) |
             ContextualParseError::InvalidRule(s, err) |
             ContextualParseError::UnsupportedRule(s, err) |
             ContextualParseError::UnsupportedViewportDescriptorDeclaration(s, err) |
@@ -276,8 +275,8 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
                 StyleParseErrorKind::OtherInvalidValue(_) => {
                     (cstr!("PEValueParsingError"), Action::Drop)
                 },
-                StyleParseErrorKind::PositionTryUnexpectedImportantDeclaration => {
-                    (cstr!("PEPositionTryImportantDeclError"), Action::Drop)
+                StyleParseErrorKind::UnexpectedImportantDeclaration => {
+                    (cstr!("PEImportantDeclError"), Action::Drop)
                 },
                 _ => (cstr!("PEUnknownProperty"), Action::Drop),
             },
@@ -290,9 +289,6 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
             ContextualParseError::InvalidKeyframeRule(..) => {
                 (cstr!("PEKeyframeBadName"), Action::Nothing)
             },
-            ContextualParseError::UnsupportedKeyframePropertyDeclaration(..) => {
-                (cstr!("PEBadSelectorKeyframeRuleIgnored"), Action::Nothing)
-            },
             ContextualParseError::InvalidRule(
                 _,
                 ParseError {