Fix issue in which removing a class does not always invalidate style

Fix issue in which removing a class does not always
invalidate style on the element losing the class.

The situation in which it fails is the following:

.class1 .class2 {}
<div class='class1'>
  <div id='inner' class='class2'>
  </div>
</div>

then code like:
document.querySelector('#inner').className = '' 

Previously, an invalidation run would be scheduled, which
calls RuleFeatureSet::invalidateStyleForClassChange, but
since the class that is being mutated does not exist on the
inner element, the variable thisElementNeedsStyleRecalc was
never set to true. What we should do instead is set a local
style recalc on every element which has a pending
invalidation list, because a pending invalidation list means
that code must have done something to that element with
CSSOM, and hence we should assume style needs to be
recomputed. (In fact we do know that for sure since code
excecuted earlier verifies that the CSSOM mutation needs
some sort of style recalc.)

BUG=344376
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167392

[chrishtr, 2014-02-18 21:55:26]

Fix issue in which removing a class does not always invalidate style on the
element being mutated.

Generalize test to cover that case, plus some
more subtree cases.

[rune, 2014-02-18 22:13:24]

lgtm

https://codereview.chromium.org/170873004/diff/30001/LayoutTests/fast/css/inv...
File LayoutTests/fast/css/invalidation/targeted-class-style-invalidation.html
(right):

https://codereview.chromium.org/170873004/diff/30001/LayoutTests/fast/css/inv...
LayoutTests/fast/css/invalidation/targeted-class-style-invalidation.html:11:
<div id="innerChild">

Nit: "innerChild" id unused. Would make sense to have it if you referred to it
in a comment like you did for "inner2".

[ojan, 2014-02-18 22:15:30]

I can see from the test case that this gets the right number of recalc'ed
elements, but just staring at the code I'm having trouble understanding it.

Would be helpful if you're change descriptions were more detailed and described
what the fix is and why it works.

[chrishtr, 2014-02-18 22:30:38]

@ojan: updated CL description.

[chrishtr, 2014-02-18 22:33:15]

https://codereview.chromium.org/170873004/diff/30001/LayoutTests/fast/css/inv...
File LayoutTests/fast/css/invalidation/targeted-class-style-invalidation.html
(right):

https://codereview.chromium.org/170873004/diff/30001/LayoutTests/fast/css/inv...
LayoutTests/fast/css/invalidation/targeted-class-style-invalidation.html:11:
<div id="innerChild">
On 2014/02/18 22:13:24, rune - CET wrote:
> 
> Nit: "innerChild" id unused. Would make sense to have it if you referred to it
> in a comment like you did for "inner2".

Done.

[esprehn, 2014-02-18 22:52:33]

https://codereview.chromium.org/170873004/diff/110001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/170873004/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:389: foundInvalidationSet = true;
Isn't this bool identical to foundInvalidationSet then?

https://codereview.chromium.org/170873004/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:418: thisElementNeedsStyleRecalc =
thisElementNeedsStyleRecalc || someChildrenNeedStyleRecalc;
This doesn't make sense now, thisElementNeedsStyleRecalc is always true if
foundInvalidationSet is true.

[ojan, 2014-02-18 22:53:38]

lgtm

Maybe add a FIXME for the issue we discussed in person (i.e. if there are no
selectors with the given class name in their rightmost simple selector, then we
only need to do a clone of the RenderStyle instead of a full recalc style).


Also, sorry, I should have been more specific in what I wanted in the change
description. I was thinking something along the lines of the following:

Fix issue in which removing a class does not always 
invalidate style on the element losing the class. 

If an element has a pending invalidation list, then that
means its class was directly modified. In that case, do
a recalc because the code later in invalidateStyleForClassChange
that looks at classes on this element doesn't know
about removed classes.

https://codereview.chromium.org/170873004/diff/30001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/170873004/diff/30001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:387: if (InvalidationList* invalidationList =
m_pendingInvalidationMap.get(element)) {
Aside: How can this be null? Instead of being an "if" should this just be
followed by an ASSERT(invalidationList)?

[chrishtr, 2014-02-18 23:14:30]

https://codereview.chromium.org/170873004/diff/110001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/170873004/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:389: foundInvalidationSet = true;
On 2014/02/18 22:52:33, esprehn wrote:
> Isn't this bool identical to foundInvalidationSet then?

No, because not all elements in the subtree of an element with a pending
invalidation list needs style recalc. The code recalculates style if one of
these is true:

1. The element has a pending invalidation list
2. The element has a class matching one in invalidationClasses
3. One or more of the element's children needed style recalc.

This patch addresses #1, but should not affect any other cases. In particular

https://codereview.chromium.org/170873004/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:418: thisElementNeedsStyleRecalc =
thisElementNeedsStyleRecalc || someChildrenNeedStyleRecalc;
On 2014/02/18 22:52:33, esprehn wrote:
> This doesn't make sense now, thisElementNeedsStyleRecalc is always true if
> foundInvalidationSet is true.

It's the other way around - foundInvalidationSet is always true if
thisElementNeedsStyleRecalc is true.

This code is still needed for case #3 above. In that case
thisElementNeedsStyleRecalc would be false before line 418, since the element
has neither a pending invalidation list nor a matching class. However, we know
we're a descendant of an invalidation list element (via foundInvalidationSet),
so we recalc style if any child did.

[chrishtr, 2014-02-18 23:19:13]

https://codereview.chromium.org/170873004/diff/30001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/170873004/diff/30001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:387: if (InvalidationList* invalidationList =
m_pendingInvalidationMap.get(element)) {
On 2014/02/18 22:53:39, ojan wrote:
> Aside: How can this be null? Instead of being an "if" should this just be
> followed by an ASSERT(invalidationList)?

Done.

[esprehn, 2014-02-18 23:24:15]

On 2014/02/18 23:14:30, Chris Harrelson wrote:
> ...
> Source/core/css/RuleFeature.cpp:418: thisElementNeedsStyleRecalc =
> thisElementNeedsStyleRecalc || someChildrenNeedStyleRecalc;
> On 2014/02/18 22:52:33, esprehn wrote:
> > This doesn't make sense now, thisElementNeedsStyleRecalc is always true if
> > foundInvalidationSet is true.
> 
> It's the other way around - foundInvalidationSet is always true if
> thisElementNeedsStyleRecalc is true.
> 
> This code is still needed for case #3 above. In that case
> thisElementNeedsStyleRecalc would be false before line 418, since the element
> has neither a pending invalidation list nor a matching class. However, we know
> we're a descendant of an invalidation list element (via foundInvalidationSet),
> so we recalc style if any child did.

If foundInvalidationSet is true, then thisElementNeedsStyleRecalc is already
true as well, so the code inside the if statement does nothing.

[chrishtr, 2014-02-18 23:30:39]

On Tue, Feb 18, 2014 at 3:24 PM, <esprehn@chromium.org> wrote:

> On 2014/02/18 23:14:30, Chris Harrelson wrote:
>
>> ...
>>
>> Source/core/css/RuleFeature.cpp:418: thisElementNeedsStyleRecalc =
>> thisElementNeedsStyleRecalc || someChildrenNeedStyleRecalc;
>> On 2014/02/18 22:52:33, esprehn wrote:
>> > This doesn't make sense now, thisElementNeedsStyleRecalc is always true
>> if
>> > foundInvalidationSet is true.
>>
>
>  It's the other way around - foundInvalidationSet is always true if
>> thisElementNeedsStyleRecalc is true.
>>
>
>  This code is still needed for case #3 above. In that case
>> thisElementNeedsStyleRecalc would be false before line 418, since the
>> element
>> has neither a pending invalidation list nor a matching class. However, we
>> know
>> we're a descendant of an invalidation list element (via
>> foundInvalidationSet),
>> so we recalc style if any child did.
>>
>
> If foundInvalidationSet is true, then thisElementNeedsStyleRecalc is
> already
> true as well, so the code inside the if statement does nothing.
>

No it's not, see my reasoning above. foundInvalidationSet is passed
recursively, thisElementNeedsStyleRecalc is not.


>
> https://codereview.chromium.org/170873004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chrishtr, 2014-02-19 00:24:18]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-02-19 00:24:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/170873004/210006

[commit-bot: I haz the power, 2014-02-19 01:52:05]

Change committed as 167392