stylo: revalidation selectors involving pseudo-elements are broken

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({regression})

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted file 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".
MozReview-Commit-ID: 9YzRLX5CxyJ
Attachment #8875568 - Flags: review?(emilio+bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8875568 - Attachment is obsolete: true
Attachment #8875568 - Flags: review?(emilio+bugs)
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
Attachment #8875569 - Flags: review?(emilio+bugs) → review+
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?
https://hg.mozilla.org/mozilla-central/rev/7459489e961d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.