1370502 - stylo: Implement ServoStyleRule::SelectorMatchesElement

Status: RESOLVED FIXED

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

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Raising to P1, this seems to be blocking more and more bits of DevTools.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1370502 - stylo: Implement ServoStyleRule::SelectorMatchesElement.

Review commit: https://reviewboard.mozilla.org/r/153720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153720/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8882634 [details]
Bug 1370502 - stylo: Implement ServoStyleRule::SelectorMatchesElement.

https://reviewboard.mozilla.org/r/153720/#review158928

::: servo/ports/geckolib/glue.rs:1283
(Diff revision 1)
> +                // matches the selector pseudo element type before proceeding.
> +                if let Some(ep) = selector_and_hashes.selector.pseudo_element() {
> +                    if *ep != p {
> +                        return false;
> +                    }
> +                }

We also want to return false here, in case the selector doesn't have a pseudo-element, right?

::: servo/ports/geckolib/glue.rs:1294
(Diff revision 1)
> +                    return false;
> +                }
> +            },
> +        };
> +
> +        let mut ctx = MatchingContext::new(MatchingMode::Normal, None, QuirksMode::NoQuirks);

I'm reasonably sure this should use `MatchingMode::ForStatelessPseudoElement` in the case there's an actual pseudo-element (but not otherwise, since we assert).

Also, the quirks mode should be gotten from `aElement->OwnerDoc()` to match gecko.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8882634 [details]
Bug 1370502 - stylo: Implement ServoStyleRule::SelectorMatchesElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153720/diff/1-2/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8882634 [details]
Bug 1370502 - stylo: Implement ServoStyleRule::SelectorMatchesElement.

https://reviewboard.mozilla.org/r/153720/#review159084

With those fixed, r=me

::: layout/style/ServoStyleRule.cpp:282
(Diff revision 2)
>  ServoStyleRule::SelectorMatchesElement(Element* aElement,
>                                         uint32_t aSelectorIndex,
>                                         const nsAString& aPseudo,
>                                         bool* aMatches)
>  {
> -  // TODO Bug 1370502
> +  nsCOMPtr<nsIAtom> pseudoElt = NS_Atomize(aPseudo);

Please verify this does the correct thing for the empty string, or special-case as appropriate.

::: servo/components/style/gecko/wrapper.rs:699
(Diff revision 2)
>          let node = self.as_node();
>          unsafe { Gecko_GetDocumentLWTheme(node.owner_doc()) }
>      }
> +
> +    /// Quirks mode getter.
> +    pub fn get_quirks_mode(&self) -> QuirksMode {

Let's call this `owner_document_quirks_mode` instead.

::: servo/ports/geckolib/glue.rs:1281
(Diff revision 2)
> +        match PseudoElement::from_pseudo_type(pseudo_type) {
> +            Some(p) => {
> +                // We need to make sure that the requested pseudo element type
> +                // matches the selector pseudo element type before proceeding.
> +                match selector_and_hashes.selector.pseudo_element() {
> +                    Some(ep) => if *ep == p {

This doesn't return false if `*ep != p`, right?

This should probably be:

```
Some(ep) if *ep == p => { /* .. */ }
_ => return false,
```

Also, perhaps `p` and `ep` aren't particularly good names, maybe `pseudo` and `selector_pseudo` or something like that?

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8882634 [details]
Bug 1370502 - stylo: Implement ServoStyleRule::SelectorMatchesElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153720/diff/2-3/

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8882634 [details]
Bug 1370502 - stylo: Implement ServoStyleRule::SelectorMatchesElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153720/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/936d01cd7b99
stylo: Implement ServoStyleRule::SelectorMatchesElement. r=emilio

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/936d01cd7b99