Do not invoke or clear hover effects on node deletion when cursor is invisible

Do not invoke new hover effects or clear existing hover effects on node deletion when cursor is invisible

If the mouse cursor is invisible and the currently 
hovered node is deleted from the DOM, then there should 
be no new hover effects invoked for any remaining nodes 
in the page (i.e., those that happen to become positioned
underneath the last known mouse position as a result of 
the deletion). Any existing hover effects for the ancestors
of the deleted node should be preserved.

BUG=240722
R=ojan@chromium.org, rbyers@chromium.org

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

[tdanderson, 2013-06-03 21:46:30]

@ojan and @rbyers, can you please have a look?

[ojan, 2013-06-03 22:55:26]

Why don't we clear hover effects when the cursor goes invisible? Seems strange
to me to not have a cursor and keep the old over effect.

[Rick Byers, 2013-06-03 23:00:17]

Nice, I was hoping there would be a simple fix like this to the gmail hover
issue.  Good work!

A couple suggestions for improving the tests.  Otherwise LGTM.

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
File LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html
(right):

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html:58:
shouldBeEqualToString(window.getComputedStyle(document.querySelector('#blue'),
':after').content, hoveredText);
Pass expression strings as the first arg to ensure readable output

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
File LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html (right):

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html:81:
shouldBe(window.getComputedStyle(document.querySelector('#blue'),
':after').content, "");
This is good, but to reduce brittleness it would be nice to have some extra
protection that this is really doing what's intended.  How about first running
the sequence with the cursor visible and verify this check (factored into a
'blue is hovered' function) fails.  You'd of course then need a way to
reintroduce the node (or maybe just use two blue nodes - being careful to treat
them identically).

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html:81:
shouldBe(window.getComputedStyle(document.querySelector('#blue'),
':after').content, "");
Put the expression (or part of the expression) inside a stringso the output
looks nice.  Right now it's "PASS  is ".

[Rick Byers, 2013-06-03 23:04:05]

On 2013/06/03 22:55:26, ojan wrote:
> Why don't we clear hover effects when the cursor goes invisible? Seems strange
> to me to not have a cursor and keep the old over effect.

That was what we tried long ago by sending bogus mousemove events.  It turns out
there are pages that rely on hover effects not changing when the cursor is
hidden.  For example, G+ comment boxes that are only shown while hovering, but
as soon as you start typing into them we hide the cursor which would then hide
the box with the input field, making it unusable :-(.  I think the "when mouse
cursor is hidden, don't change any hover states" model is much better.  See
crbug.com/153784 for more discussion and history.  This CL is consistent with
the previous change for scrolling in that bug - just applies it in another
scenario (removed nodes in addition to scrolling).

[ojan, 2013-06-03 23:55:02]

Ugh. That G+ behavior is awful. Sigh. C++ side of the change looks good.

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
File LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html
(right):

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html:68:
window.setTimeout(testAfterDelete, 0);
I don't think you need a setTimeout here. You're just trying to ensure layout
has happened, right? You can grab document.body.offsetHeight to force a sync
style resolve + layout. setTimeouts almost invariably lead to flaky/slow tests,
so should be avoided whenever possible.

[Rick Byers, 2013-06-04 00:02:02]

On 2013/06/03 23:55:02, ojan wrote:
> Ugh. That G+ behavior is awful. Sigh. C++ side of the change looks good.

A slightly less distasteful scenario that was also a problem is when hover menus
were long enough to require scrolling (eg. http://sky.fm).  Touch scrolling on
CrOS hides the mouse cursor, so using such a menu with touch was impossible in
all the previous, but nicely solved by the one we're using now.  

I'm actually pretty excited that for the first time since we launched touch
screen support, I finally feel like hover is behaving intuitively for me :-)

[tdanderson, 2013-06-04 22:17:41]

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
File LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html
(right):

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html:58:
shouldBeEqualToString(window.getComputedStyle(document.querySelector('#blue'),
':after').content, hoveredText);
On 2013/06/03 23:00:17, Rick Byers wrote:
> Pass expression strings as the first arg to ensure readable output

Done.

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html:68:
window.setTimeout(testAfterDelete, 0);
On 2013/06/03 23:55:02, ojan wrote:
> I don't think you need a setTimeout here. You're just trying to ensure layout
> has happened, right? You can grab document.body.offsetHeight to force a sync
> style resolve + layout. setTimeouts almost invariably lead to flaky/slow
tests,
> so should be avoided whenever possible.

I originally tried this but it didn't work. When I grab
document.body.offsetHeight instead of using setTimeout, the check at line 73
passes on ToT (but should fail). Just to make sure we mean the same thing, this
is what I was doing immediately before the check at line 73:

debug(document.body.offsetHeight);

I also tried changing document.body.style.color and document.body.innerHTML
before querying document.body.offsetHeight, but also with no luck.

Does setTimeout cause flakiness even when that timeout is 0?

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
File LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html (right):

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html:81:
shouldBe(window.getComputedStyle(document.querySelector('#blue'),
':after').content, "");
On 2013/06/03 23:00:17, Rick Byers wrote:
> Put the expression (or part of the expression) inside a stringso the output
> looks nice.  Right now it's "PASS  is ".

Done.

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html:81:
shouldBe(window.getComputedStyle(document.querySelector('#blue'),
':after').content, "");
On 2013/06/03 23:00:17, Rick Byers wrote:
> This is good, but to reduce brittleness it would be nice to have some extra
> protection that this is really doing what's intended.  How about first running
> the sequence with the cursor visible and verify this check (factored into a
> 'blue is hovered' function) fails.  You'd of course then need a way to
> reintroduce the node (or maybe just use two blue nodes - being careful to
treat
> them identically).

Done. As is, this could do with a bit more refactoring. I will clean it up once
I get the setTimeout issue figured out.

[ojan, 2013-06-04 23:29:07]

lgtm

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
File LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html
(right):

https://codereview.chromium.org/16285002/diff/4001/LayoutTests/fast/dom/hover...
LayoutTests/fast/dom/hover-after-dom-delete-child-invisible-cursor.html:68:
window.setTimeout(testAfterDelete, 0);
On 2013/06/04 22:17:41, tdanderson wrote:
> On 2013/06/03 23:55:02, ojan wrote:
> > I don't think you need a setTimeout here. You're just trying to ensure
layout
> > has happened, right? You can grab document.body.offsetHeight to force a sync
> > style resolve + layout. setTimeouts almost invariably lead to flaky/slow
> tests,
> > so should be avoided whenever possible.
> 
> I originally tried this but it didn't work. When I grab
> document.body.offsetHeight instead of using setTimeout, the check at line 73
> passes on ToT (but should fail). Just to make sure we mean the same thing,
this
> is what I was doing immediately before the check at line 73:
> 
> debug(document.body.offsetHeight);
> 
> I also tried changing document.body.style.color and document.body.innerHTML
> before querying document.body.offsetHeight, but also with no luck.

Oh right. I wasn't thinking through this clearly. The layout won't force
updating the hover state.

> Does setTimeout cause flakiness even when that timeout is 0?

Since scheduleHoverStateUpdate does a startOneShot(0), this might be OK. If
anyone changes that in the future to have a longer delay, then I think we'd have
a race condition in this test. I suppose we could fix it then. In either case, I
don't have a suggestion on a better way to test this.

[Rick Byers, 2013-06-05 14:47:13]

lgtm with one more suggestion on the test

https://codereview.chromium.org/16285002/diff/14001/LayoutTests/fast/dom/hove...
File LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html (right):

https://codereview.chromium.org/16285002/diff/14001/LayoutTests/fast/dom/hove...
LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html:83:
document.body.innerHTML += "<div id='red'></div>";
rather than re-add them with html, it would be cleaner to just hang onto the
node in a global variable when calling removeChild, then you can just re-insert
the node (ideally at the same place in the DOM you removed it from - using
either a container div that stays around, or keeping track of the sibling before
the node being removed).

[tdanderson, 2013-06-05 17:43:27]

https://codereview.chromium.org/16285002/diff/14001/LayoutTests/fast/dom/hove...
File LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html (right):

https://codereview.chromium.org/16285002/diff/14001/LayoutTests/fast/dom/hove...
LayoutTests/fast/dom/hover-after-dom-delete-invisible-cursor.html:83:
document.body.innerHTML += "<div id='red'></div>";
On 2013/06/05 14:47:13, Rick Byers wrote:
> rather than re-add them with html, it would be cleaner to just hang onto the
> node in a global variable when calling removeChild, then you can just
re-insert
> the node (ideally at the same place in the DOM you removed it from - using
> either a container div that stays around, or keeping track of the sibling
before
> the node being removed).

Done.

[commit-bot: I haz the power, 2013-06-05 17:43:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/16285002/22001

[commit-bot: I haz the power, 2013-06-05 18:40:33]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-06-05 19:09:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/16285002/22001

[tdanderson, 2013-06-05 19:24:44]

Committed patchset #4 manually as r151861 (presubmit successful).