servo: Merge #17858 - selectors: Fix note_next_sequence (from emilio:hover-quirk-broken); r=canaltinova
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 25 Jul 2017 15:42:37 -0700
changeset 419657 6a64e20610de04a40803749b446e48c2f39f79bd
parent 419656 d9fa2317c55a6480e534f6264a92042fea9cc2df
child 419658 215c7f7ff02642dce345f19610f0188c789cac57
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscanaltinova
milestone56.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
servo: Merge #17858 - selectors: Fix note_next_sequence (from emilio:hover-quirk-broken); r=canaltinova Selector-matching can backtrack when looking for ancestor combinators, so we can't just arrive there once and forget. Also, there was a further problem before this patch, which was that note_next_sequence was called _before_ checking whether all simple selectors matched, so the sequence you could get there is just wrong. Bug: 1384020 Reviewed-by: canaltinova MozReview-Commit-ID: 6g0ibb8EfBU Source-Repo: https://github.com/servo/servo Source-Revision: 316878b4898f5a3f3e23c513d8f95540f497971a
servo/components/selectors/matching.rs
--- a/servo/components/selectors/matching.rs
+++ b/servo/components/selectors/matching.rs
@@ -88,32 +88,32 @@ impl<'a, 'b, Impl> LocalMatchingContext<
             nesting_level: 0,
             // We flip this off once third sequence is reached.
             hover_active_quirk_disabled: selector.has_pseudo_element(),
         }
     }
 
     /// Updates offset of Selector to show new compound selector.
     /// To be able to correctly re-synthesize main SelectorIter.
-    pub fn note_next_sequence(&mut self, selector_iter: &SelectorIter<Impl>) {
+    fn note_position(&mut self, selector_iter: &SelectorIter<Impl>) {
         if let QuirksMode::Quirks = self.shared.quirks_mode() {
             if self.selector.has_pseudo_element() && self.offset != 0 {
-                // This is the _second_ call to note_next_sequence,
+                // This is the _second_ call to note_position,
                 // which means we've moved past the compound
                 // selector adjacent to the pseudo-element.
                 self.hover_active_quirk_disabled = false;
             }
 
             self.offset = self.selector.len() - selector_iter.selector_length();
         }
     }
 
     /// Returns true if current compound selector matches :active and :hover quirk.
     /// https://quirks.spec.whatwg.org/#the-active-and-hover-quirk
-    pub fn active_hover_quirk_matches(&mut self) -> bool {
+    pub fn active_hover_quirk_matches(&self) -> bool {
         if self.shared.quirks_mode() != QuirksMode::Quirks {
             return false;
         }
 
         // Don't allow it in recursive selectors such as :not and :-moz-any.
         if self.nesting_level != 0 {
             return false;
         }
@@ -485,21 +485,22 @@ pub fn matches_complex_selector<E, F>(mu
         // The only other parser-allowed Component in this sequence is a state
         // class. We just don't match in that case.
         if let Some(s) = iter.next() {
             debug_assert!(matches!(*s, Component::NonTSPseudoClass(..)),
                           "Someone messed up pseudo-element parsing");
             return false;
         }
 
-        // Advance to the non-pseudo-element part of the selector, and inform the context.
+        // Advance to the non-pseudo-element part of the selector, and inform
+        // the context.
         if iter.next_sequence().is_none() {
             return true;
         }
-        context.note_next_sequence(&mut iter);
+        context.note_position(&iter);
     }
 
     match matches_complex_selector_internal(iter,
                                             element,
                                             context,
                                             &mut RelevantLinkStatus::Looking,
                                             flags_setter) {
         SelectorMatchingResult::Matched => true,
@@ -521,18 +522,16 @@ fn matches_complex_selector_internal<E, 
     let matches_all_simple_selectors = selector_iter.all(|simple| {
         matches_simple_selector(simple, element, context, &relevant_link, flags_setter)
     });
 
     debug!("Matching for {:?}, simple selector {:?}, relevant link {:?}",
            element, selector_iter, relevant_link);
 
     let combinator = selector_iter.next_sequence();
-    // Inform the context that the we've advanced to the next compound selector.
-    context.note_next_sequence(&mut selector_iter);
     let siblings = combinator.map_or(false, |c| c.is_sibling());
     if siblings {
         flags_setter(element, HAS_SLOW_SELECTOR_LATER_SIBLINGS);
     }
 
     if !matches_all_simple_selectors {
         return SelectorMatchingResult::NotMatchedAndRestartFromClosestLaterSibling;
     }
@@ -562,16 +561,18 @@ fn matches_complex_selector_internal<E, 
                 }
             };
 
             loop {
                 let element = match next_element {
                     None => return candidate_not_found,
                     Some(next_element) => next_element,
                 };
+                // Note in which compound selector are we currently.
+                context.note_position(&selector_iter);
                 let result = matches_complex_selector_internal(selector_iter.clone(),
                                                                &element,
                                                                context,
                                                                relevant_link,
                                                                flags_setter);
                 match (result, c) {
                     // Return the status immediately.
                     (SelectorMatchingResult::Matched, _) => return result,