Use the flat tree for hover handling.

Use the flat tree for hover handling.

Modulo spec debate like[1], this is much simpler and closer to the spec than
what it was implemented.

This doesn't actually fix issue 705984, but it's a required cleanup, and I've
verified that the display: contents node is added to the :hover chain (if
there's an intermediate <div>), so I probably need to investigate how hit testing
and style invalidation is done, etc, to track down that bug.

Still this is worth landing IMO.

BUG=705984
Review-Url: https://codereview.chromium.org/2778153003
Cr-Commit-Position: refs/heads/master@{#461417}
Committed: https://chromium.googlesource.com/chromium/src/+/3f2db89d6d660abe00760144e734ccb9963cce2a

[emilio, 2017-03-28 15:37:40]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 15:38:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 16:41:49]

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

[commit-bot: I haz the power, 2017-03-28 16:41:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emilio, 2017-03-29 11:06:26]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 11:06:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emilio, 2017-03-29 11:07:10]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 11:07:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 12:15:08]

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

[commit-bot: I haz the power, 2017-03-29 12:15:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-29 12:31:46]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 12:32:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 14:00:59]

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

[commit-bot: I haz the power, 2017-03-29 14:00:59]

Dry run: This issue passed the CQ dry run.

[emilio, 2017-03-29 14:07:19]

ecobos@igalia.com changed reviewers:
+ rune@opera.com

[emilio, 2017-03-29 14:07:20]

This is green, rune, could you take a look?

[emilio, 2017-03-29 14:09:56]

On 2017/03/29 14:07:20, emilio wrote:
> This is green, rune, could you take a look?

Note that I kept the "designated element must generate a box" thing, which is
what causes the first issue I pointed out in the issue:

> * Hit testing or hover code seems to skip text nodes for this, so in the test
case the hover node that arrives to the document is the body.

(It's actually just hover code in Document.cpp fwiw):

  while (newHoverNode && !newHoverNode->layoutObject())
     ...

Actually, I guess I should update that loop to use the flat tree traversal too.

[emilio, 2017-03-29 14:23:43]

On 2017/03/29 14:09:56, emilio wrote:
> On 2017/03/29 14:07:20, emilio wrote:
> > This is green, rune, could you take a look?
> 
> Note that I kept the "designated element must generate a box" thing, which is
> what causes the first issue I pointed out in the issue:
> 
> > * Hit testing or hover code seems to skip text nodes for this, so in the
test
> case the hover node that arrives to the document is the body.
> 
> (It's actually just hover code in Document.cpp fwiw):
> 
>   while (newHoverNode && !newHoverNode->layoutObject())
>      ...
> 
> Actually, I guess I should update that loop to use the flat tree traversal
too.

I guess it's mostly equivalent because of the layoutObject check, which would
skip shadow roots IIUC. But let me know if you want to change it anyway. It's
this line, for reference:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...

[rune, 2017-03-29 21:41:43]

Regarding style invalidation, we skip style invalidation in
ContainerNode::setHovered() for nodes not having layoutObject() when passing in
true. That won't work for display:contents elements.

[rune, 2017-03-29 22:01:01]

https://codereview.chromium.org/2778153003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2778153003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6226:
newHoverNode->updateDistribution();
So we need to update the distribution, but we did not use to do that, and we did
not update the layout tree when traversing the layout object ancestors ...

This might be problematic. What happens when the distribution changes as break
the existing ancestor chain for the hovered elements? Can we end up with broken
chains, left-over hover bits and common ancestor checks failing?

[emilio, 2017-03-30 07:33:56]

On 2017/03/29 22:01:01, rune wrote:
>
https://codereview.chromium.org/2778153003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2778153003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:6226:
> newHoverNode->updateDistribution();
> So we need to update the distribution, but we did not use to do that, and we
did
> not update the layout tree when traversing the layout object ancestors ...

Note that we call Document::updateDistribution in that same function above, so
this is only needed AIUI for detached trees, in which case that call wouldn't
reach it. In particular, that fixes editing/shadow/selection-of-shadowroot.html,
which was asserting at the end of the test, where we do:

    while (container.firstChild)
        container.removeChild(container.firstChild);
 
The node being hovered is removed, and a new node is being hovered.

We also update the distribution in Document::hoveredNodeDetached, so I really
didn't think this could ever be a problem. We don't do the same in
Document::activeChainNodeDetached though, but we rely on the distribution being
up-to-date there (otherwise the FlatTreeTraversal assertions would fire).

> This might be problematic. What happens when the distribution changes as break
> the existing ancestor chain for the hovered elements? Can we end up with
broken
> chains, left-over hover bits and common ancestor checks failing?

Per above, I don't think that this can happen. I'm not sure of any distribution
change that could change the parent chain? Anyway, if this could be a problem,
this wouldn't be particularly a problem of this code, and we'd have to audit all
the :active code too.

[emilio, 2017-03-30 09:53:38]

On 2017/03/30 07:33:56, emilio wrote:
> I'm not sure of any distribution change that could change the parent chain?

This is wrong, and I'm trying to construct test-cases that fail :)

[emilio, 2017-03-30 10:29:12]

On 2017/03/30 09:53:38, emilio wrote:
> On 2017/03/30 07:33:56, emilio wrote:
> > I'm not sure of any distribution change that could change the parent chain?
> 
> This is wrong, and I'm trying to construct test-cases that fail :)

I'm failing miserably at it though. This is only relevant to disconnected
subtrees AIUI (because we did update distribution before on the document before
this), and those go through hoveredNodeDetached, which adjusts the hovered node
appropriately (and detaching a node clears the flags).

[emilio, 2017-03-30 10:33:53]

On 2017/03/30 10:29:12, emilio wrote:
> On 2017/03/30 09:53:38, emilio wrote:
> > On 2017/03/30 07:33:56, emilio wrote:
> > > I'm not sure of any distribution change that could change the parent
chain?
> > 
> > This is wrong, and I'm trying to construct test-cases that fail :)
> 
> I'm failing miserably at it though. This is only relevant to disconnected
> subtrees AIUI (because we did update distribution before on the document
before
> this), and those go through hoveredNodeDetached, which adjusts the hovered
node
> appropriately (and detaching a node clears the flags).

Hmm... Actually I guess the old hover node needs to be connected already,
because of that adjustment that hoveredNodeDetached does, and the asserts would
be legit. But that means that the needsDistributionRecalc flags may have been in
an inconsistent state in that test?

[emilio, 2017-03-30 10:36:15]

On 2017/03/30 10:33:53, emilio wrote:
> On 2017/03/30 10:29:12, emilio wrote:
> > On 2017/03/30 09:53:38, emilio wrote:
> > > On 2017/03/30 07:33:56, emilio wrote:
> > > > I'm not sure of any distribution change that could change the parent
> chain?
> > > 
> > > This is wrong, and I'm trying to construct test-cases that fail :)
> > 
> > I'm failing miserably at it though. This is only relevant to disconnected
> > subtrees AIUI (because we did update distribution before on the document
> before
> > this), and those go through hoveredNodeDetached, which adjusts the hovered
> node
> > appropriately (and detaching a node clears the flags).
> 
> Hmm... Actually I guess the old hover node needs to be connected already,
> because of that adjustment that hoveredNodeDetached does, and the asserts
would
> be legit. But that means that the needsDistributionRecalc flags may have been
in
> an inconsistent state in that test?

That looks wrong, since we definitely have a check for disconnected hoverNodes,
I'm looking into when the old hover node may not be connected in
updateHoverActiveState.

[emilio, 2017-03-30 10:43:03]

On 2017/03/30 10:36:15, emilio wrote:
> On 2017/03/30 10:33:53, emilio wrote:
> > On 2017/03/30 10:29:12, emilio wrote:
> > > On 2017/03/30 09:53:38, emilio wrote:
> > > > On 2017/03/30 07:33:56, emilio wrote:
> > > > > I'm not sure of any distribution change that could change the parent
> > chain?
> > > > 
> > > > This is wrong, and I'm trying to construct test-cases that fail :)
> > > 
> > > I'm failing miserably at it though. This is only relevant to disconnected
> > > subtrees AIUI (because we did update distribution before on the document
> > before
> > > this), and those go through hoveredNodeDetached, which adjusts the hovered
> > node
> > > appropriately (and detaching a node clears the flags).
> > 
> > Hmm... Actually I guess the old hover node needs to be connected already,
> > because of that adjustment that hoveredNodeDetached does, and the asserts
> would
> > be legit. But that means that the needsDistributionRecalc flags may have
been
> in
> > an inconsistent state in that test?
> 
> That looks wrong, since we definitely have a check for disconnected
hoverNodes,
> I'm looking into when the old hover node may not be connected in
> updateHoverActiveState.

Funnily enough, the same tests that failed the FlatTreeTraversal asserts fail
the following assert too (and they're the only two tests that fail this so far):

> DCHECK(!oldHoverNode || oldHoverNode->isConnected());

So this is not holding for some reason, even though it should due to
hoveredNodeDetached...

[emilio, 2017-03-30 10:44:41]

(For reference, the other failing test is
fast/forms/select/listbox-drag-in-from-outside.html)

[emilio, 2017-03-30 10:56:15]

On 2017/03/30 10:44:41, emilio wrote:
> (For reference, the other failing test is
> fast/forms/select/listbox-drag-in-from-outside.html)

The following assert also fails (even faster) in both of those tests, even
thought it doesn't in regular browsing:

> DCHECK(!oldHoverNode || oldHoverNode->isHovered())

[emilio, 2017-03-30 11:19:05]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 11:19:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emilio, 2017-03-30 11:23:44]

I've uploaded another patch that only updates the old node's distribution, with
a comment regarding its intention to handle detached nodes.

I've verified the asserts mentioned above are also triggered without my patch,
and for those cases the handling is effectively the same (there's no common
ancestor, so we clear all of the flags in the detached node's chain, and add the
new one).

I think the failing assertions should be investigated, but since I'm not sure
I'd prefer to wait for your feedback, to see if it's worth opening an issue with
people more experienced in this area.

[rune, 2017-03-30 12:25:19]

https://codereview.chromium.org/2778153003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2778153003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6230:
oldHoverNode->updateDistribution();
I just noticed, there a call to Document::updateDistribution() further up in
this method. I don't think updateDistribution() should be called for
disconnected nodes. Instead, we should not try to flat-tree-traverse nodes which
are !isConnected(). The ultimate fix, though, would be to update the old hover
node and clear the hover state of any disconnected ancestors as well.

I thought removing nodes should make use clear the hover in
hoveredNodeDetached() called from Element::detachLayoutTree(). Is the
performingReattach check there what's causing problems?

[commit-bot: I haz the power, 2017-03-30 12:34:30]

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

[commit-bot: I haz the power, 2017-03-30 12:34:31]

Dry run: This issue passed the CQ dry run.

[emilio, 2017-03-30 13:55:24]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 13:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emilio, 2017-03-30 14:01:06]

On 2017/03/30 12:25:19, rune wrote:
>
https://codereview.chromium.org/2778153003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2778153003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:6230:
> oldHoverNode->updateDistribution();
> I just noticed, there a call to Document::updateDistribution() further up in
> this method.

Right, I mentioned that above.

> I don't think updateDistribution() should be called for
> disconnected nodes.

(Just curious, why?)

> Instead, we should not try to flat-tree-traverse nodes which
> are !isConnected(). The ultimate fix, though, would be to update the old hover
> node and clear the hover state of any disconnected ancestors as well.

So I debugged a bit more and I think I have a proper fix for this. It's running
through the bots now, if it doesn't work I'll fall back to this.

> I thought removing nodes should make use clear the hover in
> hoveredNodeDetached() called from Element::detachLayoutTree(). Is the
> performingReattach check there what's causing problems?

I debugged it a bit more, and that's not the case.

What happens in both test cases is that we are dragging/selecting a node around,
and that means that mustBeInActiveChain would be true. That variable is busted
IMO, and what it means is that we'll skip all nodes but the dragged one from the
hover chain, which makes no sense I think.

In that case, when the hovered node is detached, we go and update it to the
parent pointer. But that parent of course isn't hovered, because we skipped it
at the time. I think making all nodes part of the hover chain is the correct
thing. An alternative would be to check !isHovered() in the loop that gets the
new hovered node (which is doable too, I guess).

Last alternative would be to just check disconnected nodes and skip updating the
distribution, but now I understand the problem it feels much like a hack.

[commit-bot: I haz the power, 2017-03-30 14:48:20]

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

[commit-bot: I haz the power, 2017-03-30 14:48:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-30 15:17:01]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 15:17:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 16:55:04]

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

[commit-bot: I haz the power, 2017-03-30 16:55:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-30 19:06:56]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 19:07:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 20:55:42]

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

[commit-bot: I haz the power, 2017-03-30 20:55:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[emilio, 2017-03-30 22:15:49]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 22:16:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emilio, 2017-03-30 22:17:56]

So, I'm going to give up for now, because there are a few test failures in
editing/ and media/ that I don't have so much time to investigate, and I'd like
to land this.

I've filled https://bugs.chromium.org/p/chromium/issues/detail?id=707027 with a
bug I found while updating this. Also I've moved the previous approach at
https://codereview.chromium.org/2787943002/, though I probably don't work on it
for a while at least.

Could you take another look rune?

[commit-bot: I haz the power, 2017-03-31 01:00:59]

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

[commit-bot: I haz the power, 2017-03-31 01:01:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-31 10:17:27]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 10:17:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 12:58:01]

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

[commit-bot: I haz the power, 2017-03-31 12:58:02]

Dry run: This issue passed the CQ dry run.

[rune, 2017-04-03 11:41:22]

lgtm

[emilio, 2017-04-03 11:49:53]

On 2017/04/03 11:41:22, rune wrote:
> lgtm

Thanks!

[emilio, 2017-04-03 11:49:58]

The CQ bit was checked by ecobos@igalia.com

[commit-bot: I haz the power, 2017-04-03 11:50:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 13:30:04]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491220198089370,
"parent_rev": "d60206ee7cd61fecbceceb5e8e8b9a73de4b00c3", "commit_rev":
"3f2db89d6d660abe00760144e734ccb9963cce2a"}

[commit-bot: I haz the power, 2017-04-03 13:30:57]

Description was changed from

==========
Use the flat tree for hover handling.

Modulo spec debate like[1], this is much simpler and closer to the spec than
what it was implemented.

This doesn't actually fix issue 705984, but it's a required cleanup, and I've
verified that the display: contents node is added to the :hover chain (if
there's an intermediate <div>), so I probably need to investigate how hit
testing
and style invalidation is done, etc, to track down that bug.

Still this is worth landing IMO.

BUG=705984
==========

to

==========
Use the flat tree for hover handling.

Modulo spec debate like[1], this is much simpler and closer to the spec than
what it was implemented.

This doesn't actually fix issue 705984, but it's a required cleanup, and I've
verified that the display: contents node is added to the :hover chain (if
there's an intermediate <div>), so I probably need to investigate how hit
testing
and style invalidation is done, etc, to track down that bug.

Still this is worth landing IMO.

BUG=705984

Review-Url: https://codereview.chromium.org/2778153003
Cr-Commit-Position: refs/heads/master@{#461417}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f2db89d6d660abe00760144e734...
==========

[commit-bot: I haz the power, 2017-04-03 13:30:58]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/3f2db89d6d660abe00760144e734...