1371112 - stylo: revalidation selectors involving pseudo-elements are broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

In the attached testcase, there should be text above the <hr> but it doesn't appear.

The reason for that is that when we go to match the two <div>s against the revalidation seletor [foo]::before they both fail to match, as a result of the changes in bug 1364850.  So we end up sharing style, and hence not having a ::before style for the second div.  Note that this means the comment I recently added to sharing/mod.rs is wrong, though I think it was right before bug 1364850 landed.

This got caught by some random mathml test that does a bunch of ::before/::after stuff when I did a try run with the patch for bug 1329361: we ended up sharing stuff incorrectly because all the revalidation selectors returned "doesn't match".

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix revalidation selectors when pseudo-elements are involved

MozReview-Commit-ID: 9YzRLX5CxyJ

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1371112 tests.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875569 [details]
Bug 1371112 tests.

https://reviewboard.mozilla.org/r/146984/#review151108

Ouch, good catch Boris.

I wonder if it would be better to just ignore pseudo-elements while matching (they're already ignored when inserting in the SelectorMap), that is using `ForStatelessPseudoElement`.

Right now we require a pseudo to be there for that, but that isn't necessary anymore (we no longer have that annoying `Selector`/`SelectorInner` distinction, and we can use `selector.has_pseudo_element()` quite easily).

In any case, r=me

Comment 4

[Boris Zbarsky [:bzbarsky]]

In fairness, this was caught by said mathml test, not me... ;)

> that is using `ForStatelessPseudoElement`.

Does that do the right thing?  Specifically, if we have a selector like "::before:hover" that will return false if we use ForStatelessPseudoElement.  I guess that might be ok in practice?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8875569 [details]
Bug 1371112 tests.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7459489e961d
tests.  r=emilio

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7459489e961d