Closed Bug 1364335 Opened 5 years ago Closed 5 years ago

stylo: Snapshot matching against states isn't right


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

53 Branch



Tracking Status
firefox55 --- fixed


(Reporter: bzbarsky, Assigned: bzbarsky)


(Blocks 1 open bug)



(1 file)

Consider this testcase:

    span { color: green; }
    :any-link + span { color: red; }
  <body onload="document.querySelector('a').removeAttribute('href');">
    <a href=""></a><span>This should be green</span>

This fails in stylo: the text is red.  I believe that happens because in servo/components/style/ in the match_non_ts_pseudo_class impl inside "impl<'a, E> Element for ElementWrapper<'a, E> where E: TElement," we have this bit:

        let flag = pseudo_class.state_flag();
        if flag.is_empty() { return something; }
        match self.snapshot().and_then(|s| s.state()) {
            Some(snapshot_state) => snapshot_state.contains(flag),

Now in this case, the pseudo-class is :any-link for which state_flag, for Gecko, returns IN_VISITED_OR_UNVISITED_STATE, which is IN_VISITED_STATE|IN_UNVISITED_STATE.  Since those two states are mutually exclusive, the snapshot can never contain them both, so snapshot_state.contains(flag) is false.  Hence we don't think there's any restyling to do on the <span> in the testcase.

I tried hacking the above to do snapshot_state.intersects(flag) and that fixes the above testcase.

Is that generally the right fix?  That is, what are the semantics of pseudo_class.state_flag() for purposes of this code?  The reason I ask is that I'm also trying to fix this code to handle :dir() correctly (bug 1364280) and for :dir the relevant states have a slightly different meaning than for :any-link.
Flags: needinfo?(emilio+bugs)
Yes, it is the right fix. Indeed, this used to be buggy in normal elements too not too long ago, and when I fixed it I left this comment[1]:

    // NB: It's important to use `intersect` instead of `contains`
    // here, to handle `:any-link` correctly.

Flags: needinfo?(emilio+bugs)
Assignee: nobody → bzbarsky
Priority: -- → P1
MozReview-Commit-ID: Jn9RLj59ntr
Attachment #8867285 - Flags: review?(emilio+bugs)
Comment on attachment 8867285 [details] [diff] [review]
tests.  Test dynamic changes to :any-link matching

Review of attachment 8867285 [details] [diff] [review]:

Attachment #8867285 - Flags: review?(emilio+bugs) → review+
Pushed by
tests.  Test dynamic changes to :any-link matching.  r=emilio
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.