Auto merge of #17918 - heycam:rule-hierarchy, r=emilio

style: Don't remain in an invalid state when encountering an at-rule in the wrong place.

Currently, attempting to parse an at-rule that is out of place, such as
an @import rule after a regular style rule, will cause the parser state
to be set to Invalid.  This will cause any following at-rule to be
rejected until we encounter a regular style rule, at which point we'll
go back to the Body state.  There's nothing in the CSS specs about
needing to reject all following at-rules (or, as the comment above
Invalid says, ignoring the entire rest of the style sheet).

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17918)
<!-- Reviewable:end -->

components/style/stylesheets/mod.rs

@@ -255,18 +255,19 @@ impl CssRule {
             shared_lock: &shared_lock,
             loader: loader,
             state: state,
+            had_hierarchy_error: false,
             namespaces: Some(&mut *guard),
         };
 
-        match parse_one_rule(&mut input, &mut rule_parser) {
-            Ok(result) => Ok((result, rule_parser.state)),
-            Err(_) => {
-                Err(match rule_parser.state {
-                    State::Invalid => SingleRuleParseError::Hierarchy,
-                    _ => SingleRuleParseError::Syntax,
-                })
-            }
-        }
+        parse_one_rule(&mut input, &mut rule_parser)
+            .map(|result| (result, rule_parser.state))
+            .map_err(|_| {
+                if rule_parser.take_had_hierarchy_error() {
+                    SingleRuleParseError::Hierarchy
+                } else {
+                    SingleRuleParseError::Syntax
+                }
+            })
     }
 }
 

components/style/stylesheets/rule_parser.rs

@@ -47,6 +47,10 @@ pub struct TopLevelRuleParser<'a> {
     pub context: ParserContext<'a>,
     /// The current state of the parser.
     pub state: State,
+    /// Whether we have tried to parse was invalid due to being in the wrong
+    /// place (e.g. an @import rule was found while in the `Body` state). Reset
+    /// to `false` when `take_had_hierarchy_error` is called.
+    pub had_hierarchy_error: bool,
     /// The namespace map we use for parsing. Needs to start as `Some()`, and
     /// will be taken out after parsing namespace rules, and that reference will
     /// be moved to `ParserContext`.
@@ -71,6 +75,16 @@ impl<'b> TopLevelRuleParser<'b> {
     pub fn state(&self) -> State {
         self.state
     }
+
+    /// Returns whether we previously tried to parse a rule that was invalid
+    /// due to being in the wrong place (e.g. an @import rule was found after
+    /// a regular style rule).  The state of this flag is reset when this
+    /// function is called.
+    pub fn take_had_hierarchy_error(&mut self) -> bool {
+        let had_hierarchy_error = self.had_hierarchy_error;
+        self.had_hierarchy_error = false;
+        had_hierarchy_error
+    }
 }
 
 /// The current state of the parser.
@@ -84,9 +98,6 @@ pub enum State {
     Namespaces = 3,
     /// We're parsing the main body of the stylesheet.
     Body = 4,
-    /// We've found an invalid state (as, a namespace rule after style rules),
-    /// and the rest of the stylesheet should be ignored.
-    Invalid = 5,
 }
 
 #[derive(Clone, Debug)]
@@ -153,8 +164,8 @@ impl<'a, 'i> AtRuleParser<'i> for TopLevelRuleParser<'a> {
         match_ignore_ascii_case! { &*name,
             "import" => {
                 if self.state > State::Imports {
-                    self.state = State::Invalid;
                     // "@import must be before any rule but @charset"
+                    self.had_hierarchy_error = true;
                     return Err(StyleParseError::UnexpectedImportRule.into())
                 }
 
@@ -180,8 +191,8 @@ impl<'a, 'i> AtRuleParser<'i> for TopLevelRuleParser<'a> {
             },
             "namespace" => {
                 if self.state > State::Namespaces {
-                    self.state = State::Invalid;
                     // "@namespace must be before any rule but @charset and @import"
+                    self.had_hierarchy_error = true;
                     return Err(StyleParseError::UnexpectedNamespaceRule.into())
                 }
                 self.state = State::Namespaces;
@@ -221,14 +232,12 @@ impl<'a, 'i> AtRuleParser<'i> for TopLevelRuleParser<'a> {
             },
             // @charset is removed by rust-cssparser if it’s the first rule in the stylesheet
             // anything left is invalid.
-            "charset" => return Err(StyleParseError::UnexpectedCharsetRule.into()),
+            "charset" => {
+                self.had_hierarchy_error = true;
+                return Err(StyleParseError::UnexpectedCharsetRule.into())
+            }
             _ => {}
         }
-        // Don't allow starting with an invalid state
-        if self.state > State::Body {
-            self.state = State::Invalid;
-            return Err(StyleParseError::UnspecifiedError.into());
-        }
         self.state = State::Body;
 
         // "Freeze" the namespace map (no more namespace rules can be parsed

components/style/stylesheets/stylesheet.rs

@@ -337,6 +337,7 @@ impl Stylesheet {
             loader: stylesheet_loader,
             context: context,
             state: State::Start,
+            had_hierarchy_error: false,
             namespaces: Some(namespaces),
         };