Bug 1466136: Don't look at the rule type from value parsing. r=hiro
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Jun 2018 17:11:00 +0200
changeset 421258 b1c44c3fe871fbf61be12d160ccca2c08b91751b
parent 421257 c58ec47c4b75852eaa89766d2dac16654074af24
child 421259 78a4d0d2dfb7421f7adb22d6d351b83115dd3b8c
push id34090
push userbtara@mozilla.com
push dateTue, 05 Jun 2018 09:30:51 +0000
treeherdermozilla-central@a358755643e9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1466136
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1466136: Don't look at the rule type from value parsing. r=hiro This would cause properties to change the value semantics between, e.g., @keyframes and non-@keyframes, which would be observable. It happens not to be observable since the animation-* and transition-* properties are not allowed in @keyframes, nor have bits in `contain`, and none of the two properties are allowed in @page. But I think it's the right thing to do. This still causes a quirk like a property value in chrome / user origins being potentially different if the value is specified via CSS var functions. But I think that is fine. MozReview-Commit-ID: GhoPt0I34oO
servo/components/style/properties/properties.mako.rs
servo/components/style/values/specified/box.rs
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -494,16 +494,21 @@ impl NonCustomPropertyId {
                 return false;
             }
             CssRuleType::Page if DISALLOWED_IN_PAGE_RULE.contains(self) => {
                 return false;
             }
             _ => {}
         }
 
+        self.allowed_in_ignoring_rule_type(context)
+    }
+
+
+    fn allowed_in_ignoring_rule_type(self, context: &ParserContext) -> bool {
         // The semantics of these are kinda hard to reason about, what follows
         // is a description of the different combinations that can happen with
         // these three sets.
         //
         // Experimental properties are generally controlled by prefs, but an
         // experimental property explicitly enabled in certain context (UA or
         // chrome sheets) is always usable in the context regardless of the
         // pref value.
@@ -1418,21 +1423,27 @@ impl UnparsedValue {
         quirks_mode: QuirksMode,
     ) -> PropertyDeclaration {
         ::custom_properties::substitute(&self.css, self.first_token_type, custom_properties)
         .ok()
         .and_then(|css| {
             // As of this writing, only the base URL is used for property
             // values.
             //
-            // FIXME(emilio): These bits are slightly fishy.
+            // NOTE(emilio): we intentionally pase `None` as the rule type here.
+            // If something starts depending on it, it's probably a bug, since
+            // it'd change how values are parsed depending on whether we're in a
+            // @keyframes rule or not, for example... So think twice about
+            // whether you want to do this!
+            //
+            // FIXME(emilio): ParsingMode is slightly fishy...
             let context = ParserContext::new(
                 Origin::Author,
                 &self.url_data,
-                Some(CssRuleType::Style),
+                None,
                 ParsingMode::DEFAULT,
                 quirks_mode,
             );
 
             let mut input = ParserInput::new(&css);
             Parser::new(&mut input).parse_entirely(|input| {
                 match self.from_shorthand {
                     None => longhand_id.parse_value(&context, input),
@@ -1669,16 +1680,34 @@ impl PropertyId {
 
         if !id.allowed_in(context) {
             return Err(());
         }
 
         Ok(id)
     }
 
+    /// Parses a property name, and returns an error if it's unknown or isn't
+    /// allowed in this context, ignoring the rule_type checks.
+    ///
+    /// This is useful for parsing stuff from CSS values, for example.
+    #[inline]
+    pub fn parse_ignoring_rule_type(
+        name: &str,
+        context: &ParserContext,
+    ) -> Result<Self, ()> {
+        let id = Self::parse_unchecked(name)?;
+
+        if !id.allowed_in_ignoring_rule_type(context) {
+            return Err(());
+        }
+
+        Ok(id)
+    }
+
     /// Returns a property id from Gecko's nsCSSPropertyID.
     #[cfg(feature = "gecko")]
     #[allow(non_upper_case_globals)]
     pub fn from_nscsspropertyid(id: nsCSSPropertyID) -> Result<Self, ()> {
         use gecko_bindings::structs::*;
         match id {
             % for property in data.longhands:
                 ${property.nscsspropertyid()} => {
@@ -1782,16 +1811,26 @@ impl PropertyId {
         let id = match self.non_custom_id() {
             // Custom properties are allowed everywhere
             None => return true,
             Some(id) => id,
         };
         id.allowed_in(context)
     }
 
+    #[inline]
+    fn allowed_in_ignoring_rule_type(&self, context: &ParserContext) -> bool {
+        let id = match self.non_custom_id() {
+            // Custom properties are allowed everywhere
+            None => return true,
+            Some(id) => id,
+        };
+        id.allowed_in_ignoring_rule_type(context)
+    }
+
     /// Whether the property supports the given CSS type.
     /// `ty` should a bitflags of constants in style_traits::CssType.
     pub fn supports_type(&self, ty: u8) -> bool {
         let id = self.non_custom_non_alias_id();
         id.map_or(0, |id| id.supported_types()) & ty != 0
     }
 
     /// Collect supported starting word of values of this property.
--- a/servo/components/style/values/specified/box.rs
+++ b/servo/components/style/values/specified/box.rs
@@ -460,17 +460,17 @@ fn change_bits_for_longhand(longhand: Lo
     }
     if property_flags.contains(PropertyFlags::ABSPOS_CB) {
         flags |= WillChangeBits::ABSPOS_CB;
     }
     flags
 }
 
 fn change_bits_for_maybe_property(ident: &str, context: &ParserContext) -> WillChangeBits {
-    let id = match PropertyId::parse(ident, context) {
+    let id = match PropertyId::parse_ignoring_rule_type(ident, context) {
         Ok(id) => id,
         Err(..) => return WillChangeBits::empty(),
     };
 
     match id.as_shorthand() {
         Ok(shorthand) => {
             shorthand.longhands().fold(WillChangeBits::empty(), |flags, p| {
                 flags | change_bits_for_longhand(p)
@@ -772,17 +772,17 @@ impl ToCss for TransitionProperty {
 impl Parse for TransitionProperty {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         let location = input.current_source_location();
         let ident = input.expect_ident()?;
 
-        let id = match PropertyId::parse(&ident, context) {
+        let id = match PropertyId::parse_ignoring_rule_type(&ident, context) {
             Ok(id) => id,
             Err(..) => return Ok(TransitionProperty::Unsupported(
                 CustomIdent::from_ident(location, ident, &["none"])?,
             )),
         };
 
         Ok(match id.as_shorthand() {
             Ok(s) => TransitionProperty::Shorthand(s),