We need to support :visited pseudo-class. It should be done via appending the style context for visited element to the normal style context with nsStyleContext::SetStyleIfVisited.

Note that we'll need a coherent story for visited privacy here, per https://dbaron.org/mozilla/visited-privacy

When we discussed this on a call, the general consensus was that it might be easiest to rewrite :visited style rules to map them to separate internal properties. If we do that, then the cascade will take care of this for us, and we don't need to mess around with the separate rule node stuff. That said, we already have the nsStyleContext-based handling for this stuff, so it might end up being easier to reuse that. We'll need to investigate.

Ting-Yu, would you be interested in working on this? It's a bit involved, but hopefully the approach in comment 2 will make the setup a simpler than what we have right now in Gecko. Instead of having an entirely separate style context for visited styles, we'd just have a few special properties like "color-if-visited", and cascade those unconditionally. If you're interested, it's probably worth chatting with dbaron to discuss the specifics and make sure there aren't any issues we haven't thought of.

jryans is going to work on this.

Emilio noticed that Chrome is discussing[1] changing to an origin-based approach for :visited in an attempt to avoid the complexity that current :visited privacy protections in Gecko and other engines entails. My initial reaction is that while Chrome's proposal does sound simpler for the style system, for the timeline of the Stylo effort, we should continue with the current plan of copying Gecko's current approach rather than trying to change the state of things. :dbaron, any thoughts on this? What do you think of the idea Chrome is considering? [1]: https://bugs.chromium.org/p/chromium/issues/detail?id=713521

I think they've been discussing that for a few years, but the less risky thing for stylo is to stick to the approach we've been taking, and then consider the alternative as a separate project (particularly if Chrome decides to take that path).

Comment on attachment 8867372 [details] Bug 1328509 - GetContentStateForVisitedHandling can access state directly. https://reviewboard.mozilla.org/r/138914/#review142240

Comment on attachment 8867373 [details] Bug 1328509 - Look for relevant links while matching. https://reviewboard.mozilla.org/r/138916/#review142310 Super excited to review this! \o/ I have a few nits, and questions, tons of questions, since I think it's important to get it right :) ::: servo/components/selectors/matching.rs:9 (Diff revision 2) > + > use bloom::BloomFilter; > use parser::{CaseSensitivity, Combinator, ComplexSelector, Component, LocalName}; > use parser::{Selector, SelectorInner, SelectorIter}; > use std::borrow::Borrow; > +use std::default::Default; nit: I think it's imported automatically, so this shouldn't be needed. ::: servo/components/selectors/matching.rs:177 (Diff revision 2) > +/// ancestor link. > +#[derive(Default)] > +pub struct RelevantLink(bool); > + > +impl RelevantLink { > + pub fn is_visited<E>(&self, element: &E, context: &MatchingContext) -> bool Please add docs to these functions, mentioned how're they expected to be used. ::: servo/components/selectors/matching.rs:212 (Diff revision 2) > + > +/// Tracks whether we are currently looking for relevant links. A "relevant > +/// link" is the element being matched if it is a link or the nearest ancestor > +/// link. > +#[derive(Copy, Clone)] > +enum RelevantLinkMatcher { nit: I'd expect a type named `RelevantLinkMatcher` to be a `struct` instead of an `enum`. Also, I'm not sure the name reflects what it's actually doing. I think the usual pattern we use to have for this kind of things (like, when we look for viewport units in `cssparser`[1], would be something like a tri-state with: ``` enum RelevantLinkStatus { // Or a better name? Looking, Found, NotLooking, } ``` That being said, this is mostly a matter of design... What are your opinions on it? Perhaps it's worth to check with Simon in this regard. [1]: https://github.com/servo/rust-cssparser/blob/a22151de8a36a9715d0d5ffced68883dc0daa85b/src/tokenizer.rs#L211 ::: servo/components/style/restyle_hints.rs:690 (Diff revision 2) > } > > // We can ignore the selector flags, since they would have already > // been set during original matching for any element that might > // change its matching behavior here. > + // TODO jryans: Do we need to rematch in visited mode as well? I think this means we don't properly restyle when `:visited` changes (but no other state changes), could you test it? That means that when the link becomes visited, the link color wouldn't change in this case. I think it could be difficult/impossible to trigger with the mouse because of `:active` rules. We definitely don't need to re-match, but I think we shouldn't do any special handling of `:visited` during restyle hint processing. Just treating really visited links as visited, and really unvisited links as unvisited should work. That means we may need to use `RelevantLinkVisited` here, or add a third mode... Gecko seems to use `eLinksVisitedOrUnvisited` for the relevant piece of code, perhaps dbaron can elaborate on why. ::: servo/components/style/stylist.rs:940 (Diff revision 2) > let mut results = BitVec::new(); > self.selectors_for_cache_revalidation.lookup(*element, &mut |selector| { > results.push(matches_selector(selector, > element, > Some(bloom), > - &mut StyleRelations::empty(), > + &mut MatchingContext::default(), I'm not totally sure this is right either, this at least needs a justification. Why ignoring the visited state while trying to share style? Also, do we cache the visited style too in the style sharing cache? Why or why not? I _think_ it may be ok, given we check `element.get_state() == candidate_element.get_state()`, but worth checking. Also, this may mean we're there's a sort of timing attack that could be done here, I believe, though dbaron may double-check me here. If we have: ``` <a href="this/page/url">This page (known visited)</a><a href="https://amazon.com">Link to external page</a> ``` We'd be faster in the case amazon.com is visited (given we'd share style with the previous link, skipping all the selector-matching process). Not sure what's the best way to prevent this, if it's indeed a potential attack vector. Perhaps disallowing sharing across links? That'd be unfortunate :(. Also, not sure how much should we care about it (ideally, not much?) given there are similar (and better, I think) attacks, using something like mix-blend-mode[1]. [1]: https://bugs.chromium.org/p/chromium/issues/detail?id=712246

ni? David for the questions in the previous comment. If you could take a look, that'd be awesome :)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #24) > Comment on attachment 8867373 [details] > Bug 1328509 - Look for relevant links while matching. > ::: servo/components/style/restyle_hints.rs:690 > (Diff revision 2) > > } > > > > // We can ignore the selector flags, since they would have already > > // been set during original matching for any element that might > > // change its matching behavior here. > > + // TODO jryans: Do we need to rematch in visited mode as well? > > I think this means we don't properly restyle when `:visited` changes (but no > other state changes), could you test it? > > That means that when the link becomes visited, the link color wouldn't > change in this case. I think it could be difficult/impossible to trigger > with the mouse because of `:active` rules. > > We definitely don't need to re-match, but I think we shouldn't do any > special handling of `:visited` during restyle hint processing. Just treating > really visited links as visited, and really unvisited links as unvisited > should work. > > That means we may need to use `RelevantLinkVisited` here, or add a third > mode... Gecko seems to use `eLinksVisitedOrUnvisited` for the relevant piece > of code, perhaps dbaron can elaborate on why. So the place Gecko uses eLinksVisitedOrUnvisited is when we're deciding whether a state, attribute, etc., change requires us to do restyling. I think this is needed for two reasons: (1) not exposing visitedness through performance differences (2) doing restyling correctly in all cases Consider a case where there's a rule [rel="next"]:visited { color: blue }. If the rel attribute on a link changes between "next" and not-"next", then: (a) if the link is visited, we need to run restyling in order to draw the color change (b) if the link is *not* visited, then we also need to run restyling, in order to avoid a detectable performance difference from case (a). The use of eLinksVisitedOrUnvisited also means, I *think*, that HasStateDependentStyle will return correct results when either NS_EVENT_STATE_LINK or NS_EVENT_STATE_VISITED changes, so that we get correct restyling when a link changes between visited and unvisited. nsHTMLStyleSheet::HasStateDependentStyle also helps here, for the default styles. I haven't thought this bit through as carefully, though. > ::: servo/components/style/stylist.rs:940 > (Diff revision 2) > > let mut results = BitVec::new(); > > self.selectors_for_cache_revalidation.lookup(*element, &mut |selector| { > > results.push(matches_selector(selector, > > element, > > Some(bloom), > > - &mut StyleRelations::empty(), > > + &mut MatchingContext::default(), > > I'm not totally sure this is right either, this at least needs a > justification. > > Why ignoring the visited state while trying to share style? > > Also, do we cache the visited style too in the style sharing cache? Why or > why not? > > I _think_ it may be ok, given we check `element.get_state() == > candidate_element.get_state()`, but worth checking. > > Also, this may mean we're there's a sort of timing attack that could be done > here, I believe, though dbaron may double-check me here. So I'm not sure what the problem you're pointing out is. But two question I'd ask are (1) whether the data in the style sharing cache include the visited state or not (2) whether the data in the style sharing cache include whether the style has an associated if-visited style (which could be an issue if you could potentially share between links and non-links via the cache). (nsStyleContext does, but I believe the plan for servo was that it wouldn't.) > If we have: > > ``` > <a href="this/page/url">This page (known visited)</a><a > href="https://amazon.com">Link to external page</a> > ``` > > We'd be faster in the case amazon.com is visited (given we'd share style > with the previous link, skipping all the selector-matching process). > > Not sure what's the best way to prevent this, if it's indeed a potential > attack vector. Perhaps disallowing sharing across links? That'd be > unfortunate :(. That sounds like you're saying the visited state is part of what's cached -- I suppose that's unfortunate. I guess this may be a problem for Gecko as well...

Comment on attachment 8867373 [details] Bug 1328509 - Look for relevant links while matching. https://reviewboard.mozilla.org/r/138916/#review142310 > nit: I think it's imported automatically, so this shouldn't be needed. Thanks, I'll remove it! > Please add docs to these functions, mentioned how're they expected to be used. Oops, looks like I missed these. I'll add some docs! > nit: I'd expect a type named `RelevantLinkMatcher` to be a `struct` instead of an `enum`. > > Also, I'm not sure the name reflects what it's actually doing. > > I think the usual pattern we use to have for this kind of things (like, when we look for viewport units in `cssparser`[1], would be something like a tri-state with: > > ``` > enum RelevantLinkStatus { // Or a better name? > Looking, > Found, > NotLooking, > } > ``` > > That being said, this is mostly a matter of design... What are your opinions on it? Perhaps it's worth to check with Simon in this regard. > > [1]: https://github.com/servo/rust-cssparser/blob/a22151de8a36a9715d0d5ffced68883dc0daa85b/src/tokenizer.rs#L211 Renaming to `RelevantLinkStatus` seems clearer, I'll try that. I'll take a look at the tri-state approach, thanks for the idea! :) > I think this means we don't properly restyle when `:visited` changes (but no other state changes), could you test it? > > That means that when the link becomes visited, the link color wouldn't change in this case. I think it could be difficult/impossible to trigger with the mouse because of `:active` rules. > > We definitely don't need to re-match, but I think we shouldn't do any special handling of `:visited` during restyle hint processing. Just treating really visited links as visited, and really unvisited links as unvisited should work. > > That means we may need to use `RelevantLinkVisited` here, or add a third mode... Gecko seems to use `eLinksVisitedOrUnvisited` for the relevant piece of code, perhaps dbaron can elaborate on why. I created a manual visited restyle test to check this: 1. Load a page that uses visited that you have not visited yet (such as layout/reftests/css-visited/color-on-visited-1.html) 2. In a separate window, load the linked page Using debug logging on the restyle hints, I can confirm this causes visited to be the only state change, so the test case seems correct. When I first wrote these patches, we did indeed restyle visited for this test case, but that was actually happening by accident because of a UA sheet :any-link selector being handled incorrectly for snapshots, which :bz recently fixed in servo/servo#16841. Since we are producing both unvisited and visited styles (in the next patch) which are based separate matching results, I think we do need to match both: running only unvisited would miss the unvisited -> visited transition, and only visited would miss the visited -> unvisited transition. However, we don't need both all the time. I'll reuse logic similar to the next for cascading so that we only do the extra visited match if a relevant link was ever found. I'll add an extra patch to the queue to cover this work. Gecko's `eLinksVisitedOrUnvisited` mode means "report as dependent on both unvisited and visited". That doesn't seem quite like what we want at this spot in Servo because we are trying to test for changes. Matching in such a mode would mean you miss visited transitions since both states would match, so you'd detect no change. > I'm not totally sure this is right either, this at least needs a justification. > > Why ignoring the visited state while trying to share style? > > Also, do we cache the visited style too in the style sharing cache? Why or why not? > > I _think_ it may be ok, given we check `element.get_state() == candidate_element.get_state()`, but worth checking. > > Also, this may mean we're there's a sort of timing attack that could be done here, I believe, though dbaron may double-check me here. > > If we have: > > ``` > <a href="this/page/url">This page (known visited)</a><a href="https://amazon.com">Link to external page</a> > ``` > > We'd be faster in the case amazon.com is visited (given we'd share style with the previous link, skipping all the selector-matching process). > > Not sure what's the best way to prevent this, if it's indeed a potential attack vector. Perhaps disallowing sharing across links? That'd be unfortunate :(. > > Also, not sure how much should we care about it (ideally, not much?) given there are similar (and better, I think) attacks, using something like mix-blend-mode[1]. > > [1]: https://bugs.chromium.org/p/chromium/issues/detail?id=712246 It's definitely complex, and I don't understand all the pieces with confidence, so it's good to discuss! :) In the next patch in the series, the visited `ComputedValues` are stored inside the regular `ComputedValues`. During the cascade, we compute the unvisited / regular styles. If there is a relevant link during matching, we also compute the styles if visited. This has nothing to do with the element's _actual_ visited state, but only if there exists a relevant link during matching (meaning there is some visited style that could activate now or in the future). Since the style sharing cache stores the regular `ComputedValues`, it would also have the visited values as well for anything inserted into the cache. However, we are not trying to store the _state_ of visited or unvisited, just the two sets of styles. Unlike all other element states, a change in state of unvisited vs. visited does not change the style system's ouptut, since we have already computed both possible outputs up front. I think we can make the style sharing cache work without timing attacks: 1. When comparing element and candidate state, we should ignore both unvisited and visited in the comparison. (It has no bearing on the output.) 2. We should compare whether a relevant link exists for both element and candidate through revalidation: * Mark :link and :visited as pseudo-classes that need revalidation in `needs_cache_revalidation` * In `match_revalidation_selectors`, push a result for the unvisited match. If there is a relevant link, push an extra result for the visited match. Since the process does not depend on the element's actual visited state at all, it appears protected from timing attacks. Does the approach make sense to you? (The proposal I wrote here is not what the current patches do, so I'll extend them to do it assuming it makes sense.)

Comment on attachment 8867373 [details] Bug 1328509 - Look for relevant links while matching. https://reviewboard.mozilla.org/r/138916/#review144366 ::: servo/components/selectors/matching.rs:128 (Diff revision 3) > pub bloom_filter: Option<&'a BloomFilter>, > + /// Input that controls how matching for links is handled. > + pub visited_handling: VisitedHandlingMode, > + /// Output that records whether we encountered a "relevant link" while > + /// matching. > + relevant_link_found: bool, How does it sound to move `RelevantLinkStatus` into `MatchingContext`? I'd love to avoid adding more arguments to the `matches_xxx` functions. Also, perhaps this member variable could be removed altogether, and use `relevant_link_status == RelevantLinkStatus::Found` instead? (Of course we should have something like `stop_looking()` that would preserve the `Found` status, instead of just blanket-setting it to `NotLooking`). ::: servo/components/selectors/matching.rs:141 (Diff revision 3) > { > Self { > relations: StyleRelations::empty(), > matching_mode: matching_mode, > bloom_filter: bloom_filter, > + visited_handling: VisitedHandlingMode::default(), I'd prefer to be explicit here instead of using `default()`, given it's not too verbose, but it's not a big deal if you don't want to. ::: servo/components/selectors/matching.rs:211 (Diff revision 3) > + /// addtional links. > + fn examine_potential_link<E>(&self, element: &E, context: &mut MatchingContext) > + -> RelevantLinkStatus > + where E: Element, > + { > + match (*self, element.is_link()) { What about just returning early if we're not looking for relevant links? I think it'd be slightly easier to read: ``` if *self != RelevantLinkStatus::Looking { return *self; } if !element.is_link() { return RelevantLinkStatus::Looking; } context.relevant_link_found = true; RelevantLinkStatus::Found ``` Also, I think this would be a bit nicer if the `RelevantLinkStatus` was kept in the `MatchingContext`, so we wouldn't need the out-of-band setting of `relevant_link_found`.

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review144372 ::: servo/components/style/matching.rs:1152 (Diff revision 3) > - let mut matching_context = > - MatchingContext::new(MatchingMode::Normal, Some(bloom)); > + // Create a local `MatchingContext` based on other inputs from the one > + // passed in (a local context is used so that we only borrow the > + // bloom filter for this function). > + let mut local_matching_context = matching_context.clone(); > + local_matching_context.matching_mode = MatchingMode::Normal; > + local_matching_context.bloom_filter = Some(context.thread_local.bloom_filter.filter()); I'm somewhat concerned about this, because people have to remember to copy the inputs on one side, and the outputs on the other. Do you think we could pass `VisitedHandlingMode` down here and just create a `MatchingContext` here? Presumably what we want is returning some kind of `MatchingResults` that just returns the output we care about (rule node, relevant link found, and relations), instead of using the `MatchingContext` as an in-out parameter all across the file. I think that'd be cleaner and preferrable, but other thing we could do would be adding something like the following to `MatchingContext`: ``` fn with_filter<'b, F, R>(&mut self, filter: &'b BloomFilter, f: F) -> R where F: FnMut(&mut Self) -> R { self.bloom_filter = Some(filter); let ret = f(self); self.bloom_filter = None; ret } ``` I'd really really prefer the first, but wdyt? ::: servo/components/style/matching.rs:1184 (Diff revision 3) > + mut rules_adder: A, > + mut rules_taker: T) > + where A: FnMut(&mut ElementData, &PseudoElement, StrongRuleNode) -> bool, > + T: FnMut(&mut ElementData, &PseudoElement) -> Option<ComputedStyle> > { > + debug!("Match pseudos for {}, visited: {:?}", self.get_local_name(), Ditto here re. debug. ::: servo/components/style/matching.rs:1239 (Diff revision 3) > } > }); > > + // Copy `MatchingContext` outputs to the one passed in. > + matching_context.relations = local_matching_context.relations; > + matching_context.relevant_link_found = local_matching_context.relevant_link_found; So, on top of my comment above, it's unclear to me why we need to look for relevant links here. This is just done for "eager" pseudo-elements (at the moment, ::before and ::after, we may add `::first-line`/`::first-letter` too, but I don't think that changes the general equation here). I think there's no case where a pseudo-element would find a relevant link when the paren't hasn't, so I think this can get much simpler, just requiring a `VisitedHandlingMode` as an argument. Perhaps we could avoid this "copying in-params and out-params" around with that, just creating the `MatchingContext` where it was. I also wonder whether once that's done, we could just get rid of the new generic functions added, perhaps just matching on `VisitedHandlingMode`, which would be lovely :) ::: servo/components/style/matching.rs:1564 (Diff revision 3) > fn cascade_pseudos(&self, > context: &mut StyleContext<Self>, > - mut data: &mut ElementData) > + mut data: &mut ElementData, > + cascade_visited: CascadeVisitedMode) > { > + debug!("Cascade pseudos for {}, visited: {:?}", self.get_local_name(), We implement `Debug` on `TElement`, which also outputs the `id` so it's easier if you want to look at a specific element, so you can probably do: ``` debug!("Cascade pseudos for {:?}, visited: {:?}", self); ``` ::: servo/components/style/properties/properties.mako.rs:2494 (Diff revision 3) > pub fn apply_declarations<'a, F, I>(device: &Device, > is_root_element: bool, > iter_declarations: F, > inherited_style: &ComputedValues, > layout_parent_style: &ComputedValues, > + visited: Option<Arc<ComputedValues>>, let's name this `visited_style`, or `style_if_visited`, here and in the member.

Comment on attachment 8867375 [details] Bug 1328509 - Rule replacement for visited rules. https://reviewboard.mozilla.org/r/138920/#review144386 This looks fine, I think there are other places where we do replacements, but those are for animations only I think. If we wanted, I guess we could just rename `replace_rules` to `replace_rules_internal`, and make that call both with `Visited` and `Unvisited`.

Comment on attachment 8869270 [details] Bug 1328509 - Restyle hints for visited. https://reviewboard.mozilla.org/r/140826/#review144390 ::: servo/components/style/restyle_hints.rs:520 (Diff revision 1) > // FIXME(bz): How can I set this up so once Servo adds :dir() support we > // don't forget to update this code? > #[cfg(feature = "gecko")] > Component::NonTSPseudoClass(NonTSPseudoClass::Dir(ref s)) => dir_selector_to_state(s), > + // Consider :link and :visited as sensitive to both visited states to > + // avoid timing attacks by doing work in either case. So I think you mean either "we avoid the attack by doing work in both cases", or "we're preventing attacks that would work if we did work in one case but not other", I guess? That being said, it's unclear to me why this is needed... When would one state change without the other? Aren't they mutually exclussive? If so, whenever visited state changes, we'd always grab the same set of selectors, regardless of the presence of these two lines, right? ::: servo/components/style/restyle_hints.rs:815 (Diff revision 1) > > + // If we are sensitive to visitedness and the visited state changed, > + // we force a restyle here. Matching doesn't depend on the actual > + // visited state at all, so we can't look at matching results to > + // decide what to do for this case. > + if state_changes.intersects(IN_VISITED_OR_UNVISITED_STATE) { Do we allow sibling combinators depending on `:visited`? That is, do we allow selectors such as `:visited + foo` to affect the styling of the page? I think if not, we could just move this above the loop and do `return RestyleHint::subtree()`. ::: servo/components/style/restyle_hints.rs:854 (Diff revision 1) > + // state and visited handling mode when deciding if visited > + // matches. Instead, we are rematching here in case there is some > + // :visited selector whose matching result changed for some _other_ > + // element state or attribute. > + if now_context.relevant_link_found { > + let mut visited_context = MatchingContext::new_for_visited(VisitedHandlingMode::RelevantLinkVisited); I think doing twice the work if we found a relevant link may not be great (specially since this would happen every time you hover a link on the page). I'll need to think about this a bit more to see whether we can do better, but as a first idea, could we perhaps gate this re-selector-match-as-visited in whether the selector affects visited/unvisited style? (that is, `dep.sensitivities.states.intersects(IN_VISITED_OR_UNVISITED_STATE)`)? I think that'd address most of the perf concerns I have here.

Comment on attachment 8869271 [details] Bug 1328509 - Style sharing cache for visited. https://reviewboard.mozilla.org/r/140828/#review144392 ::: servo/components/style/gecko/selector_parser.rs:166 (Diff revision 1) > pub fn needs_cache_revalidation(&self) -> bool { > + // For :link and :visited, there are state flags, but we ignore these > + // because styles don't actually changed based on visitedness (since > + // both possibilities are computed up front). We need to revalidate to > + // apply the special visited matching rules. > + if matches!(*self, NonTSPseudoClass::Link | NonTSPseudoClass::Visited) { So it's not clear to me why this is needed? If both are computed upfront and in the same way, they would unevitably end up with the same set of styles regardless of visitedness, right? Could you point out an example that would fail without these lines? ::: servo/components/style/matching.rs:297 (Diff revision 1) > &mut |_, _| {})); > } > > let for_element = info.revalidation_match_results.as_ref().unwrap(); > let for_candidate = candidate.revalidation_match_results.as_ref().unwrap(); > - debug_assert!(for_element.len() == for_candidate.len()); > + for_element.len() == for_candidate.len() && for_element == for_candidate Why is this needed? The assert is pretty important, since it ensures that we end up looking at the same selector buckets everywhere. Otherwise we could end up with the same vectors referring to totally different selectors. Was this failing in your local testing? If so, happy to help you debug it (or debugging it myself), but this assertion is not negotiable ;) ::: servo/components/style/stylist.rs:1027 (Diff revision 1) > element, > &mut matching_context, > flags_setter)); > + // If there's a relevant link, match again in visited mode, just > + // like we do when cascading visited styles. > + if matching_context.relevant_link_found { Per the first comment, I don't think this is needed either.

Comment on attachment 8867377 [details] Bug 1328509 - Filter visited cascade to only visited dependent properties. https://reviewboard.mozilla.org/r/138924/#review144396 ::: servo/components/style/properties/properties.mako.rs:2593 (Diff revision 3) > }; > > + // Only a few properties are allowed to depend on the visited state > + // of links. When cascading visited styles, we can save time by > + // only processing these properties. > + let is_visited_dependent = matches!(longhand_id, Can we add a `LonghandId::is_visited_dependent` method instead? (I know we don't do that for early properties right now, and it's a shame)

Comment on attachment 8867376 [details] Bug 1328509 - Wire up visited values in ServoStyleSet::GetContext. https://reviewboard.mozilla.org/r/138922/#review144400 ::: layout/style/nsStyleContext.cpp:1151 (Diff revision 3) > // One style context has a style-if-visited and the other doesn't. > // Presume a difference. > hint |= nsChangeHint_RepaintFrame; > } else if (thisVis && !NS_IsHintSubset(nsChangeHint_RepaintFrame, hint)) { > + // TODO jryans: What does "differently" mean here? Should the sentence > + // about differently be removed? It feels pretty similar to Gecko... Indeed, you can probably remove this, and presumably the assertion too, once you make it return non-null. ::: layout/style/nsStyleContext.cpp:1234 (Diff revision 3) > > mozilla::NonOwningStyleContextSource StyleSource() const { > return mozilla::NonOwningStyleContextSource(mComputedValues); > } > > nsStyleContext* GetStyleIfVisited() { So this class just exists for the purpose of `CalcStyleDifference`. Presumably you can use some more template magic there to allow that function to handle a simple stack-allocated wrapper over ServoComputedValues, and use that there.

Comment on attachment 8867373 [details] Bug 1328509 - Look for relevant links while matching. https://reviewboard.mozilla.org/r/138916/#review144366 > How does it sound to move `RelevantLinkStatus` into `MatchingContext`? I'd love to avoid adding more arguments to the `matches_xxx` functions. > > Also, perhaps this member variable could be removed altogether, and use `relevant_link_status == RelevantLinkStatus::Found` instead? (Of course we should have something like `stop_looking()` that would preserve the `Found` status, instead of just blanket-setting it to `NotLooking`). I agree that, in general, it would be better to add less arguments to `matches` functions. However, I don't think it makes sense to move `RelevantLinkStatus` into `MatchingContext`. `relevant_link_found` in `MatchingContext` means "was a relevant link ever found for any selector we attempted for this element". In contrast, the `RelevantLinkStatus` is created anew for each _selector_ (in `matches_complex_selector`) because we want `is_visited` / `is_unvisited` to use that selector's specific relevant link state, not a global value from `MatchingContext`. I'll add comments to clarify the different purposes for each one. Is it okay to keep as-is given this info? > I'd prefer to be explicit here instead of using `default()`, given it's not too verbose, but it's not a big deal if you don't want to. Sure, that works for me, I wasn't sure which I preferred. > What about just returning early if we're not looking for relevant links? I think it'd be slightly easier to read: > > ``` > if *self != RelevantLinkStatus::Looking { > return *self; > } > > if !element.is_link() { > return RelevantLinkStatus::Looking; > } > > > context.relevant_link_found = true; > RelevantLinkStatus::Found > ``` > > Also, I think this would be a bit nicer if the `RelevantLinkStatus` was kept in the `MatchingContext`, so we wouldn't need the out-of-band setting of `relevant_link_found`. Haha, I thought about using this style while writing it actually... I wasn't sure whether `if` or `match` style was more readable here. I'll change to `if` like you suggest. :) As for `relevant_link_found`, we do need to keep separate values as I explained above. I'll add more comments here are about this.

:emilio, see my question and additional info about `RelevantLinkStatus` in comment 45. Is it okay to keep that as-is, given that `RelevantLinkStatus` and `relevant_link_found` are tracking different things?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #46) > :emilio, see my question and additional info about `RelevantLinkStatus` in > comment 45. Is it okay to keep that as-is, given that `RelevantLinkStatus` > and `relevant_link_found` are tracking different things? Yeah, that's fine for me, thanks for elaborating on it :)

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review144372 > I'm somewhat concerned about this, because people have to remember to copy the inputs on one side, and the outputs on the other. > > Do you think we could pass `VisitedHandlingMode` down here and just create a `MatchingContext` here? > > Presumably what we want is returning some kind of `MatchingResults` that just returns the output we care about (rule node, relevant link found, and relations), instead of using the `MatchingContext` as an in-out parameter all across the file. > > I think that'd be cleaner and preferrable, but other thing we could do would be adding something like the following to `MatchingContext`: > > ``` > fn with_filter<'b, F, R>(&mut self, filter: &'b BloomFilter, f: F) -> R > where F: FnMut(&mut Self) -> R > { > self.bloom_filter = Some(filter); > let ret = f(self); > self.bloom_filter = None; > ret > } > ``` > > I'd really really prefer the first, but wdyt? I agree the copying in and out isn't great, sorry about that! (I threw it in a bit hastily while rebasing when `bloom_filter` moved into `MatchingContext`.) I've changed `match_primary` to wrap the outputs in a new `MatchingResults` like you suggest. Both matching methods take in the `VisistedHandlingMode` and create the `MatchingContext` locally. Thanks for the ideas! > So, on top of my comment above, it's unclear to me why we need to look for relevant links here. This is just done for "eager" pseudo-elements (at the moment, ::before and ::after, we may add `::first-line`/`::first-letter` too, but I don't think that changes the general equation here). > > I think there's no case where a pseudo-element would find a relevant link when the paren't hasn't, so I think this can get much simpler, just requiring a `VisitedHandlingMode` as an argument. > > Perhaps we could avoid this "copying in-params and out-params" around with that, just creating the `MatchingContext` where it was. > > I also wonder whether once that's done, we could just get rid of the new generic functions added, perhaps just matching on `VisitedHandlingMode`, which would be lovely :) Yes, I agree with your assessment, we can reuse the relevant link value from the primary. I've changed accordingly, so we only passed in the `VisitedHandlingMode` and create the context in `match_pseudos`. We only use the rules here, so we don't need to return `MatchingResults`. I thought the adder and taker functions were kind of neat myself... But I guess it is a bit overly complex. :) I was able to remove them and match on the visited mode like you suggest. > We implement `Debug` on `TElement`, which also outputs the `id` so it's easier if you want to look at a specific element, so you can probably do: > > ``` > debug!("Cascade pseudos for {:?}, visited: {:?}", self); > ``` Ah, good point, I'll change to that! :) > let's name this `visited_style`, or `style_if_visited`, here and in the member. Okay, I went with `visited_style`. (I found it a bit strange when first reading this code that we have various args named `*_style` with the type `ComputedValues`... Naming them all `*_values` would seem a bit more clear to me, but I'll worry about that in a different bug!)

Comment on attachment 8867375 [details] Bug 1328509 - Rule replacement for visited rules. https://reviewboard.mozilla.org/r/138920/#review144386 Yes, there are two other calls to `update_rule_at_level` for animations with NAC pseudo elements, but I believe that case is okay to ignore for visited. I renamed to `replace_rules_internal` like you suggested.

Comment on attachment 8869270 [details] Bug 1328509 - Restyle hints for visited. https://reviewboard.mozilla.org/r/140826/#review144390 > So I think you mean either "we avoid the attack by doing work in both cases", or "we're preventing attacks that would work if we did work in one case but not other", I guess? > > That being said, it's unclear to me why this is needed... When would one state change without the other? Aren't they mutually exclussive? > > If so, whenever visited state changes, we'd always grab the same set of selectors, regardless of the presence of these two lines, right? Ah yes, you're right. Since they are exclusive, state changes will always have both visited and unvisited for a change in either direction. I'll revert this part. > Do we allow sibling combinators depending on `:visited`? > > That is, do we allow selectors such as `:visited + foo` to affect the styling of the page? > > I think if not, we could just move this above the loop and do `return RestyleHint::subtree()`. No, we don't allow them. Once we hit a sibiling combinator, we stop looking for relevant links, so it wouldn't match. I'll move this above the loop up like you suggest. > I think doing twice the work if we found a relevant link may not be great (specially since this would happen every time you hover a link on the page). > > I'll need to think about this a bit more to see whether we can do better, but as a first idea, could we perhaps gate this re-selector-match-as-visited in whether the selector affects visited/unvisited style? (that is, `dep.sensitivities.states.intersects(IN_VISITED_OR_UNVISITED_STATE)`)? I think that'd address most of the perf concerns I have here. Okay, I think it's reasonable to add this extra check, yes. I'll do that for now, and we can continue to think about optimizations.

Comment on attachment 8867377 [details] Bug 1328509 - Filter visited cascade to only visited dependent properties. https://reviewboard.mozilla.org/r/138924/#review144396 > Can we add a `LonghandId::is_visited_dependent` method instead? (I know we don't do that for early properties right now, and it's a shame) Good idea! I also converted `is_early_property` to a method as well, so we'll have the same approach for both.

Comment on attachment 8867376 [details] Bug 1328509 - Wire up visited values in ServoStyleSet::GetContext. https://reviewboard.mozilla.org/r/138922/#review144400 > Indeed, you can probably remove this, and presumably the assertion too, once you make it return non-null. Okay, I'll tweak my comment to mention the follow up bug 1364484 here. > So this class just exists for the purpose of `CalcStyleDifference`. Presumably you can use some more template magic there to allow that function to handle a simple stack-allocated wrapper over ServoComputedValues, and use that there. Okay, I'll give that a try in the follow bug 1364484.

Comment on attachment 8869271 [details] Bug 1328509 - Style sharing cache for visited. https://reviewboard.mozilla.org/r/140828/#review144392 > So it's not clear to me why this is needed? If both are computed upfront and in the same way, they would unevitably end up with the same set of styles regardless of visitedness, right? > > Could you point out an example that would fail without these lines? Okay, I think I agree, sorry. When I first thought about the style sharing cache, I missed that revalidation is happening only after we've already confirmed a bunch of other matches, like parent element, local name, etc. So it seems like in general the style sharing cache is safe, since both styles are computed up front. I'll reduce this patch to just the state comparison change. > Why is this needed? The assert is pretty important, since it ensures that we end up looking at the same selector buckets everywhere. Otherwise we could end up with the same vectors referring to totally different selectors. > > Was this failing in your local testing? If so, happy to help you debug it (or debugging it myself), but this assertion is not negotiable ;) No, I wasn't hitting the asserting, but I had assumed it could possibly fail since I made a change to conditional add an extra entry based on the relevant link status. However, we've already checked that parent element, local name, and link status all match, so the relevant link status should also match, and the length will be the same. I'll revert this part.

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review145254 ::: servo/components/style/matching.rs:1022 (Diff revision 3) > + let relevant_link_found = pseudo_matching_context.relevant_link_found; > + if relevant_link_found { > + let mut matching_context = > + MatchingContext::new_for_visited(VisitedHandlingMode::RelevantLinkVisited); > + self.match_pseudos(context, data, &mut matching_context, |d, p, r| { > + debug_assert!(d.styles_mut().pseudos.has(p), A crashtest points out that these assertions aren't right for a pseudo that has _only_ :link or _only_ :visited, such as `:visited:before { ... }` as the only selector. For the primary element, it's less of an issue, since we currently always create rule nodes for the primary, even if the declarations list is empty. I'm planning to allow adding a pseudo to the `EagerPseudoStyles` via either unvisited or visited styles, and only removing a pseudo once both have no rules.

Comment on attachment 8870200 [details] Bug 1328509 - Check failure patterns in assertSnapshots. https://reviewboard.mozilla.org/r/141654/#review145296

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review145428 ::: servo/components/style/data.rs:221 (Diff revisions 3 - 4) > + rule_tree: &RuleTree, > + declarations: &mut ApplicableDeclarationList, > + guards: &StylesheetGuards) > + -> bool { > + let rules = rule_tree.compute_rule_node(declarations, guards); > if self.has(pseudo) { nit: `if let Some(mut style) = self.get_mut(pseudo)` instead? ::: servo/components/style/data.rs:239 (Diff revisions 3 - 4) > - /// Returns true if the rule node changed. > - pub fn set_rules(&mut self, pseudo: &PseudoElement, rules: StrongRuleNode) -> bool { > - debug_assert!(self.has(pseudo)); > + /// Returns true if the psuedo-element was removed. > + pub fn remove_rules(&mut self, > + pseudo: &PseudoElement, > + rule_tree: &RuleTree) > + -> bool { > + if !self.has(pseudo) { nit: At the beginning of the scope: ``` { let mut style = match self.get_mut(pseudo) { Some(s) => s, None => return false, }; // ... } ``` ::: servo/components/style/data.rs:264 (Diff revisions 3 - 4) > + rule_tree: &RuleTree, > + declarations: &mut ApplicableDeclarationList, > + guards: &StylesheetGuards) > + -> bool { > + let rules = rule_tree.compute_rule_node(declarations, guards); > + if self.has(pseudo) { ditto re `if let`, here and everywhere else. ::: servo/components/style/data.rs:273 (Diff revisions 3 - 4) > + } > + // It's possible (though probably quite rare) to have only visited rules > + // for a pseudo, if you have something like `:visited:before` as the > + // only rule for that pseudo. For that case, we insert empty rules for > + // the unvisited case. > + let mut style = ComputedStyle::new_partial(rule_tree.root()); ::: servo/components/style/matching.rs:395 (Diff revisions 3 - 4) > } > > /// Various helper methods to ease navigating the style storage locations > /// depending on the current cascade mode. > impl CascadeVisitedMode { > + /// Returns whether there is a rules node based on the cascade mode. nit: a rule node ::: servo/components/style/matching.rs:997 (Diff revisions 3 - 4) > + relevant_link_found: false, > + } > + } > + > + /// Create `MatchingResults` from the output fields of `MatchingContext`. > + fn new_with_context(rules: StrongRuleNode, context: MatchingContext) -> Self { nit: `new_with_context` implies that it "stores" the context, which isn't true... Perhaps `new_from_context`? ::: servo/components/style/matching.rs:1049 (Diff revisions 3 - 4) > // cascade for regular styles because the visited ComputedValues > // are placed within the regular ComputedValues, which is immutable > // after the cascade. > - let relevant_link_found = pseudo_matching_context.relevant_link_found; > if relevant_link_found { > - let mut matching_context = > + self.match_pseudos(context, data, VisitedHandlingMode::RelevantLinkVisited); This is quite more understandable than before :) ::: servo/components/style/matching.rs:1239 (Diff revisions 3 - 4) > > if !applicable_declarations.is_empty() { > - let new_rules = > - compute_rule_node::<Self>(rule_tree, > + matches_different_pseudos |= match visited_handling { > + // Regular, unvisited matching > + VisitedHandlingMode::AllLinksUnvisited => { > + data.styles_mut().pseudos.add_rules(&pseudo, Perhaps worth to get the `VisitedHandlingMode` through `add_rules` and `remove_rules`? That'd make this easier to read I think. ::: servo/components/style/animation.rs:487 (Diff revision 4) > /* is_root = */ false, > iter, > previous_style, > previous_style, > /* cascade_info = */ None, > + None, nit: Leave a comment with `/* visited_style = */ None`. ::: servo/components/style/data.rs:43 (Diff revision 4) > + > + /// The rule node representing the ordered list of rules matched for this > + /// node if visited, only computed if there's a relevant link for this > + /// element. A element's "relevant link" is the element being matched if it > + /// is a link or the nearest ancestor link. > + visited_rules: Option<StrongRuleNode>, Could we leave a `TODO` in `StrongRuleNode`'s member, mentioning that we should use `NonZero` when stable to save space here? ::: servo/components/style/data.rs:272 (Diff revision 4) > + return false > + } > + // It's possible (though probably quite rare) to have only visited rules > + // for a pseudo, if you have something like `:visited:before` as the > + // only rule for that pseudo. For that case, we insert empty rules for > + // the unvisited case. Question: Do we really need to care about this case where we have `:visited::before`, but not any other rule? I doubt we honor it, right? (that's the kind of thing that would have a perf difference). From a quick test, we don't. If so, given we match unvisited styles before visited ones, can't we just ignore it completely when cascading the visited style? That would make this code quite less complex I think, even if it implies an existence check when cascading the visited styles. ::: servo/components/style/matching.rs:564 (Diff revision 4) > } > > fn cascade_internal(&self, > context: &StyleContext<Self>, > - primary_style: &ComputedStyle, > - eager_pseudo_style: Option<&ComputedStyle>) > + primary_style: &mut ComputedStyle, > + mut eager_pseudo_style: &mut Option<&mut ComputedStyle>, I'm failing to see when we're using the mutable references here, is it from a previous approach? If so, should this be reverted? ::: servo/components/style/matching.rs:1096 (Diff revision 4) > + // which is immutable after the cascade. > + if data.styles().primary.has_visited_rules() { > + self.cascade_primary(context, &mut data, CascadeVisitedMode::Visited); > + } > + self.cascade_primary(context, &mut data, CascadeVisitedMode::Unvisited); > + if data.styles().pseudos.iter() You can remove `pseudos.iter()`, and use `primary.has_visited_rules()` and `!pseudos.is_empty()` instead, potentially?

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review145436 ::: servo/components/style/data.rs:272 (Diff revision 4) > + return false > + } > + // It's possible (though probably quite rare) to have only visited rules > + // for a pseudo, if you have something like `:visited:before` as the > + // only rule for that pseudo. For that case, we insert empty rules for > + // the unvisited case. I don't think we should try to even do selector matching if the unvisited rules don't match... (That's what Gecko does, too) ::: servo/components/style/matching.rs:1222 (Diff revision 4) > + visited_handling); > > // Compute rule nodes for eagerly-cascaded pseudo-elements. > let mut matches_different_pseudos = false; > - let mut rule_nodes_changed = false; > SelectorImpl::each_eagerly_cascaded_pseudo_element(|pseudo| { So we should early-return here if visited_handling, is `RelevantLinkVisited` and the pseudo doesn't exist already, instead of minting one with an empty rule node. That would reduce some complexity introduced by this patch, I believe.

Comment on attachment 8870198 [details] Bug 1328509 - Add visited pseudo crashtests. https://reviewboard.mozilla.org/r/141650/#review145438 ::: layout/style/crashtests/crashtests.list:174 (Diff revision 1) > load 1331272.html > HTTP load 1333001-1.html > pref(dom.animations-api.core.enabled,true) load 1340344.html > load 1342316-1.html > load 1356601-1.html > +load content-on-link-before.html Perhaps `content-only-on-...` would be a more descriptive file name? Not a big deal anyway.

Comment on attachment 8869270 [details] Bug 1328509 - Restyle hints for visited. https://reviewboard.mozilla.org/r/140826/#review145440 This will need a bit of rebasing I think to account for recent changes by cam, but should be mostly renaming and moving stuff around, so r=me assuming the patch doesn't end up looking widely different.

Comment on attachment 8870199 [details] Bug 1328509 - Add visited reftest for attr / state change. https://reviewboard.mozilla.org/r/141652/#review145442 ::: layout/reftests/css-visited/color-on-visited-attr-1.html:10 (Diff revision 1) > +:link { color: fuchsia; } > +[bob]:visited { color: purple; } > +</style> > +<a href="visited-page.html">link</a> > +<script> > + // Attempt to set attribute _after_ the visited state takes effect Gah, is there a better way than a `setTimeout` for this? I'm afraid of the test breaking because of scheduling changes. Also, perhaps we should use `reftest-wait`? I don't see what would prevent the harness from taking the page snapshot before the timeout executing.

Comment on attachment 8870199 [details] Bug 1328509 - Add visited reftest for attr / state change. https://reviewboard.mozilla.org/r/141652/#review145444 I'm unsure about how will the harness behave with `setTimeout`, I'll defer the review of these to Xidorn. The test itself looks fine. ::: layout/style/test/test_visited_reftests.html:41 (Diff revision 1) > "!= color-on-link-1-ref.html color-on-visited-1-ref.html", > "== color-on-link-1.html color-on-link-1-ref.html", > "== color-on-link-before-1.html color-on-link-1-ref.html", > "== color-on-visited-1.html color-on-visited-1-ref.html", > + // Bug 1366908: Gecko misses this case currently, but it seems unintentional > + // "== color-on-visited-attr-1.html color-on-visited-1-ref.html", `fails-if(!stylo)` instead, if it's possible?

Comment on attachment 8869271 [details] Bug 1328509 - Style sharing cache for visited. https://reviewboard.mozilla.org/r/140828/#review145446 This has also moved recently (whoops), but r=me, the change should be equally trivial. Also, thanks _a lot_ for the excelent commit messages!

Comment on attachment 8867376 [details] Bug 1328509 - Wire up visited values in ServoStyleSet::GetContext. https://reviewboard.mozilla.org/r/138922/#review145448 r=me ::: layout/style/ServoStyleSet.cpp:225 (Diff revision 4) > - // XXXbholley: nsStyleSet does visited handling here. > + bool isLink = false; > + bool isVisitedLink = false; > + // If we need visited styles for callers where `aElementForAnimation` is null, > + // we can precompute these and pass them as flags, similar to nsStyleSet.cpp. > + if (aElementForAnimation) { > + if (nsCSSRuleProcessor::IsLink(aElementForAnimation)) { nit: you save a few lines using: `isLink = nsCSSRuleProcessor::IsLink(aElementForAnimation)`. ::: layout/style/ServoStyleSet.cpp:228 (Diff revision 4) > + // we can precompute these and pass them as flags, similar to nsStyleSet.cpp. > + if (aElementForAnimation) { > + if (nsCSSRuleProcessor::IsLink(aElementForAnimation)) { > + isLink = true; > + } > + if (nsCSSRuleProcessor::GetContentState(aElementForAnimation) same. ::: layout/style/nsCSSRuleProcessor.cpp:1271 (Diff revision 4) > +EventStates > +nsCSSRuleProcessor::GetContentState(Element* aElement) > +{ > + bool usingPrivateBrowsing = false; > + nsILoadContext* loadContext = aElement->OwnerDoc()->GetLoadContext(); > + if (loadContext) { nit: either `if (nsILoadContext* loadContext = ...)`, or move `usingPrivateBrowsing` after `loadContext`, and do: ``` bool usingPrivateBrowsing = loadContext && loadContext->UsePrivateBrowsing(); ```

Comment on attachment 8867377 [details] Bug 1328509 - Filter visited cascade to only visited dependent properties. https://reviewboard.mozilla.org/r/138924/#review145452 Lovely, thanks!

Comment on attachment 8867378 [details] Bug 1328509 - Enable visited tests for Stylo. https://reviewboard.mozilla.org/r/138926/#review145454

Comment on attachment 8870199 [details] Bug 1328509 - Add visited reftest for attr / state change. https://reviewboard.mozilla.org/r/141652/#review145444 > `fails-if(!stylo)` instead, if it's possible? I guess the simplest way is to push this test into the test list when the preference says stylo is enabled. That should be something doable.

Comment on attachment 8870199 [details] Bug 1328509 - Add visited reftest for attr / state change. https://reviewboard.mozilla.org/r/141652/#review145442 > Gah, is there a better way than a `setTimeout` for this? I'm afraid of the test breaking because of scheduling changes. > > Also, perhaps we should use `reftest-wait`? I don't see what would prevent the harness from taking the page snapshot before the timeout executing. css-visited tests are run by test_visited_reftests.html, and that test simply waits the test page to be identical to the reference page, or report timeout if that doesn't happen in a certain time. And that one doesn't support `reftest-wait`. But this test still looks fragile, and may not be testing what it is meant to test... because there is no guarantee that after 100ms the link would get the visited color. But we probably have to live with it... I don't have good idea otherwise either.

Comment on attachment 8870199 [details] Bug 1328509 - Add visited reftest for attr / state change. https://reviewboard.mozilla.org/r/141652/#review145470 Looks fine. r=me with enabling visited-attr test for stylo.

Comment on attachment 8870199 [details] Bug 1328509 - Add visited reftest for attr / state change. https://reviewboard.mozilla.org/r/141652/#review145442 > css-visited tests are run by test_visited_reftests.html, and that test simply waits the test page to be identical to the reference page, or report timeout if that doesn't happen in a certain time. And that one doesn't support `reftest-wait`. > > But this test still looks fragile, and may not be testing what it is meant to test... because there is no guarantee that after 100ms the link would get the visited color. But we probably have to live with it... I don't have good idea otherwise either. I agree the `setTimeout` is bad, but I don't see a great alternative for the moment. Really all of the visited tests are a bit fragile, since they just keep polling like :xidorn said. I filed bug 1367186 to consider some kind of event or notification once visited styles are applied, so that we can make many of the visited tests more reliable.

Comment on attachment 8870199 [details] Bug 1328509 - Add visited reftest for attr / state change. https://reviewboard.mozilla.org/r/141652/#review145444 > I guess the simplest way is to push this test into the test list when the preference says stylo is enabled. That should be something doable. Good idea, I'll condition this on the Stylo pref.

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review145436 > I don't think we should try to even do selector matching if the unvisited rules don't match... (That's what Gecko does, too) Ah, you're right. I'll reduce the complexity here so that we don't try to visited match for pseudos if unvisited did not match. > So we should early-return here if visited_handling, is `RelevantLinkVisited` and the pseudo doesn't exist already, instead of minting one with an empty rule node. > > That would reduce some complexity introduced by this patch, I believe. Right, that seems like it should work, thanks!

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review145428 > nit: `if let Some(mut style) = self.get_mut(pseudo)` instead? Thanks, fixed! > nit: At the beginning of the scope: > > ``` > { > let mut style = match self.get_mut(pseudo) { > Some(s) => s, > None => return false, > }; > > // ... > } > ``` This is simpler now, let me know in the next round if I should tweak it. > ditto re `if let`, here and everywhere else. Okay, I've applied this everywhere I used `if` and `unwrap` together. > nit: a rule node Thanks, fixed. > nit: `new_with_context` implies that it "stores" the context, which isn't true... Perhaps `new_from_context`? Sure, makes sense. :) > Perhaps worth to get the `VisitedHandlingMode` through `add_rules` and `remove_rules`? > > That'd make this easier to read I think. Okay, I pushed the visited handling down through add and remove with a wrapper. > nit: Leave a comment with `/* visited_style = */ None`. Okay, fixed. > Could we leave a `TODO` in `StrongRuleNode`'s member, mentioning that we should use `NonZero` when stable to save space here? Ah, cool, didn't know about `NonZero`. Added a comment! > Question: Do we really need to care about this case where we have `:visited::before`, but not any other rule? > > I doubt we honor it, right? (that's the kind of thing that would have a perf difference). From a quick test, we don't. > > If so, given we match unvisited styles before visited ones, can't we just ignore it completely when cascading the visited style? That would make this code quite less complex I think, even if it implies an existence check when cascading the visited styles. As discussed elsewhere, we don't actually need to match visited pseudo without unvisited, so I've changed this. > I'm failing to see when we're using the mutable references here, is it from a previous approach? If so, should this be reverted? This has evolved enough times that I am not sure anymore either... :) In any case, they aren't needed anymore, so I removed them. > You can remove `pseudos.iter()`, and use `primary.has_visited_rules()` and `!pseudos.is_empty()` instead, potentially? I don't think we want to check the primary's visited rules, becuase we could have visited rules only on the pseudo. However, I've also added an early return to both `cascade_primary` and `cascade_eager_pseudo` if there are no rules, so I think we can remove both of the checks here.

Comment on attachment 8870198 [details] Bug 1328509 - Add visited pseudo crashtests. https://reviewboard.mozilla.org/r/141650/#review145438 > Perhaps `content-only-on-...` would be a more descriptive file name? Not a big deal anyway. Okay, I added `only`.

Comment on attachment 8867376 [details] Bug 1328509 - Wire up visited values in ServoStyleSet::GetContext. https://reviewboard.mozilla.org/r/138922/#review145448 > nit: you save a few lines using: > > `isLink = nsCSSRuleProcessor::IsLink(aElementForAnimation)`. Ah, yeah, not sure why I didn't do that before... :) > same. Fixed. > nit: either `if (nsILoadContext* loadContext = ...)`, or move `usingPrivateBrowsing` after `loadContext`, and do: > > ``` > bool usingPrivateBrowsing = loadContext && loadContext->UsePrivateBrowsing(); > ``` Okay, cleaned up!

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review145788 Ok, I'm much more comfortable with this. Sorry it took so long! r=me with the comments below addressed, assuming try is happy. ::: servo/components/style/data.rs:230 (Diff revision 5) > + /// Returns true if the psuedo-element was removed. > + fn remove_unvisited_rules(&mut self, pseudo: &PseudoElement) -> bool { > + if !self.has(pseudo) { > + return false > + } > + self.take(pseudo); nit: This can become just `self.take(pseudo).is_some()`, I think. Also, s/psuedo/pseudo/ in the comment. ::: servo/components/style/data.rs:237 (Diff revision 5) > + } > + > + /// Adds the visited rule node for a given pseudo-element. It is assumed to > + /// already exist because unvisited styles should have been added first. > /// > - /// Returns true if the rule node changed. > + /// Returns true if the pseudo-element is new. There's no point in the return value now. Please clean this up before landing, here and below. ::: servo/components/style/matching.rs:383 (Diff revision 5) > + cascade_visited); > > // NB: Animations for pseudo-elements in Gecko are handled while > // traversing the pseudo-elements themselves. > if !context.shared.traversal_flags.for_animation_only() { > self.process_animations(context, Presumably you don't want to call `process_animations` nor `accumulate_damage` if the style is visited. The second one may be needed for now until you fix the `CalcStyleDifference` stuff that get's the style if visited, but worth leaving a `TODO`. ::: servo/components/style/matching.rs:415 (Diff revision 5) > - pseudo: &PseudoElement) { > + pseudo: &PseudoElement, > + cascade_visited: CascadeVisitedMode) { > debug_assert!(pseudo.is_eager()); > let (mut styles, restyle) = data.styles_and_restyle_mut(); > let mut pseudo_style = styles.pseudos.get_mut(pseudo).unwrap(); > - let old_values = pseudo_style.values.take(); > + if !cascade_visited.has_rules(pseudo_style) { leaving a comment here may be worth it, given it's probably non-obvious at a glance. ::: servo/components/style/stylist.rs:647 (Diff revision 5) > > // Read the comment on `precomputed_values_for_pseudo` to see why it's > // difficult to assert that display: contents nodes never arrive here > // (tl;dr: It doesn't apply for replaced elements and such, but the > // computed value is still "contents"). > + // Bug 1364242: We need to add visited support for lazy pseudos (I'm not sure how/if this is observable btw).

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review145788 > nit: This can become just `self.take(pseudo).is_some()`, I think. > > Also, s/psuedo/pseudo/ in the comment. Thanks, fixed. (I seem to misspell "pseudo" frequently...) > There's no point in the return value now. Please clean this up before landing, here and below. Returning a bool here gives it type parity with the unvisited version, so it's a little easier to call. I'll add a comment about the constant value. > Presumably you don't want to call `process_animations` nor `accumulate_damage` if the style is visited. > > The second one may be needed for now until you fix the `CalcStyleDifference` stuff that get's the style if visited, but worth leaving a `TODO`. At the moment, it appears we can skip both, so I changed to only do them in unvisited. I added a TODO like you mention to double check this for in the follow up bug. > leaving a comment here may be worth it, given it's probably non-obvious at a glance. Okay, added comments in both locations. > (I'm not sure how/if this is observable btw). Gecko supports something like: ``` ::-moz-selection { color: red } :visited::-moz-selection { color: blue } ``` The selected text in a visited link will be blue in Gecko, but currently red in Stylo. Not sure if there are any tests for these cases, I'll check in the follow up bug.

Comment on attachment 8867374 [details] Bug 1328509 - Match and cascade visited styles. https://reviewboard.mozilla.org/r/138918/#review145788 > Gecko supports something like: > > ``` > ::-moz-selection { color: red } > :visited::-moz-selection { color: blue } > ``` > > The selected text in a visited link will be blue in Gecko, but currently red in Stylo. Not sure if there are any tests for these cases, I'll check in the follow up bug. Heh, I tested this while writing my previous comment with `::selection`, which we don't parse. I guess that explains why I thought we didn't expose it, whoops.

Gecko side is ready to land once the Servo PR merges.

Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e20d3d5170c2 GetContentStateForVisitedHandling can access state directly. r=dbaron https://hg.mozilla.org/integration/autoland/rev/65bbc73c87aa Add visited pseudo crashtests. r=emilio https://hg.mozilla.org/integration/autoland/rev/c2ce10b8bc37 Add visited reftest for attr / state change. r=xidorn https://hg.mozilla.org/integration/autoland/rev/008efac7764b Wire up visited values in ServoStyleSet::GetContext. r=emilio https://hg.mozilla.org/integration/autoland/rev/182a88607e08 Check failure patterns in assertSnapshots. r=xidorn https://hg.mozilla.org/integration/autoland/rev/8388d8c76fed Enable visited tests for Stylo. r=emilio

Backed out in https://hg.mozilla.org/integration/autoland/rev/1889d4ce29b7 for the surprisingly localized failure https://treeherder.mozilla.org/logviewer.html#?job_id=101862282&repo=autoland - opt and debug, e10s and not, but Windows 7 only, that one color-on-visited-focus-1.html color-on-visited-1-ref.html test.

For the backout.

I guess I'll leave out the extra tests for now until there's a less fragile way to express them.

Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/332c6207cf86 GetContentStateForVisitedHandling can access state directly. r=dbaron https://hg.mozilla.org/integration/autoland/rev/f469745cfa7a Add visited pseudo crashtests. r=emilio https://hg.mozilla.org/integration/autoland/rev/23067369ca08 Wire up visited values in ServoStyleSet::GetContext. r=emilio https://hg.mozilla.org/integration/autoland/rev/127f613fab52 Check failure patterns in assertSnapshots. r=xidorn https://hg.mozilla.org/integration/autoland/rev/f729a9e2ce8e Enable visited tests for Stylo. r=emilio

https://hg.mozilla.org/mozilla-central/rev/332c6207cf86 https://hg.mozilla.org/mozilla-central/rev/f469745cfa7a https://hg.mozilla.org/mozilla-central/rev/23067369ca08 https://hg.mozilla.org/mozilla-central/rev/127f613fab52 https://hg.mozilla.org/mozilla-central/rev/f729a9e2ce8e