Closed Bug 1371963 Opened 7 years ago Closed 7 years ago

stylo: Fix remaining failures for :active and :hover quirk

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: canova, Assigned: canova)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1371963 - stylo: Update test expectations for :hover and :active quirk
59 bytes, text/x-review-board-request
bholley
: review+
Details 
We implemented :active and :hover quirk in bug 1355724. But we still need to fix remaining 2 failures.
The problem was the matching of pseudo elements in `ForStatelessPseudoElement` mode.
It has a special handling in another part of the code, and it was consuming the pseudo element without checking it for quirk in the first place. So I changed that flag we check while matching :hover and :active quirk to cover that case too and made that flag true when it goes to that specific path. We also had to clear that flag when we go into new compound selector.

Try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ed609b60445f0c8bb914e1b32c021850a010ba
Comment on attachment 8876925 [details]
Bug 1371963 - stylo: Fix :active and :hover quirk

https://reviewboard.mozilla.org/r/148248/#review152624

::: servo/components/selectors/matching.rs:192
(Diff revision 1)
> -    /// Holds a bool flag to see if LocalMatchingContext is within a functional
> -    /// pseudo class argument. This is used for pseudo classes like
> -    /// `:-moz-any` or `:not`. If this flag is true, :active and :hover
> -    /// quirk shouldn't match.
> -    pub within_functional_pseudo_class_argument: bool,
> +    /// Holds a bool flag to see whether :active and :hover quirk should try to
> +    /// match or not. This flag can only be true in these two cases:
> +    /// - LocalMatchingContext is currently within a functional pseudo class
> +    /// like `:-moz-any` or `:not`.
> +    /// - PseudoElements are encountered when matching mode is ForStatelessPseudoElement.
> +    pub dont_match_hover_active_quirk: bool,

Let's call this hover_active_quirk_disabled.

::: servo/components/selectors/matching.rs:503
(Diff revision 1)
>      if context.shared.matching_mode == MatchingMode::ForStatelessPseudoElement {
>          match *iter.next().unwrap() {
>              // Stateful pseudo, just don't match.
>              Component::NonTSPseudoClass(..) => return false,
>              Component::PseudoElement(..) => {
> +                context.dont_match_hover_active_quirk = true;

This handling seems pretty fragile.


How about handling this in LocalStyleContext::new() and note_next_sequence? Specifically:
* LocalStyleContext::new would set hover_active_quirk_disabled if selector.has_pseudo() is true.
* note_next_sequence would do the following:


if !selector.has_pseudo() {
  debug_assert!(!self.hover_active_quirk_disabled);
else if self.offset != 0 {
  // This is the _second_ call to note_next_sequence,
  // which means we've moved past the compound
  // selector adjacent to the pseudo-element.
  self.hover_active_quirk_disabled = true;
}
Attachment #8876925 - Flags: review?(bobbyholley) → review-
Comment on attachment 8876926 [details]
Bug 1371963 - stylo: Update test expectations for :hover and :active quirk

https://reviewboard.mozilla.org/r/148250/#review152628
Attachment #8876926 - Flags: review?(bobbyholley) → review+
Comment on attachment 8876925 [details]
Bug 1371963 - stylo: Fix :active and :hover quirk

https://reviewboard.mozilla.org/r/148248/#review152624

> This handling seems pretty fragile.
> 
> 
> How about handling this in LocalStyleContext::new() and note_next_sequence? Specifically:
> * LocalStyleContext::new would set hover_active_quirk_disabled if selector.has_pseudo() is true.
> * note_next_sequence would do the following:
> 
> 
> if !selector.has_pseudo() {
>   debug_assert!(!self.hover_active_quirk_disabled);
> else if self.offset != 0 {
>   // This is the _second_ call to note_next_sequence,
>   // which means we've moved past the compound
>   // selector adjacent to the pseudo-element.
>   self.hover_active_quirk_disabled = true;
> }

Oh, I wasn't aware that `has_pseudo_element` exists. This definitely looks better. Thanks!

But a side note, we can't add `debug_assert!(!self.hover_active_quirk_disabled);` line probably, since we are passing true if it's within a functional pseudo class like :-moz-any. This assertion will fail in that case.
Oh, actually we don't need the `has_pseudo_element` method since we have `shared.matching_mode == MatchingMode::ForStatelessPseudoElement` check. Removing it.
(In reply to Nazım Can Altınova [:canaltinova] from comment #9)
> Oh, actually we don't need the `has_pseudo_element` method since we have
> `shared.matching_mode == MatchingMode::ForStatelessPseudoElement` check.
> Removing it.

We do need it, because we may also match pseudo-element selectors in non-ForStatelessPseudoElement mode (that's just the mode that means that we've already dealt with the pseudo-element part at the rule-hash level). I _think_ that those other cases can never be QuirksMode, but I don't want to depend on that, hence the has_pseudo_element check.
Comment on attachment 8876925 [details]
Bug 1371963 - stylo: Fix :active and :hover quirk

https://reviewboard.mozilla.org/r/148248/#review152654

r=me with that changed.

::: servo/components/selectors/matching.rs:201
(Diff revision 3)
>      where Impl: SelectorImpl
>  {
>      /// Constructs a new `LocalMatchingContext`.
>      pub fn new(shared: &'a mut MatchingContext<'b>,
>                 selector: &'a Selector<Impl>) -> Self {
> +        let quirk_disabled = shared.matching_mode == MatchingMode::ForStatelessPseudoElement;

Change this to selector.has_pseudo_element(), and explain that we flip it off once the third sequence is reached (since we count the :: as a combinator).

::: servo/components/selectors/matching.rs:215
(Diff revision 3)
>  
>      /// 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>) {
>          if let QuirksMode::Quirks = self.shared.quirks_mode {
> +            if self.shared.matching_mode == MatchingMode::ForStatelessPseudoElement &&

Make this has_pseudo_element instead.
Attachment #8876925 - Flags: review?(bobbyholley) → review+
Attachment #8876925 - Attachment is obsolete: true
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a755c216c3ea
stylo: Update test expectations for :hover and :active quirk r=bholley
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/a755c216c3ea
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: