952328 - element not restyled if it matches a .class:visited selector and the class is changed

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Cameron McCormack (:heycam)]

Attachment: test.html

Haven't looked into where the problem is yet, but we don't restyle the element after the class="" attribute is removed.

Comment 1

[Cameron McCormack (:heycam)]

The regression came in somewhere between Firefox 3.6 and 4.

Comment 2

[Cameron McCormack (:heycam)]

When class="" is removed, HasAttributeDependentStyle is called to determine if we need to post a restyle event, but it returns nsRestyleHint(0).  This is because here:

  https://hg.mozilla.org/mozilla-central/file/78f2c5e0b9fb/layout/style/nsCSSRuleProcessor.cpp#l2713

we use false for aIsRelevantLink, which results in the SelectorMatches call just below returning false (since the :visited part of the selector is thought not to match).  The comment for NodeMatchContext::mIsRelevantLink says it must be false when TreeMatchContext::mForStyling is false (which it is).  And the assertion at the top of SelectorMatches is:

    NS_ABORT_IF_FALSE(aTreeMatchContext.mForStyling ||
                    !aNodeMatchContext.mIsRelevantLink,
                    "mIsRelevantLink should be set to false when mForStyling "
                    "is false since we don't know how to set it correctly in "
                    "Has(Attribute|State)DependentStyle");

Is it really true that it's not possible to set it correctly?  Wouldn't it be better to use true rather than false, so that we optimise less (by having HasAttributeDependentStyle return true more than necessary) but get correct behaviour?

Comment 3

[Cameron McCormack (:heycam)]

Attachment: visited-restyle

Is there anything wrong with this?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So a few thoughts here before I actually dig into the code:

 (1) We don't want performance to depend in any way on whether a link is visited.  (We're not perfect here, but I'd rather avoid introducing more problems.)  This means that whether we restyle should not depend on whether the link is visited.  (We also always have both the style-if-visited and style-if-unvisited.)

 (2) This leads me to believe that HasAttributeDependentStyle should return true if *either* the style-if-unvisited or the style-if-visited depends on the attribute.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8350427 [details] [diff] [review]
visited-restyle

>+  if (nodeContext.mIsRelevantLink) {
>+    data->mTreeMatchContext.SetHaveRelevantLink();
>+  }

This is the only bit I've examined so far, because it's the most suspicious.  The have-relevant-link state on the TreeMatchContext is an out parameter -- the only thing that uses it is nsStyleSet::{ResolveStyleFor,ResolvePseudoElementStyle,ProbePseudoElementStyle,ResolveXULTreePseudoStyle}, to determine whether to create a separate style-if-visited style context.  That's not relevant here, so I believe this call does nothing other than confuses what the have-relevant-link state on the tree match context is for, so I think these three lines should be removed.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The next question is why you don't also need to fix HasStateDependentStyle.  (There might be a reason -- the state and attribute optimizations are a bit different.  But it's suspicious.)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So what's supposed to make (2) from comment 4 work in the first place is that the tree match context's VisitedHandling() returns nsRuleWalker::eLinksVisitedOrUnvisited, which is in fact how nsStyleSet::HasAttributeDependentStyle sets up the tree match context.  And that doesn't work its magic in nsCSSRuleProcessor::GetContentStateForVisitedHandling unless we actually do set the is-relevant-link state on the NodeMatchContext correctly.

So other than comment 5, the change to AttributeEnumFunc seems correct.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The assertion text, however, is correct that this patch isn't setting mIsRelevantLink correctly.  That's because HasAttributeDependentStyle and HasStateDependentStyle are called on the initial subsequence of a selector, up to and including the combinator-separated piece with the attribute or state selector in it, but excluding the pieces to the right.  It's possible that those might match the relevant link.

Having mIsRelevantLink sometimes set incorrectly *when* VisitedHandling() is nsRuleWalker::eLinksVisitedOrUnvisited seems like it's probably not harmful, though.  We'll get occasional false positives for restyling, but we'll also avoid some false positives that we'd get if we considered :visited styles all the way up the tree.  That said, it's confusing, and I think the cost of determining if something is a link plus the confusion outweighs the performance benefit in the (extremely rare) case of nested links.

So I'm inclined to think that instead of the fix you propose, it would be better to change nsCSSRuleProcessor::GetContentStateForVisitedHandling so that it honors aVisitedHandling == nsRuleWalker::eLinksVisitedOrUnvisited even when aIsRelevantLink is false.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8350427 [details] [diff] [review]
visited-restyle

I think I'd prefer the change described in comment 8, though feel free to re-request review if you disagree.

Comment 10

[Cameron McCormack (:heycam)]

This was fixed by https://github.com/servo/servo/pull/19520 in Firefox 59.