Reset RestyleFlags when Element is removed.

Reset RestyleFlags when Element is removed.

The RestyleFlags are set based on selectors and DOM structure. Wether the
elements are attached or detached is irrelevant. Furthermore, when these
flags are reset, we must be sure all elements which may contribute to these
flags will have their style recalculated in order to set the flags properly.
That is not necessarily the case for attach/detach since siblings, or
descendants of siblings of the detached element will not have their style
recalculated in general.

Instead, reset RestyleFlags when an element is removed from its parent. If
the element is re-inserted into the DOM, siblings and sibling subtrees will
have the style recalculated to update the flags properly.

R=esprehn@chromium.org
BUG=376139
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175199

[esprehn, 2014-05-30 17:46:18]

This doesn't seem right, we should never be running the SelectorChecker in
ResolveStyle mode over an element that's
not attached (or about to be attached).

How does that happen?

https://codereview.chromium.org/304183009/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (left):

https://codereview.chromium.org/304183009/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:1380: data->clearRestyleFlags();
This doesn't make sense, when you're inserted into the DOM we will always go
through ::attach with a NeedsReattachStyleChagne and reset the flags here. How
did can that get missed?

https://codereview.chromium.org/304183009/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:1426: data->clearRestyleFlags();
We only execute this line when you remove a child, and only if the element has
not been detached already. What's the code path that is running the
SelectorChecker in ResolveStyle mode on a detached element? That seems like the
real bug.

[esprehn, 2014-05-30 17:55:38]

Now I understand, the issue is that unlike the childrenAffectedByDirectAdjacent
like bits where we always set them on the parent of the element being matched,
we set the childrenAffectedByHover bit on the actual element being matched: 
element.setChildrenAffectedByHover(); (in SelectorChecker).

That means that if you had a:hover + b we set the bit on <a>. Then if you
reattach <a> it loses the bit and when it's hovered we won't know to recalcStyle
the <b>.

That sucks that the "children affected by" bit also means "siblings affected by"
for :hover, :focus, :drag and :active. Perhaps we should rename them to
"ChildrenOrSiblingAffectedBy"

lgtm

[esprehn, 2014-05-30 17:56:16]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:56:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rune@opera.com/304183009/1

[rune, 2014-05-30 18:09:37]

On 2014/05/30 17:46:18, esprehn wrote:
> This doesn't seem right, we should never be running the SelectorChecker in
> ResolveStyle mode over an element that's
> not attached (or about to be attached).
> 
> How does that happen?

The nature of childrenAffectedByHover is that it's set on a different element
than the one we're currently resolving style for. That might be attached later
in the case of the reversed attach loop in recalcChildStyle.

> https://codereview.chromium.org/304183009/diff/1/Source/core/dom/Element.cpp
> File Source/core/dom/Element.cpp (left):
> 
>
https://codereview.chromium.org/304183009/diff/1/Source/core/dom/Element.cpp#...
> Source/core/dom/Element.cpp:1380: data->clearRestyleFlags();
> This doesn't make sense, when you're inserted into the DOM we will always go
> through ::attach with a NeedsReattachStyleChagne and reset the flags here. How
> did can that get missed?

It will be reset, but it won't be set to the correct values again:

https://code.google.com/p/chromium/issues/detail?id=376139#c7

Attach/detach is not relevant to these flags except for the display:none case
where it's not interesting to have the bits set. Not that it matters if they're
not reset.

In general we don't have the machinery to recreate the restyle flags correctly
for attach/detach, and I don't think it makes sense to have it connected to
attach/detach either. Recreating the correct flags means doing a sibling forest
style recalc (in the presence of adjacent selectors) for the element being
re-attached, which is not something we want.

>
https://codereview.chromium.org/304183009/diff/1/Source/core/dom/Element.cpp#...
> Source/core/dom/Element.cpp:1426: data->clearRestyleFlags();
> We only execute this line when you remove a child, and only if the element has
> not been detached already. What's the code path that is running the
> SelectorChecker in ResolveStyle mode on a detached element? That seems like
the
> real bug.

I'd need to take another look at that one. My intention was to reset the flag
when the node is removed from its parent. If doing that causes a detach clearing
the rare data, this code is unnecessary.

[commit-bot: I haz the power, 2014-05-30 19:07:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6541)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-30 19:30:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-30 19:30:10]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6551)

[rune, 2014-05-31 11:58:16]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-05-31 11:58:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rune@opera.com/304183009/1

[commit-bot: I haz the power, 2014-05-31 20:19:58]

Change committed as 175199

[rune, 2014-06-01 15:35:44]

On 2014/05/30 18:09:37, rune wrote:

> I'd need to take another look at that one. My intention was to reset the flag
> when the node is removed from its parent. If doing that causes a detach
clearing
> the rare data, this code is unnecessary.

Just FTR, I've checked that the call to clearRestyleFlags in
Element::removedFrom is correct/necessary.