From 543946adb55bad923f5f074d6c3f52193206faa5 Mon Sep 17 00:00:00 2001 From: David Shin Date: Fri, 16 Aug 2024 13:14:05 +0000 Subject: [PATCH] Bug 1900233: Disallow custom property and important declarations in @position-try. r=firefox-style-system-reviewers,emilio Differential Revision: https://phabricator.services.mozilla.com/D217629 --- .../en-US/chrome/layout/css.properties | 1 + layout/style/test/mochitest.toml | 1 + .../test/test_css_parse_error_smoketest.html | 4 +++ .../style/properties/declaration_block.rs | 29 +++++++++++++++---- servo/components/style/properties/mod.rs | 5 ++-- servo/components/style_traits/lib.rs | 2 ++ servo/ports/geckolib/error_reporter.rs | 3 ++ ...position-try-allowed-declarations.html.ini | 9 ------ .../at-position-try-cssom.html.ini | 3 -- 9 files changed, 37 insertions(+), 20 deletions(-) delete mode 100644 testing/web-platform/meta/css/css-anchor-position/at-position-try-allowed-declarations.html.ini diff --git a/dom/locales/en-US/chrome/layout/css.properties b/dom/locales/en-US/chrome/layout/css.properties index e9ec13286821..0ecfc4f5edab 100644 --- a/dom/locales/en-US/chrome/layout/css.properties +++ b/dom/locales/en-US/chrome/layout/css.properties @@ -47,6 +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. 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/mochitest.toml b/layout/style/test/mochitest.toml index ffd05af150cd..2f44fc73f39d 100644 --- a/layout/style/test/mochitest.toml +++ b/layout/style/test/mochitest.toml @@ -15,6 +15,7 @@ prefs = [ "layout.css.transform-box-content-stroke.enabled=true", "layout.css.transition-behavior.enabled=true", "layout.css.field-sizing.enabled=true", + "layout.css.anchor-positioning.enabled=true", ] support-files = [ "animation_utils.js", diff --git a/layout/style/test/test_css_parse_error_smoketest.html b/layout/style/test/test_css_parse_error_smoketest.html index 96d8edce3a29..d6c790d1812a 100644 --- a/layout/style/test/test_css_parse_error_smoketest.html +++ b/layout/style/test/test_css_parse_error_smoketest.html @@ -125,6 +125,10 @@ css: ":host:hover { color: red; }", error: ":host selector in ‘:host:hover’ is not featureless and will never match. Maybe you intended to use :host()?" }, + { + css: "@position-try --foo { width: 10px !important; }", + error: "Property cannot be declared as !important in a @position-try rule. Declaration dropped." + }, ]; // Tests that apply only to constructed style sheets diff --git a/servo/components/style/properties/declaration_block.rs b/servo/components/style/properties/declaration_block.rs index f3c381e70458..2d237530f573 100644 --- a/servo/components/style/properties/declaration_block.rs +++ b/servo/components/style/properties/declaration_block.rs @@ -1458,6 +1458,15 @@ impl<'i> DeclarationParserState<'i> { Ok(()) => 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 @@ -1609,12 +1618,20 @@ fn report_one_css_error<'i>( return; } } - error = match *property { - PropertyId::Custom(ref c) => { - StyleParseErrorKind::new_invalid(format!("--{}", c), error) - }, - _ => StyleParseErrorKind::new_invalid(property.non_custom_id().unwrap().name(), error), - }; + // Was able to parse property ID - Either an invalid value, or is constrained + // by the rule block it's in to be invalid. In the former case, we need to unwrap + // the error to be more specific. + if !matches!( + error.kind, + ParseErrorKind::Custom(StyleParseErrorKind::PositionTryUnexpectedImportantDeclaration) + ) { + error = match *property { + PropertyId::Custom(ref c) => { + StyleParseErrorKind::new_invalid(format!("--{}", c), error) + }, + _ => StyleParseErrorKind::new_invalid(property.non_custom_id().unwrap().name(), error), + }; + } } let location = error.location; diff --git a/servo/components/style/properties/mod.rs b/servo/components/style/properties/mod.rs index 93aa0e80fcb9..e36ea36f856f 100644 --- a/servo/components/style/properties/mod.rs +++ b/servo/components/style/properties/mod.rs @@ -25,6 +25,7 @@ use crate::gecko_bindings::structs::{nsCSSPropertyID, AnimatedPropertyID, RefPtr use crate::logical_geometry::WritingMode; use crate::parser::ParserContext; use crate::str::CssString; +use crate::stylesheets::CssRuleType; use crate::stylesheets::Origin; use crate::stylist::Stylist; use crate::values::{computed, serialize_atom_name}; @@ -486,8 +487,8 @@ impl PropertyId { fn allowed_in(&self, context: &ParserContext) -> bool { let id = match self.non_custom_id() { - // Custom properties are allowed everywhere - None => return true, + // Custom properties are allowed everywhere, except `position-try`. + None => return !context.nesting_context.rule_types.contains(CssRuleType::PositionTry), Some(id) => id, }; id.allowed_in(context) diff --git a/servo/components/style_traits/lib.rs b/servo/components/style_traits/lib.rs index 511143730f63..c2f5314ce3be 100644 --- a/servo/components/style_traits/lib.rs +++ b/servo/components/style_traits/lib.rs @@ -170,6 +170,8 @@ pub enum StyleParseErrorKind<'i> { AnimationPropertyInKeyframeBlock, /// The property is not allowed within a page rule. NotAllowedInPageRule, + /// `!important` declarations are disallowed in `@position-try`. + PositionTryUnexpectedImportantDeclaration, } impl<'i> From> for StyleParseErrorKind<'i> { diff --git a/servo/ports/geckolib/error_reporter.rs b/servo/ports/geckolib/error_reporter.rs index 3115684ff0fe..1170f5875b7e 100644 --- a/servo/ports/geckolib/error_reporter.rs +++ b/servo/ports/geckolib/error_reporter.rs @@ -276,6 +276,9 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { StyleParseErrorKind::OtherInvalidValue(_) => { (cstr!("PEValueParsingError"), Action::Drop) }, + StyleParseErrorKind::PositionTryUnexpectedImportantDeclaration => { + (cstr!("PEPositionTryImportantDeclError"), Action::Drop) + }, _ => (cstr!("PEUnknownProperty"), Action::Drop), }, ContextualParseError::UnsupportedPropertyDeclaration(..) => { diff --git a/testing/web-platform/meta/css/css-anchor-position/at-position-try-allowed-declarations.html.ini b/testing/web-platform/meta/css/css-anchor-position/at-position-try-allowed-declarations.html.ini deleted file mode 100644 index c32f87ff7da2..000000000000 --- a/testing/web-platform/meta/css/css-anchor-position/at-position-try-allowed-declarations.html.ini +++ /dev/null @@ -1,9 +0,0 @@ -[at-position-try-allowed-declarations.html] - [--custom: 1px is disallowed in @position-try] - expected: FAIL - - [top: 1px !important is disallowed in @position-try] - expected: FAIL - - [inset: 1px !important is disallowed in @position-try] - expected: FAIL diff --git a/testing/web-platform/meta/css/css-anchor-position/at-position-try-cssom.html.ini b/testing/web-platform/meta/css/css-anchor-position/at-position-try-cssom.html.ini index 94fa272e5bd5..e998436951fe 100644 --- a/testing/web-platform/meta/css/css-anchor-position/at-position-try-cssom.html.ini +++ b/testing/web-platform/meta/css/css-anchor-position/at-position-try-cssom.html.ini @@ -4,6 +4,3 @@ [CSSPositionTryRule.style.setProperty setting allowed and disallowed properties] expected: FAIL - - [CSSPositionTryDescriptors.item] - expected: FAIL