From f4e38939cdc1e70927bb0fa8f483487a3e55d8d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Aug 2024 23:26:14 +0000 Subject: [PATCH] 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 --- .../en-US/chrome/layout/css.properties | 2 +- .../test/test_css_parse_error_smoketest.html | 2 +- servo/components/style/error_reporting.rs | 6 -- servo/components/style/parser.rs | 6 ++ .../style/properties/declaration_block.rs | 18 ++-- .../stylesheets/font_feature_values_rule.rs | 6 +- .../style/stylesheets/keyframes_rule.rs | 97 ++----------------- servo/components/style/stylesheets/mod.rs | 3 + servo/components/style_traits/lib.rs | 11 +-- servo/ports/geckolib/error_reporter.rs | 8 +- 10 files changed, 31 insertions(+), 128 deletions(-) diff --git a/dom/locales/en-US/chrome/layout/css.properties b/dom/locales/en-US/chrome/layout/css.properties index 0ecfc4f5edab..4f59368ed1c5 100644 --- a/dom/locales/en-US/chrome/layout/css.properties +++ b/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. diff --git a/layout/style/test/test_css_parse_error_smoketest.html b/layout/style/test/test_css_parse_error_smoketest.html index d6c790d1812a..33fef41d18ad 100644 --- a/layout/style/test/test_css_parse_error_smoketest.html +++ b/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." }, ]; diff --git a/servo/components/style/error_reporting.rs b/servo/components/style/error_reporting.rs index 6db4c18e2780..74a742741ec0 100644 --- a/servo/components/style/error_reporting.rs +++ b/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) diff --git a/servo/components/style/parser.rs b/servo/components/style/parser.rs index f639f5d49f9f..ad7a573cc23f 100644 --- a/servo/components/style/parser.rs +++ b/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 diff --git a/servo/components/style/properties/declaration_block.rs b/servo/components/style/properties/declaration_block.rs index 2d237530f573..49b1d020f74f 100644 --- a/servo/components/style/properties/declaration_block.rs +++ b/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) => { diff --git a/servo/components/style/stylesheets/font_feature_values_rule.rs b/servo/components/style/stylesheets/font_feature_values_rule.rs index 3f82f7f4fd46..c84e38a50ec8 100644 --- a/servo/components/style/stylesheets/font_feature_values_rule.rs +++ b/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); } } diff --git a/servo/components/style/stylesheets/keyframes_rule.rs b/servo/components/style/stylesheets/keyframes_rule.rs index 96e916b553b2..b058e0043a64 100644 --- a/servo/components/style/stylesheets/keyframes_rule.rs +++ b/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>> { - 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,33 +582,9 @@ impl<'a, 'b, 'i> QualifiedRuleParser<'i> for KeyframeListParser<'a, 'b> { start: &ParserState, input: &mut Parser<'i, 't>, ) -> Result> { - 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, block: Arc::new(self.shared_lock.wrap(block)), @@ -632,59 +603,3 @@ impl<'a, 'b, 'i> RuleBodyItemParser<'i, Arc>, 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 - } -} diff --git a/servo/components/style/stylesheets/mod.rs b/servo/components/style/stylesheets/mod.rs index 0a012e5d5ae5..806869fb2bcd 100644 --- a/servo/components/style/stylesheets/mod.rs +++ b/servo/components/style/stylesheets/mod.rs @@ -424,6 +424,9 @@ impl From 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 { diff --git a/servo/components/style_traits/lib.rs b/servo/components/style_traits/lib.rs index c2f5314ce3be..18ca4832c843 100644 --- a/servo/components/style_traits/lib.rs +++ b/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> for StyleParseErrorKind<'i> { diff --git a/servo/ports/geckolib/error_reporter.rs b/servo/ports/geckolib/error_reporter.rs index 1170f5875b7e..8ea2dbc7f309 100644 --- a/servo/ports/geckolib/error_reporter.rs +++ b/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 {