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

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.

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",

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

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,6 +1618,13 @@ fn report_one_css_error<'i>(
                 return;
             }
         }
+        // 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)
@@ -1616,6 +1632,7 @@ fn report_one_css_error<'i>(
                 _ => StyleParseErrorKind::new_invalid(property.non_custom_id().unwrap().name(), error),
             };
         }
+    }
 
     let location = error.location;
     let error = ContextualParseError::UnsupportedPropertyDeclaration(slice, error, selectors);

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)

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<ValueParseErrorKind<'i>> for StyleParseErrorKind<'i> {

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(..) => {

testing/web-platform/meta/css/css-anchor-position/at-position-try-allowed-declarations.html.ini

@@ -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

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