Bug 1345206 - Ignore non-margin properties in @page rule. r=xidorn draft
authorJ. Ryan Stinnett <jryans@gmail.com>
Tue, 04 Apr 2017 11:41:49 -0500
changeset 558638 e53b122fdd0f57698247ec56677d9d7396bb6eff
parent 558637 b8bd09a9df6cdc72a1e5a9f133e4421c29b2911b
child 558639 2e8b83ee117278d19eb5dd1641a494980009e182
push id52927
push userbmo:jryans@gmail.com
push dateFri, 07 Apr 2017 20:53:03 +0000
reviewersxidorn
bugs1345206
milestone55.0a1
Bug 1345206 - Ignore non-margin properties in @page rule. r=xidorn Extend Servo's @page parsing to match Gecko's CSS 2.2 behavior, where only margin properties are allowed in an @page rule. Other properties are ignored. MozReview-Commit-ID: IPYUlnkLYSb
servo/components/style/keyframes.rs
servo/components/style/properties/data.py
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/longhand/margin.mako.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/properties/shorthand/margin.mako.rs
servo/components/style/stylesheets.rs
servo/components/style/supports.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/keyframes.rs
+++ b/servo/components/style/keyframes.rs
@@ -13,17 +13,17 @@ use properties::{Importance, PropertyDec
 use properties::{PropertyDeclarationId, LonghandId, ParsedDeclaration};
 use properties::LonghandIdSet;
 use properties::animated_properties::TransitionProperty;
 use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
 use shared_lock::{SharedRwLock, SharedRwLockReadGuard, Locked, ToCssWithGuard};
 use std::fmt;
 use std::sync::Arc;
 use style_traits::ToCss;
-use stylesheets::{MemoryHoleReporter, Stylesheet};
+use stylesheets::{CssRuleType, MemoryHoleReporter, Stylesheet};
 
 /// A number from 0 to 1, indicating the percentage of the animation when this
 /// keyframe should run.
 #[derive(Debug, Copy, Clone, PartialEq, PartialOrd)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub struct KeyframePercentage(pub f32);
 
 impl ::std::cmp::Ord for KeyframePercentage {
@@ -398,17 +398,17 @@ impl<'a, 'b> AtRuleParser for KeyframeDe
     type AtRule = ParsedDeclaration;
 }
 
 impl<'a, 'b> DeclarationParser for KeyframeDeclarationParser<'a, 'b> {
     type Declaration = ParsedDeclaration;
 
     fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result<ParsedDeclaration, ()> {
         let id = try!(PropertyId::parse(name.into()));
-        match ParsedDeclaration::parse(id, self.context, input, true) {
+        match ParsedDeclaration::parse(id, self.context, input, true, CssRuleType::Keyframe) {
             Ok(parsed) => {
                 // In case there is still unparsed text in the declaration, we should roll back.
                 if !input.is_exhausted() {
                     Err(())
                 } else {
                     Ok(parsed)
                 }
             }
--- a/servo/components/style/properties/data.py
+++ b/servo/components/style/properties/data.py
@@ -92,17 +92,18 @@ def arg_to_bool(arg):
 
 
 class Longhand(object):
     def __init__(self, style_struct, name, spec=None, animation_type=None, derived_from=None, keyword=None,
                  predefined_type=None, custom_cascade=False, experimental=False, internal=False,
                  need_clone=False, need_index=False, gecko_ffi_name=None, depend_on_viewport_size=False,
                  allowed_in_keyframe_block=True, complex_color=False, cast_type='u8',
                  has_uncacheable_values=False, logical=False, alias=None, extra_prefixes=None, boxed=False,
-                 creates_stacking_context=False, fixpos_cb=False, abspos_cb=False):
+                 creates_stacking_context=False, fixpos_cb=False, abspos_cb=False,
+                 allowed_in_page_rule=False):
         self.name = name
         if not spec:
             raise TypeError("Spec should be specified for %s" % name)
         self.spec = spec
         self.keyword = keyword
         self.predefined_type = predefined_type
         self.ident = to_rust_ident(name)
         self.camel_case = to_camel_case(self.ident)
@@ -119,16 +120,17 @@ class Longhand(object):
         self.cast_type = cast_type
         self.logical = arg_to_bool(logical)
         self.alias = alias.split() if alias else []
         self.extra_prefixes = extra_prefixes.split() if extra_prefixes else []
         self.boxed = arg_to_bool(boxed)
         self.creates_stacking_context = arg_to_bool(creates_stacking_context)
         self.fixpos_cb = arg_to_bool(fixpos_cb)
         self.abspos_cb = arg_to_bool(abspos_cb)
+        self.allowed_in_page_rule = arg_to_bool(allowed_in_page_rule)
 
         # https://drafts.csswg.org/css-animations/#keyframes
         # > The <declaration-list> inside of <keyframe-block> accepts any CSS property
         # > except those defined in this specification,
         # > but does accept the `animation-play-state` property and interprets it specially.
         self.allowed_in_keyframe_block = allowed_in_keyframe_block \
             and allowed_in_keyframe_block != "False"
 
@@ -149,29 +151,31 @@ class Longhand(object):
         # copy of the computed value.
         #
         # See components/style/helpers/animated_properties.mako.rs.
         self.need_clone = need_clone or self.animatable
 
 
 class Shorthand(object):
     def __init__(self, name, sub_properties, spec=None, experimental=False, internal=False,
-                 allowed_in_keyframe_block=True, alias=None, extra_prefixes=None):
+                 allowed_in_keyframe_block=True, alias=None, extra_prefixes=None,
+                 allowed_in_page_rule=False):
         self.name = name
         if not spec:
             raise TypeError("Spec should be specified for %s" % name)
         self.spec = spec
         self.ident = to_rust_ident(name)
         self.camel_case = to_camel_case(self.ident)
         self.derived_from = None
         self.experimental = ("layout.%s.enabled" % name) if experimental else None
         self.sub_properties = sub_properties
         self.internal = internal
         self.alias = alias.split() if alias else []
         self.extra_prefixes = extra_prefixes.split() if extra_prefixes else []
+        self.allowed_in_page_rule = arg_to_bool(allowed_in_page_rule)
 
         # https://drafts.csswg.org/css-animations/#keyframes
         # > The <declaration-list> inside of <keyframe-block> accepts any CSS property
         # > except those defined in this specification,
         # > but does accept the `animation-play-state` property and interprets it specially.
         self.allowed_in_keyframe_block = allowed_in_keyframe_block \
             and allowed_in_keyframe_block != "False"
 
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -7,17 +7,17 @@
 #![deny(missing_docs)]
 
 use cssparser::{DeclarationListParser, parse_important};
 use cssparser::{Parser, AtRuleParser, DeclarationParser, Delimiter};
 use error_reporting::ParseErrorReporter;
 use parser::{ParserContext, log_css_error};
 use std::fmt;
 use style_traits::ToCss;
-use stylesheets::{Origin, UrlExtraData};
+use stylesheets::{CssRuleType, Origin, UrlExtraData};
 use super::*;
 #[cfg(feature = "gecko")] use properties::animated_properties::AnimationValueMap;
 
 /// A declaration [importance][importance].
 ///
 /// [importance]: https://drafts.csswg.org/css-cascade/#importance
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
@@ -608,39 +608,40 @@ pub fn append_serialization<'a, W, I, N>
 
 /// A helper to parse the style attribute of an element, in order for this to be
 /// shared between Servo and Gecko.
 pub fn parse_style_attribute(input: &str,
                              url_data: &UrlExtraData,
                              error_reporter: &ParseErrorReporter)
                              -> PropertyDeclarationBlock {
     let context = ParserContext::new(Origin::Author, url_data, error_reporter);
-    parse_property_declaration_list(&context, &mut Parser::new(input))
+    parse_property_declaration_list(&context, &mut Parser::new(input), CssRuleType::Style)
 }
 
 /// Parse a given property declaration. Can result in multiple
 /// `PropertyDeclaration`s when expanding a shorthand, for example.
 ///
 /// The vector returned will not have the importance set;
 /// this does not attempt to parse !important at all
 pub fn parse_one_declaration(id: PropertyId,
                              input: &str,
                              url_data: &UrlExtraData,
                              error_reporter: &ParseErrorReporter)
                              -> Result<ParsedDeclaration, ()> {
     let context = ParserContext::new(Origin::Author, url_data, error_reporter);
     Parser::new(input).parse_entirely(|parser| {
-        ParsedDeclaration::parse(id, &context, parser, false)
+        ParsedDeclaration::parse(id, &context, parser, false, CssRuleType::Style)
             .map_err(|_| ())
     })
 }
 
 /// A struct to parse property declarations.
 struct PropertyDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
+    rule_type: CssRuleType,
 }
 
 
 /// Default methods reject all at rules.
 impl<'a, 'b> AtRuleParser for PropertyDeclarationParser<'a, 'b> {
     type Prelude = ();
     type AtRule = (ParsedDeclaration, Importance);
 }
@@ -648,17 +649,17 @@ impl<'a, 'b> AtRuleParser for PropertyDe
 
 impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> {
     type Declaration = (ParsedDeclaration, Importance);
 
     fn parse_value(&mut self, name: &str, input: &mut Parser)
                    -> Result<(ParsedDeclaration, Importance), ()> {
         let id = try!(PropertyId::parse(name.into()));
         let parsed = input.parse_until_before(Delimiter::Bang, |input| {
-            ParsedDeclaration::parse(id, self.context, input, false)
+            ParsedDeclaration::parse(id, self.context, input, false, self.rule_type)
                 .map_err(|_| ())
         })?;
         let importance = match input.try(parse_important) {
             Ok(()) => Importance::Important,
             Err(()) => Importance::Normal,
         };
         // In case there is still unparsed text in the declaration, we should roll back.
         if !input.is_exhausted() {
@@ -667,21 +668,23 @@ impl<'a, 'b> DeclarationParser for Prope
         Ok((parsed, importance))
     }
 }
 
 
 /// Parse a list of property declarations and return a property declaration
 /// block.
 pub fn parse_property_declaration_list(context: &ParserContext,
-                                       input: &mut Parser)
+                                       input: &mut Parser,
+                                       rule_type: CssRuleType)
                                        -> PropertyDeclarationBlock {
     let mut block = PropertyDeclarationBlock::new();
     let parser = PropertyDeclarationParser {
         context: context,
+        rule_type: rule_type,
     };
     let mut iter = DeclarationListParser::new(input, parser);
     while let Some(declaration) = iter.next() {
         match declaration {
             Ok((parsed, importance)) => parsed.expand_push_into(&mut block, importance),
             Err(range) => {
                 let pos = range.start;
                 let message = format!("Unsupported property declaration: '{}'",
--- a/servo/components/style/properties/longhand/margin.mako.rs
+++ b/servo/components/style/properties/longhand/margin.mako.rs
@@ -10,10 +10,11 @@
     <%
         spec = "https://drafts.csswg.org/css-box/#propdef-margin-%s" % side[0]
         if side[1]:
             spec = "https://drafts.csswg.org/css-logical-props/#propdef-margin-%s" % side[1]
     %>
     ${helpers.predefined_type("margin-%s" % side[0], "LengthOrPercentageOrAuto",
                               "computed::LengthOrPercentageOrAuto::Length(Au(0))",
                               alias=maybe_moz_logical_alias(product, side, "-moz-margin-%s"),
-                              animation_type="normal", logical = side[1], spec = spec)}
+                              animation_type="normal", logical = side[1], spec = spec,
+                              allowed_in_page_rule=True)}
 % endfor
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -27,17 +27,17 @@ use font_metrics::FontMetricsProvider;
 #[cfg(feature = "servo")] use logical_geometry::{LogicalMargin, PhysicalSide};
 use logical_geometry::WritingMode;
 use media_queries::Device;
 use parser::{Parse, ParserContext};
 use properties::animated_properties::TransitionProperty;
 #[cfg(feature = "servo")] use servo_config::prefs::PREFS;
 use shared_lock::StylesheetGuards;
 use style_traits::ToCss;
-use stylesheets::{Origin, UrlExtraData};
+use stylesheets::{CssRuleType, Origin, UrlExtraData};
 #[cfg(feature = "servo")] use values::Either;
 use values::{HasViewportPercentage, computed};
 use cascade_info::CascadeInfo;
 use rule_tree::StrongRuleNode;
 #[cfg(feature = "servo")] use values::specified::BorderStyle;
 
 pub use self::declaration_block::*;
 
@@ -972,18 +972,22 @@ impl ParsedDeclaration {
     /// > The <declaration-list> inside of <keyframe-block> accepts any CSS property
     /// > except those defined in this specification,
     /// > but does accept the `animation-play-state` property and interprets it specially.
     ///
     /// This will not actually parse Importance values, and will always set things
     /// to Importance::Normal. Parsing Importance values is the job of PropertyDeclarationParser,
     /// we only set them here so that we don't have to reallocate
     pub fn parse(id: PropertyId, context: &ParserContext, input: &mut Parser,
-                 in_keyframe_block: bool)
+                 in_keyframe_block: bool, rule_type: CssRuleType)
                  -> Result<ParsedDeclaration, PropertyDeclarationParseError> {
+        debug_assert!(rule_type == CssRuleType::Keyframe ||
+                      rule_type == CssRuleType::Page ||
+                      rule_type == CssRuleType::Style,
+                      "Declarations are only expected inside a keyframe, page, or style rule.");
         match id {
             PropertyId::Custom(name) => {
                 let value = match input.try(|i| CSSWideKeyword::parse(context, i)) {
                     Ok(keyword) => DeclaredValueOwned::CSSWideKeyword(keyword),
                     Err(()) => match ::custom_properties::SpecifiedValue::parse(context, input) {
                         Ok(value) => DeclaredValueOwned::Value(value),
                         Err(()) => return Err(PropertyDeclarationParseError::InvalidValue),
                     }
@@ -999,16 +1003,21 @@ impl ParsedDeclaration {
                                 return Err(PropertyDeclarationParseError::AnimationPropertyInKeyframeBlock)
                             }
                         % endif
                         % if property.internal:
                             if context.stylesheet_origin != Origin::UserAgent {
                                 return Err(PropertyDeclarationParseError::UnknownProperty)
                             }
                         % endif
+                        % if not property.allowed_in_page_rule:
+                            if rule_type == CssRuleType::Page {
+                                return Err(PropertyDeclarationParseError::NotAllowedInPageRule)
+                            }
+                        % endif
 
                         ${property_pref_check(property)}
 
                         match longhands::${property.ident}::parse_declared(context, input) {
                             Ok(value) => {
                                 Ok(ParsedDeclaration::LonghandOrCustom(value))
                             },
                             Err(()) => Err(PropertyDeclarationParseError::InvalidValue),
@@ -1027,16 +1036,21 @@ impl ParsedDeclaration {
                             return Err(PropertyDeclarationParseError::AnimationPropertyInKeyframeBlock)
                         }
                     % endif
                     % if shorthand.internal:
                         if context.stylesheet_origin != Origin::UserAgent {
                             return Err(PropertyDeclarationParseError::UnknownProperty)
                         }
                     % endif
+                    % if not shorthand.allowed_in_page_rule:
+                        if rule_type == CssRuleType::Page {
+                            return Err(PropertyDeclarationParseError::NotAllowedInPageRule)
+                        }
+                    % endif
 
                     ${property_pref_check(shorthand)}
 
                     match input.try(|i| CSSWideKeyword::parse(context, i)) {
                         Ok(keyword) => {
                             Ok(ParsedDeclaration::${shorthand.camel_case}CSSWideKeyword(keyword))
                         },
                         Err(()) => {
@@ -1100,16 +1114,18 @@ pub enum PropertyDeclarationParseError {
     ExperimentalProperty,
     /// The property declaration contained an invalid value.
     InvalidValue,
     /// 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,
 }
 
 impl fmt::Debug for PropertyDeclaration {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         try!(self.id().to_css(f));
         try!(f.write_str(": "));
         self.to_css(f)
     }
--- a/servo/components/style/properties/shorthand/margin.mako.rs
+++ b/servo/components/style/properties/shorthand/margin.mako.rs
@@ -1,8 +1,9 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 <%namespace name="helpers" file="/helpers.mako.rs" />
 
 ${helpers.four_sides_shorthand("margin", "margin-%s", "specified::LengthOrPercentageOrAuto::parse",
-                               spec="https://drafts.csswg.org/css-box/#propdef-margin")}
+                               spec="https://drafts.csswg.org/css-box/#propdef-margin",
+                               allowed_in_page_rule=True)}
--- a/servo/components/style/stylesheets.rs
+++ b/servo/components/style/stylesheets.rs
@@ -256,16 +256,17 @@ pub enum CssRule {
     FontFace(Arc<Locked<FontFaceRule>>),
     Viewport(Arc<Locked<ViewportRule>>),
     Keyframes(Arc<Locked<KeyframesRule>>),
     Supports(Arc<Locked<SupportsRule>>),
     Page(Arc<Locked<PageRule>>),
 }
 
 #[allow(missing_docs)]
+#[derive(PartialEq, Eq, Copy, Clone)]
 pub enum CssRuleType {
     // https://drafts.csswg.org/cssom/#the-cssrule-interface
     Style               = 1,
     Charset             = 2,
     Import              = 3,
     Media               = 4,
     FontFace            = 5,
     Page                = 6,
@@ -1095,17 +1096,17 @@ impl<'a, 'b> AtRuleParser for NestedRule
             }
             AtRulePrelude::Keyframes(name) => {
                 Ok(CssRule::Keyframes(Arc::new(self.shared_lock.wrap(KeyframesRule {
                     name: name,
                     keyframes: parse_keyframe_list(&self.context, input, self.shared_lock),
                 }))))
             }
             AtRulePrelude::Page => {
-                let declarations = parse_property_declaration_list(self.context, input);
+                let declarations = parse_property_declaration_list(self.context, input, CssRuleType::Page);
                 Ok(CssRule::Page(Arc::new(self.shared_lock.wrap(PageRule(
                     Arc::new(self.shared_lock.wrap(declarations))
                 )))))
             }
         }
     }
 }
 
@@ -1118,15 +1119,15 @@ impl<'a, 'b> QualifiedRuleParser for Nes
             stylesheet_origin: self.stylesheet_origin,
             namespaces: self.namespaces,
         };
         SelectorList::parse(&selector_parser, input)
     }
 
     fn parse_block(&mut self, prelude: SelectorList<SelectorImpl>, input: &mut Parser)
                    -> Result<CssRule, ()> {
-        let declarations = parse_property_declaration_list(self.context, input);
+        let declarations = parse_property_declaration_list(self.context, input, CssRuleType::Style);
         Ok(CssRule::Style(Arc::new(self.shared_lock.wrap(StyleRule {
             selectors: prelude,
             block: Arc::new(self.shared_lock.wrap(declarations))
         }))))
     }
 }
--- a/servo/components/style/supports.rs
+++ b/servo/components/style/supports.rs
@@ -4,16 +4,17 @@
 
 //! [@supports rules](https://drafts.csswg.org/css-conditional-3/#at-supports)
 
 use cssparser::{parse_important, Parser, Token};
 use parser::ParserContext;
 use properties::{PropertyId, ParsedDeclaration};
 use std::fmt;
 use style_traits::ToCss;
+use stylesheets::CssRuleType;
 
 #[derive(Debug)]
 /// An @supports condition
 ///
 /// https://drafts.csswg.org/css-conditional-3/#at-supports
 pub enum SupportsCondition {
     /// `not (condition)`
     Not(Box<SupportsCondition>),
@@ -206,13 +207,13 @@ impl Declaration {
     /// https://drafts.csswg.org/css-conditional-3/#support-definition
     pub fn eval(&self, cx: &ParserContext) -> bool {
         let id = if let Ok(id) = PropertyId::parse((&*self.prop).into()) {
             id
         } else {
             return false
         };
         let mut input = Parser::new(&self.val);
-        let res = ParsedDeclaration::parse(id, cx, &mut input, /* in_keyframe */ false);
+        let res = ParsedDeclaration::parse(id, cx, &mut input, /* in_keyframe */ false, CssRuleType::Style);
         let _ = input.try(parse_important);
         res.is_ok() && input.is_exhausted()
     }
 }
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -74,17 +74,17 @@ use style::properties::{PropertyDeclarat
 use style::properties::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP;
 use style::properties::animated_properties::{AnimationValue, Interpolate, TransitionProperty};
 use style::properties::parse_one_declaration;
 use style::restyle_hints::{self, RestyleHint};
 use style::selector_parser::PseudoElementCascadeType;
 use style::sequential;
 use style::shared_lock::{SharedRwLock, SharedRwLockReadGuard, StylesheetGuards, ToCssWithGuard, Locked};
 use style::string_cache::Atom;
-use style::stylesheets::{CssRule, CssRules, ImportRule, MediaRule, NamespaceRule};
+use style::stylesheets::{CssRule, CssRules, CssRuleType, ImportRule, MediaRule, NamespaceRule};
 use style::stylesheets::{Origin, Stylesheet, StyleRule};
 use style::stylesheets::StylesheetLoader as StyleStylesheetLoader;
 use style::supports::parse_condition_or_declaration;
 use style::thread_state;
 use style::timer::Timer;
 use style::traversal::{ANIMATION_ONLY, UNSTYLED_CHILDREN_ONLY};
 use style::traversal::{resolve_style, DomTraversal, TraversalDriver, TraversalFlags};
 use style_traits::ToCss;
@@ -912,17 +912,17 @@ pub extern "C" fn Servo_ParseProperty(pr
         return RawServoDeclarationBlockStrong::null()
     };
     let value = unsafe { value.as_ref().unwrap().as_str_unchecked() };
 
     let url_data = unsafe { RefPtr::from_ptr_ref(&data) };
     let reporter = StdoutErrorReporter;
     let context = ParserContext::new(Origin::Author, url_data, &reporter);
 
-    match ParsedDeclaration::parse(id, &context, &mut Parser::new(value), false) {
+    match ParsedDeclaration::parse(id, &context, &mut Parser::new(value), false, CssRuleType::Style) {
         Ok(parsed) => {
             let global_style_data = &*GLOBAL_STYLE_DATA;
             let mut block = PropertyDeclarationBlock::new();
             parsed.expand_push_into(&mut block, Importance::Normal);
             Arc::new(global_style_data.shared_lock.wrap(block)).into_strong()
         }
         Err(_) => RawServoDeclarationBlockStrong::null()
     }