Change hittest node to be scrollbar's parent when hittest include a scrollbar

Change hittest node to be scrollbar's parent when hittest include a scrollbar

For this issue, we pass the MouseMove or MouseLeave event to subframe
when hittest result includes a scrollbar and a subframe (not
scrollbar's parent), then we missing the scrollbar because do a another
hit test in subframe. 

In this patch, I change the innerNode of hittestResult to be
scrollbar's parent when hittestResult has a scrollbar at the end of
hittest. Also I remove the same code in
Document::updateHoverActiveState.

BUG=706401
Review-Url: https://codereview.chromium.org/2784313002
Cr-Commit-Position: refs/heads/master@{#461850}
Committed: https://chromium.googlesource.com/chromium/src/+/2275c1f6d1f074d065de4d0d1aef624f9fcefa66

[chaopeng, 2017-03-30 15:20:14]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 15:20: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:48:13]

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

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

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_...)

[chaopeng, 2017-03-30 20:41:20]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

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

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

[chaopeng, 2017-03-30 20:57:02]

Description was changed from

==========
Not For Review

BUG=706401
==========

to

==========
Change hittest node to be scrollbar's parent when hittest include a scrollbar

For this issue, we pass the MouseMove or MouseLeave event to subframe
when hittest result includes a scrollbar and a subframe (not
scrollbar's parent), then we missing the scrollbar because do a another
hit test in subframe. 

In this patch, I change the innerNode of hittestResult to be
scrollbar's parent when hittestResult has a scrollbar at the end of
hittest. Also I remove the same code in
Document::updateHoverActiveState.

BUG=706401
==========

[chaopeng, 2017-03-30 20:57:02]

chaopeng@chromium.org changed reviewers:
+ bokan@chromium.org

[chaopeng, 2017-03-30 20:58:01]

chaopeng@chromium.org changed reviewers:
+ mustaq@chromium.org

[chaopeng, 2017-03-30 20:58:01]

PTAL. Thank you.

[commit-bot: I haz the power, 2017-03-30 22:53:18]

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

[commit-bot: I haz the power, 2017-03-30 22:53:19]

Dry run: This issue passed the CQ dry run.

[mustaq, 2017-03-31 16:57:49]

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/plugins/overlay-scrollbar-mouse-capture.html
(right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:41:
// If the scrollbar captures the input the plugin shouldn't be receiving
This comment is now irrelevant because the bug is already fixed?

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:166:
result.setURLElement(node->enclosingLinkEventParentOrSelf());
Please add a comment for |HitTestResult.m_innerURLElement| to say that for
non-URL, it's the enclosing elem that triggers navigation.

[bokan, 2017-03-31 20:07:01]

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/plugins/overlay-scrollbar-mouse-capture.html
(right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:41:
// If the scrollbar captures the input the plugin shouldn't be receiving
On 2017/03/31 16:57:48, mustaq wrote:
> This comment is now irrelevant because the bug is already fixed?

Right, we should only have crbug references if the bug is still open and TODO
active.

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

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6180: Scrollbar* hitScrollbar) {
You can remove the hitScrollbar parameter now.

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:161: ScrollableArea*
scrollableArea = result.scrollbar()->getScrollableArea();
We should probably clear the innerNode first. That way if we hit a scrollbar
whose scrollable area isn't associated with a LayoutBox we aren't hitting some
random element below too.

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10910: // Ensure hittest
only has scrollbar.
What changed here? Shouldn't the hit test set the scrollbar's ScrollableArea's
associated Element as the innerElement?

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11080:
EXPECT_EQ(document->hoverNode()->nodeName(), "IFRAME");
Give the iframe an id, save document.getElelementById("iframe") and compare the
pointers here.

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11098:
EXPECT_FALSE(document->hoverNode());
The iframe shouldn't be hovered, but shouldn't the main frame's documentElement?

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11111:
mouseMoveOverIFrameAndScrollbar, Vector<WebMouseEvent>());
For safety, I'd change the coordinate here by at least a pixel. It's not out of
the question that a event handler might one day add some logic to not do
anything if consecutive mouse moves have the same coordinate.

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/data/scrollbar-and-iframe-hover.html
(right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/data/scrollbar-and-iframe-hover.html:16:
<div style="position: absolute; top: 1000px;">
A nice trick here is to just add `height: 2000px` to your body's style above.
Then you don't need the dummy div.

[chaopeng, 2017-04-03 15:28:45]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 15:29:19]

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

[chaopeng, 2017-04-03 15:53:48]

The CQ bit was unchecked by chaopeng@chromium.org

[chaopeng, 2017-04-03 17:49:50]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[chaopeng, 2017-04-03 17:50:26]

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11098:
EXPECT_FALSE(document->hoverNode());
On 2017/03/31 20:07:00, bokan wrote:
> The iframe shouldn't be hovered, but shouldn't the main frame's
documentElement?

For root scrollbar: scrollableArea->layoutBox()->node()  is #document not
documentElement and #document is not Element that is why hoverNode is null.

[commit-bot: I haz the power, 2017-04-03 17:50:44]

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-04-03 20:00:50]

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

[commit-bot: I haz the power, 2017-04-03 20:00:50]

Dry run: This issue passed the CQ dry run.

[chaopeng, 2017-04-03 21:06:05]

Updated. PTAL. Thank you.

[bokan, 2017-04-04 15:18:01]

A few nits but otherwise lgtm. Please wait for mustaq@ to stamp it before
committing (I think you still need to address one of this comments).

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11098:
EXPECT_FALSE(document->hoverNode());
On 2017/04/03 17:50:26, chaopeng wrote:
> On 2017/03/31 20:07:00, bokan wrote:
> > The iframe shouldn't be hovered, but shouldn't the main frame's
> documentElement?
> 
> For root scrollbar: scrollableArea->layoutBox()->node()  is #document not
> documentElement and #document is not Element that is why hoverNode is null.

Ok, that behavior is unchanged from before this patch? i.e. today hovering a
root scrollbar sets hoverNode to null? If so, the original check (EXPECT_FALSE)
is fine.

https://codereview.chromium.org/2784313002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/plugins/overlay-scrollbar-mouse-capture.html
(right):

https://codereview.chromium.org/2784313002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:41:
// The scrollbar captures the input and the plugin would not receive
nit: would -> should

https://codereview.chromium.org/2784313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2784313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:163: // below too.
Nit: "Clear innerNode if hitTest if we hit..." -> "Clear innerNode if we
hit...".

Nit: "scrollable area" -> "ScrollableArea"

Nit: "...associated with a LayoutBox we aren't..." -> "...associated with a
LayoutBox so we aren't..."

https://codereview.chromium.org/2784313002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2784313002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11068: EXPECT_TRUE(iframe);
I usually make these kinds of "test correctness" checks ASSERTs. The difference
is that a failed EXPECT doesn't stop running the rest of the test while an
ASSERT stops right there. If there's no iframe, then there's really no point in
running the rest of the test as something's has gone quite wrong and it's not
what the test is checking (e.g. the test file has changed).

[chaopeng, 2017-04-04 18:20:36]

mustaq@ PTAL.

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2784313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11098:
EXPECT_FALSE(document->hoverNode());
On 2017/04/04 15:18:01, bokan wrote:
> On 2017/04/03 17:50:26, chaopeng wrote:
> > On 2017/03/31 20:07:00, bokan wrote:
> > > The iframe shouldn't be hovered, but shouldn't the main frame's
> > documentElement?
> > 
> > For root scrollbar: scrollableArea->layoutBox()->node()  is #document not
> > documentElement and #document is not Element that is why hoverNode is null.
> 
> Ok, that behavior is unchanged from before this patch? i.e. today hovering a
> root scrollbar sets hoverNode to null? If so, the original check
(EXPECT_FALSE)
> is fine.

Yes, we dont change the behavior.

[mustaq, 2017-04-04 18:56:57]

LGTM with a nit on comments:

https://codereview.chromium.org/2784313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/HitTestResult.h (right):

https://codereview.chromium.org/2784313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/HitTestResult.h:208: Member<Element>
m_innerURLElement;  // m_innerURLElement saves the enclosing
Nit: "For non-URL, this is the enclosing that triggers navigation."

Also, for sake of readability, please move all multi-line comments in this file
to above the member, e.g. as in Node.h.

[chaopeng, 2017-04-04 19:41:03]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-04-04 19:41:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/2784313002/#ps120001
(title: "mustaq comments addressed")

[commit-bot: I haz the power, 2017-04-04 19:41:43]

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-04 21:43:58]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491334863617420,
"parent_rev": "05267be8219e262196306d78d7eade8083de7595", "commit_rev":
"2275c1f6d1f074d065de4d0d1aef624f9fcefa66"}

[commit-bot: I haz the power, 2017-04-04 21:44:56]

Description was changed from

==========
Change hittest node to be scrollbar's parent when hittest include a scrollbar

For this issue, we pass the MouseMove or MouseLeave event to subframe
when hittest result includes a scrollbar and a subframe (not
scrollbar's parent), then we missing the scrollbar because do a another
hit test in subframe. 

In this patch, I change the innerNode of hittestResult to be
scrollbar's parent when hittestResult has a scrollbar at the end of
hittest. Also I remove the same code in
Document::updateHoverActiveState.

BUG=706401
==========

to

==========
Change hittest node to be scrollbar's parent when hittest include a scrollbar

For this issue, we pass the MouseMove or MouseLeave event to subframe
when hittest result includes a scrollbar and a subframe (not
scrollbar's parent), then we missing the scrollbar because do a another
hit test in subframe. 

In this patch, I change the innerNode of hittestResult to be
scrollbar's parent when hittestResult has a scrollbar at the end of
hittest. Also I remove the same code in
Document::updateHoverActiveState.

BUG=706401

Review-Url: https://codereview.chromium.org/2784313002
Cr-Commit-Position: refs/heads/master@{#461850}
Committed:
https://chromium.googlesource.com/chromium/src/+/2275c1f6d1f074d065de4d0d1aef...
==========

[commit-bot: I haz the power, 2017-04-04 21:44:58]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/2275c1f6d1f074d065de4d0d1aef...