Closed Bug 1371112 Opened 3 years ago Closed 3 years ago

stylo: revalidation selectors involving pseudo-elements are broken

Categories

(Core :: CSS Parsing and Computation, defect)

53 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached 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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.