This is documented in https://quirks.spec.whatwg.org/#the-active-and-hover-quirk And test_hover_quirk.html checks this behavior.

The spec seems to indicate that it should mostly a Servo-side only change.

Nazim, can you take this one too?

Sure!

Sorry for long delay on this. I was pretty busy lately. With this patch I added :hover :active quirk but broke standards mode in the meantime :) Interestingly with that patch it's unable to match some elements(in active-and-hover-manual.html test) in standards mode despite this code is being surrounded by a QuirksMode if. The first thing that comes to my mind is the wrong propagation of QuirksMode. I'll look into that now but I wanted to get some early feedback about this code. I needed to pass `ComplexSelector` to `match_non_ts_pseudo_class` method to be able to get all these information for quirk. I hope that won't be a performance issue.

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review149238 So, in general this is great. My one issue with this patch is passing the ComplexSelector down into SimpleSelector matching. I think this is undesirable for the following reasons: * It breaks that abstraction boundary. * It potentially causes us to recompute the same information (whether the hover quirk matches) several times. * It breaks the patches I'm writing now, which enable selector matching with an offset into the selector. With my approach there, we create an iterator at the entry point of selector matching to represent the effective selector. So instead, I think we should make a struct called LocalMatchingContext, which is per-element, and instantiated at the top of matches_complex_selector. It would contain a pointer to the MatchContext, and also have a method to compute whether the hover quirk matches (and cache the result in an Option<bool> within the struct). Then, we'd pass the LocalMatchingContext instead of the MatchingContext to all the callees, since they can use it to find the MatchingContext when needed, as well as compute whether the hover quirk matches. Make sense?

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review149414 ::: servo/components/selectors/matching.rs:177 (Diff revision 2) > +pub struct LocalMatchingContext<'a, 'b: 'a> { > + pub matching_context: &'a mut MatchingContext<'b>, I think we can just use the same lifetime for both. ::: servo/components/selectors/matching.rs:411 (Diff revision 2) > + let active_hover_quirk = if let QuirksMode::Quirks = context.quirks_mode { > + Some(SelectorImpl::active_hover_quirk_matches(complex_selector.iter())) > + } else { > + None > + }; We definitely shouldn't compute this unconditionally. The idea was that LocalMatchingContext should hold a borrow of the ComplexSelector, such that it can generate the answer in response to a method call. ::: servo/components/style/gecko/selector_parser.rs:212 (Diff revision 2) > + fn active_hover_quirk_matches(mut iter: SelectorIter<Self>) -> bool > + { Can we move the iteration to the caller, and just have active_hover_quirk_matches operate on &Component::NonTSPseudoClass instead?

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review151396 ::: servo/components/selectors/matching.rs:177 (Diff revision 4) > } > > +/// Holds per-element data alongside a pointer to MatchingContext. > +pub struct LocalMatchingContext<'a, 'b: 'a, 'c: 'a, E: SelectorImpl + 'c> > +{ > + pub matching_context: &'a mut MatchingContext<'b>, Let's call this 'shared', to match the terminology we use with LocalStyleContext. ::: servo/components/selectors/matching.rs:178 (Diff revision 4) > > +/// Holds per-element data alongside a pointer to MatchingContext. > +pub struct LocalMatchingContext<'a, 'b: 'a, 'c: 'a, E: SelectorImpl + 'c> > +{ > + pub matching_context: &'a mut MatchingContext<'b>, > + pub selector_iter: &'a mut SelectorIter<'c, E>, I don't understand why this needs to be a reference, and why it can't just be a member. More generally, the way this patch calls next_sequence() twice is wasteful. What we should do instead is to have a method on LocalMatchingContext called note_next_sequence(iter: &SelectorIter). This method would check whether we're in quirks mode, and if so, clone |iter| into a local Option<SelectorIter>. That way, the overhead is restricted to QuirksMode, and it's cheaper (since we just clone the iterator, rather than walking the sequence a second time). ::: servo/components/selectors/matching.rs:179 (Diff revision 4) > +/// Holds per-element data alongside a pointer to MatchingContext. > +pub struct LocalMatchingContext<'a, 'b: 'a, 'c: 'a, E: SelectorImpl + 'c> > +{ > + pub matching_context: &'a mut MatchingContext<'b>, > + pub selector_iter: &'a mut SelectorIter<'c, E>, > + pub functional_class_argument: bool, I don't have a problem with this per se, but it needs documentation. I also think |within_functional_pseudo_class_argument| is a more descriptive name. ::: servo/components/style/gecko/selector_parser.rs:241 (Diff revision 4) > + fn active_hover_quirk_matches(component: &Component<Self>) -> bool > + { The duplication between gecko and servo here isn't great. Why not just have a method on NonTSPseudoClass called is_active_or_hover? That would also solve the problem of this method being inaccurately named.

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review151800 ::: servo/components/selectors/matching.rs:209 (Diff revision 5) > + > + /// 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<E>) { > + if let QuirksMode::Quirks = self.shared.quirks_mode { > + self.offset = self.selector.len() - selector_iter.len(); Hm, I'm not sure if there is a better way to do this. Tried to get a reference of the iterators but didn't work and I just went with that.

Comment on attachment 8876193 [details] Bug 1355724 - stylo: Fix propagation of quirks mode information to servo side https://reviewboard.mozilla.org/r/147630/#review151904 ::: layout/base/nsPresContext.cpp:1203 (Diff revision 1) > return; > } > > StyleSetHandle styleSet = mShell->StyleSet(); > + if (styleSet->IsServo()) { > + styleSet->AsServo()->SetCompatibilityMode(compatMode); This should probably happen before returning early for svg documents.

Comment on attachment 8876193 [details] Bug 1355724 - stylo: Fix propagation of quirks mode information to servo side https://reviewboard.mozilla.org/r/147630/#review151906 ::: servo/ports/geckolib/glue.rs:1523 (Diff revision 1) > > +#[no_mangle] > +pub extern "C" fn Servo_StyleSet_SetCompatibilityMode(raw_data: RawServoStyleSetBorrowed, > + quirks_mode: nsCompatibility) { > + let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); > + data.stylist.set_quirks_mode(quirks_mode.into()); Instead of doing all this though, could we make stylist.quirks_mode() a function instead, and look into the pres context (via the `Device`) in that case? That will ensure it's correct all the time afaict.

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review151908 r=me with those comments addressed. ::: servo/components/selectors/matching.rs:175 (Diff revision 5) > +pub struct LocalMatchingContext<'a, 'b: 'a, E: SelectorImpl> > +{ > + /// Shared `MatchingContext`. > + pub shared: &'a mut MatchingContext<'b>, Again, why not just make this MatchingContext<'a>? Lifetimes are stretchy. ::: servo/components/selectors/matching.rs:179 (Diff revision 5) > + /// Holds a Selector reference and an offset for :active and :hover quirk. > + pub selector: &'a Selector<E>, > + /// Offset is neccessary to re-synthesize SelectorIter for each compound > + /// selector when main SelectorIter calls next_sequence. > + offset: usize, Technically this member is just the selector reference, and the offset is below. Let's make this as follows: /// A reference to the base selector we're matching against. pub selector: &'a Selector<E>, /// The offset of the current compound selector being matched, kept up to date by /// the callees when the iterator is advanced. This, in conjunction with the selector /// reference above, allows callees to synthesize an iterator for the current compound /// selector on-demand. This is necessary because the primary iterator may already have /// been advanced partway through the current compound selector, and the callee may need /// the whole thing. ::: servo/components/selectors/matching.rs:213 (Diff revision 5) > + if let QuirksMode::Quirks = self.shared.quirks_mode { > + self.offset = self.selector.len() - selector_iter.len(); > + } > + } > + > + /// Returns true if current compound selector matches :active and :hover quirk. Link to the spec here? ::: servo/components/selectors/matching.rs:214 (Diff revision 5) > + self.offset = self.selector.len() - selector_iter.len(); > + } > + } > + > + /// Returns true if current compound selector matches :active and :hover quirk. > + pub fn active_hover_quirk_matches(&mut self) -> Option<bool> { I don't understand whey we need to return an Option<bool> here. The only consumer I see just cares about Some(true) vs None. So how about just returning true or false? ::: servo/components/selectors/matching.rs:505 (Diff revision 5) > + // We are telling LocalMatchingContext to check if it's in quirks mode, > + // and if so, re-synthesize current iterator. This additional iterator is > + // needed for :active and :hover quirk matches. This comment isn't really accurate, since we're not re-synthesizing per se. And we probably don't need to go into detail about what it's for. I'd just say: // Inform the context that the we've advanced to the next compound selector. ::: servo/components/selectors/matching.rs:543 (Diff revision 5) > + // We are telling LocalMatchingContext to check if it's in quirks mode, > + // and if so, re-synthesize current iterator. This additional iterator is > + // needed for :active and :hover quirk matches. Same for this comment. ::: servo/components/selectors/matching.rs:639 (Diff revision 5) > F: FnMut(&E, ElementSelectorFlags), > { > match *selector { > Component::Combinator(_) => unreachable!(), > Component::PseudoElement(ref pseudo) => { > - element.match_pseudo_element(pseudo, context) > + element.match_pseudo_element(pseudo, &mut context.shared) Why &mut here? ::: servo/components/style/gecko/wrapper.rs:1389 (Diff revision 5) > // NB: It's important to use `intersect` instead of `contains` > // here, to handle `:any-link` correctly. Now that any-link is handled separately, seems like we should change this. Probably not worth changing back to contains in case there's some other psuedo class in that list that depends on multiple bits (though I sure don't see one), but worth adjusting the comment here. How about: /// FIXME: This can/should probably be contains() now that any-link (which depends in multiple bits) is handled in its own case below. ::: servo/components/style/gecko/wrapper.rs:1398 (Diff revision 5) > + // Some means we checked before and it's in QuirksMode. Therefore > + // we don't need to check here again. I don't understand this comment. ::: servo/components/style/gecko/wrapper.rs:1401 (Diff revision 5) > + NonTSPseudoClass::Active | > + NonTSPseudoClass::Hover => { > + // Some means we checked before and it's in QuirksMode. Therefore > + // we don't need to check here again. > + if let Some(true) = context.active_hover_quirk_matches() { > + if !self.is_link() && !context.within_functional_pseudo_class_argument { How about moving the within_functional_pseudo_class_argument check into active_hover_quirk_matches? ::: servo/components/style/gecko/wrapper.rs:1404 (Diff revision 5) > + return false; > + } > + } I think the |return| here is a bit confusing. With all the simplifications I mention here, this should all be able to become: (!context.active_hover_quirk_matches() || self.is_link()) && self.get_state().contains(..) ::: servo/components/style/gecko/wrapper.rs:1407 (Diff revision 5) > + > + // NB: It's important to use `intersect` instead of `contains` > + // here, to handle `:any-link` correctly. > + self.get_state().intersects(pseudo_class.state_flag()) This can be contains() now, with the comment removed.

Comment on attachment 8876193 [details] Bug 1355724 - stylo: Fix propagation of quirks mode information to servo side Redirecting this to emilio, since he's already looked at it.

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review151978 ::: servo/components/selectors/matching.rs:111 (Diff revision 5) > +#[derive(Clone, PartialEq, Eq, Debug)] > +pub enum QuirksMode { > + /// Quirks mode. > + Quirks, > + /// Limited quirks mode. > + LimitedQuirks, Please whatever you end up doing here, coordinate it with Simon, which is doing similar work in https://github.com/servo/servo/pull/17213 ::: servo/components/selectors/matching.rs:192 (Diff revision 5) > + /// quirk shouldn't match. > + pub within_functional_pseudo_class_argument: bool, > +} > + > +impl<'a, 'b, E> LocalMatchingContext<'a, 'b, E> > + where E: SelectorImpl Don't call this parameter `E`, call it `Impl`, since we use `E` for `Element`. ::: servo/components/selectors/matching.rs:209 (Diff revision 5) > + > + /// 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<E>) { > + if let QuirksMode::Quirks = self.shared.quirks_mode { > + self.offset = self.selector.len() - selector_iter.len(); What exactly didn't work? A reference probably doesn't, but a clone of the iterator should work fine... Not a big deal, I guess. ::: servo/components/selectors/matching.rs:766 (Diff revision 5) > } > Component::Negation(ref negated) => { > - !negated.iter().all(|ss| matches_simple_selector(ss, element, context, relevant_link, flags_setter)) > + context.within_functional_pseudo_class_argument = true; > + let result = !negated.iter().all(|ss| matches_simple_selector(ss, element, context, > + relevant_link, flags_setter)); > + context.within_functional_pseudo_class_argument = false; This will still not work for nested stuff, right? If so, why?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #19) > This will still not work for nested stuff, right? If so, why? Oh right, you need to store the old value, and restore that.

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review151908 > Again, why not just make this MatchingContext<'a>? Lifetimes are stretchy. We still need to hold a mutable reference to it, right? when I try &mut MatchingContext<'a> rustc doesn't let met to not put a lifetime parameter to that reference. And &'a mut MatchingContext<'a> doesn't work because of conflicting lifetemes, unfortunately. > Why &mut here? Yes, it wasn't neccessary. I did this for another change but forgot to remove this. > I don't understand this comment. Nevermind that, that was about `Option<bool>` but since we remove it, it doesn't apply anymore

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review151978 > What exactly didn't work? A reference probably doesn't, but a clone of the iterator should work fine... Not a big deal, I guess. Cloning the iterator didn't work because of lifetime conflicts :/

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review152134 ::: servo/components/selectors/matching.rs:175 (Diff revision 6) > } > } > } > > +/// Holds per-element data alongside a pointer to MatchingContext. > +pub struct LocalMatchingContext<'a, 'b: 'a, Impl: SelectorImpl> nit: either add the bounds in a where clause, or move the brace to the previous line. ::: servo/components/selectors/matching.rs:220 (Diff revision 6) > + } > + > + /// 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 { > + if self.shared.quirks_mode == QuirksMode::Quirks && nit: It may look cleaner returning early: ``` if self.shared.quirks_mode != QuirksMode::Quirks || self.within_functional_pseudo_class_argument { return false; } // ... ``` ::: servo/components/selectors/parser.rs:430 (Diff revision 6) > pub fn from_vec(vec: Vec<Component<Impl>>, specificity_and_flags: u32) -> Self { > let header = HeaderWithLength::new(SpecificityAndFlags(specificity_and_flags), vec.len()); > Selector(Arc::into_thin(Arc::from_header_and_iter(header, vec.into_iter()))) > } > + > + pub fn len(&self) -> usize { nit: This is now public, so document what it represents exactly. ::: servo/components/selectors/parser.rs:448 (Diff revision 6) > /// returning the combinator if the sequence was found. > pub fn next_sequence(&mut self) -> Option<Combinator> { > self.next_combinator.take() > } > + > + pub fn len(&self) -> usize { nit: Use this implementing ExactSizeIterator instead. But it looks to me this is super-confusing, given this iterator relies on returning none as soon as it finds a combinator... Perhaps another name (`selector_length`?) would be cleaner. ::: servo/components/style/context.rs:67 (Diff revision 6) > LimitedQuirks, > /// No quirks mode. > NoQuirks, > } > > +impl From<QuirksMode> for SelectorQuirksMode { Why not just using the selectors quirks mode everywhere? If you don't want the hassle of of renaming imports you can do `pub use selectors::matching::QuirksMode`. ::: servo/components/style/gecko/selector_parser.rs:241 (Diff revision 6) > > type PseudoElement = PseudoElement; > type NonTSPseudoClass = NonTSPseudoClass; > + > + #[inline] > + fn is_active_or_hover(pseudo_class: &Self::NonTSPseudoClass) -> bool nit: brace to its own line. ::: servo/components/style/gecko/selector_parser.rs:243 (Diff revision 6) > type NonTSPseudoClass = NonTSPseudoClass; > + > + #[inline] > + fn is_active_or_hover(pseudo_class: &Self::NonTSPseudoClass) -> bool > + { > + match *pseudo_class { nit: You can use `matches!(*pseudo_class, PseudoClass::Active | PseudoClass::Hover)`. ::: servo/components/style/gecko/wrapper.rs:1398 (Diff revision 6) > - NonTSPseudoClass::Link => relevant_link.is_unvisited(self, context), > - NonTSPseudoClass::Visited => relevant_link.is_visited(self, context), > + NonTSPseudoClass::AnyLink => self.is_link(), > + NonTSPseudoClass::Link => relevant_link.is_unvisited(self, context.shared), > + NonTSPseudoClass::Visited => relevant_link.is_visited(self, context.shared), > + NonTSPseudoClass::Active | > + NonTSPseudoClass::Hover => { > + (!context.active_hover_quirk_matches() || self.is_link()) && I find this condition super-confusing to read, and could benefit of getting split. In particular, what about: ``` if context.active_hover_quirk_matches() && !self.is_link() { return false; } self.get_state().contains(pseudo_class.state_flag()); ``` ::: servo/components/style/restyle_hints.rs:769 (Diff revision 6) > -> bool > { > self.element.match_pseudo_element(pseudo_element, context) > } > > fn is_link(&self) -> bool { this can just be `self.element.is_link()`, the link pseudo-classes are mutually exclussive. ::: servo/components/style/servo/selector_parser.rs:304 (Diff revision 6) > type BorrowedLocalName = LocalName; > type BorrowedNamespaceUrl = Namespace; > + > + #[inline] > + fn is_active_or_hover(pseudo_class: &Self::NonTSPseudoClass) -> bool > + { nit: Brace + matches!. ::: servo/components/style/stylist.rs:739 (Diff revision 6) > // Bug 1364242: We need to add visited support for lazy pseudos > let mut declarations = ApplicableDeclarationList::new(); > let mut matching_context = > - MatchingContext::new(MatchingMode::ForStatelessPseudoElement, None); > + MatchingContext::new(MatchingMode::ForStatelessPseudoElement, > + None, > + self.quirks_mode.into()); Just using `selector`'s QuirksMode would allow you to get rid of all these `into()`.

Comment on attachment 8876193 [details] Bug 1355724 - stylo: Fix propagation of quirks mode information to servo side https://reviewboard.mozilla.org/r/147630/#review152136 r=me, with those. ::: layout/base/nsPresContext.cpp:1190 (Diff revision 2) > nsIDocument* doc = mShell->GetDocument(); > if (!doc) { > return; > } > > + nsCompatibility compatMode = CompatibilityMode(); You don't need this variable now, right? ::: layout/base/nsPresContext.cpp:1201 (Diff revision 2) > if (doc->IsSVGDocument()) { > // SVG documents never load quirk.css. > return; > } > > - bool needsQuirkSheet = CompatibilityMode() == eCompatibility_NavQuirks; > + bool needsQuirkSheet = compatMode == eCompatibility_NavQuirks; nit: You added extra space here, but as discussed above, you don't need to change this line, please restore it. ::: servo/ports/geckolib/glue.rs:1521 (Diff revision 2) > pub extern "C" fn Servo_StyleSet_Drop(data: RawServoStyleSetOwned) { > let _ = data.into_box::<PerDocumentStyleData>(); > } > > +#[no_mangle] > +pub extern "C" fn Servo_StyleSet_CompatModeChanged(raw_data: RawServoStyleSetBorrowed) { Please add a comment here about this happening always early enough in the document lifetime so we don't need to worry about invalidating the stylist... Though just in case using `data.stylesheets.force_dirty()` wouldn't hurt. ::: servo/ports/geckolib/glue.rs:1524 (Diff revision 2) > > +#[no_mangle] > +pub extern "C" fn Servo_StyleSet_CompatModeChanged(raw_data: RawServoStyleSetBorrowed) { > + let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); > + let quirks_mode = unsafe { > + (*(*data.stylist.device().pres_context).mDocument nit: Alignment here looks slightly broken.

Comment on attachment 8873605 [details] Bug 1355724 - stylo: Support :hover and :active quirk https://reviewboard.mozilla.org/r/144986/#review152134 > Why not just using the selectors quirks mode everywhere? > > If you don't want the hassle of of renaming imports you can do `pub use selectors::matching::QuirksMode`. We can't use use selector's QuirksMode because servo's quirks mode derives HeapSizeOf and selector's quirks mode doesn't. > I find this condition super-confusing to read, and could benefit of getting split. > > In particular, what about: > > ``` > if context.active_hover_quirk_matches() && !self.is_link() { > return false; > } > self.get_state().contains(pseudo_class.state_flag()); > ``` Actually I did something like this but changed after bholley's suggestion. I'm okay with both but we should get his opinions on this too.

I mostly wanted to get of the 'return', since it's not clear from local context whether that short-circuits any other logic or not. How about a compromise of: > if context.active_hover_quirk_matches() && !self.is_link() { > false > } else { > self.get_state().contains(pseudo_class.state_flag()); > }

Servo side: https://github.com/servo/servo/pull/17266

Comment on attachment 8876435 [details] Bug 1355724 - stylo: Update test expectations for :active and :hover quirk https://reviewboard.mozilla.org/r/147786/#review152166 ::: layout/style/test/stylo-failures.md:107 (Diff revision 1) > * :-moz-lwtheme-* bug 1367312 > * test_selectors.html `:-moz-lwtheme` [3] > * :-moz-window-inactive bug 1348489 > * test_selectors.html `:-moz-window-inactive` [2] > * Quirks mode support > - * test_hover_quirk.html: hover quirks bug 1355724 [6] > + * test_hover_quirk.html: hover quirks bug 1355724 [2] Why are there remaining failures? File another bug for those?

Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/958eeb68e32e stylo: Fix propagation of quirks mode information to servo side r=emilio https://hg.mozilla.org/integration/autoland/rev/145daf186ca7 stylo: Update test expectations for :active and :hover quirk r=emilio

https://hg.mozilla.org/mozilla-central/rev/958eeb68e32e https://hg.mozilla.org/mozilla-central/rev/145daf186ca7