523294 - [FIX]Make HasAttributeDependentStyle handle irrelevant values better

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This came up in bug 522930.  The issue is that if we change a class value from "x" to "y" we'll end up with HasAttributeDependentStyle returning true if there's a ".z" selector around.

The current setup for handling attribute changes is to record the intrinsic state before the attr change, then record it after.  Then we do two things:

1)  A ContentStatesChanged notification on the changed states.
2)  An AttributeChanged notification on the changed attribute and the changed
    state.

The former finds all the selectors impacted by the state change, except maybe the ones for which matching depended on the attribute we just changed.  The latter finds all selectors impacted by the attribute and returns true if any match while ignoring that attribute and the given set of states. This setup is what we checked in in bug 315920.

I'm proposing that we nix at least the current HasAttributeDependentStyle thing, and replace it with a setup where we check in AttributeWillChange whether any rules related to that attribute match the node and the same thing again in AttributeChanged.  The former will find rules we will possinle stop matching; the latter will find rules that we're going to start matching.

We can handle the state change the same way, come to think of it; just have HasAttributeDepedentStyle walk the same things that HasStateDependentStyle does or something along those lines.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds good to me.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Thinking about it some more, walking the state-dependent selectors every time is a bit of an annoyance, especially since most states have nothing to do with most attributes.

Just doing the HasStateDependentStyle() as we do now and doing the two things with attributes does give correct behavior, for what it's worth.  These are the possible cases:

1)  Selector includes none of the relevant states and doesn't include the
    attribute.  Nothing changes in whether we match it.
2)  Selector includes relevant states but not the relevant attribute.
    HasStateDependentStyle catches this.
3)  Selector includes relevant attribute.  Then we restyle if we match it
    either before or after; if neither, there's no need to restyle.  Doesn't
    matter what the states are doing.

So this would work; I'm just hoping to find a way to eliminate those two IntrinsicState() calls.  And maybe not have to create RuleProcessorData twice, since we're guaranteed that no one but us messed with the DOM (except in the UnsetAttr case at the moment, which I think is just a bug).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Change the signature of HasAttributeDependentStyle

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2: the guts of the new attribute-change-tracking setup

This applies on top of the patches for bug 523288.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. Clean up by getting rid of the now-unused argument.

Comment 6

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

Comment on attachment 407936 [details] [diff] [review]
Part 1.  Change the signature of HasAttributeDependentStyle

r=dbaron

We should also do this to HasStateDependentStyle.

Also, maybe we should consider dropping the "Has" from both method names?

(No need to mess with your patch queue internals for either of these; they can be followups, whenever...)

Comment 7

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

Comment on attachment 407937 [details] [diff] [review]
Part 2: the guts of the new attribute-change-tracking setup

I think this patch needs to change the comments in nsIStyleRuleProcessor.h to match the changes it's making.  I'd sort of like to see what those comments change to in order to review the code...

Comment 8

[Boris Zbarsky [:bzbarsky]]

> We should also do this to HasStateDependentStyle.

Yep.

> Also, maybe we should consider dropping the "Has" from both method names?

And maybe rename that altogether, yeah.  I'll think about it and file some followups.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with better comments

> I think this patch needs to change the comments in nsIStyleRuleProcessor.h

Ah, yes.  How does that look?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 413494 [details] [diff] [review]
Part 2 with better comments

I made one more local change to this comment: s/bitwise or/bitwise OR/

Comment 11

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

Comment on attachment 413494 [details] [diff] [review]
Part 2 with better comments

-  const PRBool setNodeFlags = aForStyling && aStateMask == 0 && !aAttribute;
+  const PRBool setNodeFlags = aForStyling;

Could you assert that aStateMask != 0 implies !aForStyling at the
start of SelectorMatches?

>+      // If the selector does match due to the dependence on aStateMask, then we
>+      // want to keep result true so that the final result of SelectorMatches is
>+      // true.  Doing so tells StateEnumFunc that there is a dependence on the
>+      // state.

Could you rewrap the comment back down to 72?

+  // Since we get both before and after notifications for attributes, we don't
+  // have to ignore aData->mAttribute while matching.  Just check whether we
+  // have selectors relevant to aData->mAttribute that we match.  If this is
+  // the before change notification, that will catch rules we might stop
+  // matching; if the after change one the ones we might have started matching.

I think the final "one" should be a comma.  Or something like that.

The check for the style attribute in nsFrameManager::HasAttributeDependentStyle
should probably only be on the after notification, just like the hardcoded
checks in nsCSSRuleProcessor::HasAttributeDependentStyle and the code in
nsHTMLStyleSheet::HasAttributeDependentStyle.

r=dbaron with that

Comment 12

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

Comment on attachment 407938 [details] [diff] [review]
Part 3.  Clean up by getting rid of the now-unused argument.

Maybe my review is good enough here?  Check MXR after you land, though, in case of merge "conflicts".

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 407938 [details] [diff] [review]
Part 3.  Clean up by getting rid of the now-unused argument.

> Maybe my review is good enough here?

Sicking says yes.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Could you assert that aStateMask != 0 implies !aForStyling 

Done.

> Could you rewrap the comment back down to 72?

Done.  Do you generally prefer comments wrapped to 72?

> I think the final "one" should be a comma. 

It needs a comma after "one", definitely.  Added.  And replaced "one" with "notification".

> The check for the style attribute in
> nsFrameManager::HasAttributeDependentStyle
> should probably only be on the after notification

Good catch.  Fixed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> I'll think about it and file some followups.

Filed bug 531933.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/eb86d6432f02
http://hg.mozilla.org/mozilla-central/rev/2a90f6882822
http://hg.mozilla.org/mozilla-central/rev/cc8aab2f36b3