Closed Bug 1364335 Opened 7 years ago Closed 7 years ago

stylo: Snapshot matching against states isn't right

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

tests. Test dynamic changes to :any-link matching
1.66 KB, patch
emilio
: review+
Details | Diff | Splinter Review 
Consider this testcase:

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

This fails in stylo: the text is red.  I believe that happens because in servo/components/style/restyle_hints.rs 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.
    self.get_state().intersects(pseudo_class.state_flag())

[1]: https://github.com/servo/servo/blob/master/components/style/gecko/wrapper.rs#L1152
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]:
-----------------------------------------------------------------

r=me
Attachment #8867285 - Flags: review?(emilio+bugs) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f401077e0b
tests.  Test dynamic changes to :any-link matching.  r=emilio
https://hg.mozilla.org/mozilla-central/rev/91f401077e0b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: