Document::updateHoverActiveState only cancel hover and active state when hitting native scrollbar

Document::updateHoverActiveState only cancel hover and active state when hitting
native scrollbar

This patch is change Document::updateHoverActiveState argument bool hitScrollbar
to Scrollbar hitScrollbar and check hitScrollbar is native scrollbar. This patch
can make `div:hover::-webkit-scrollbar-thumb` working correctly.

BUG=667206
Committed: https://crrev.com/bb7a6b0e1d0b5ca9aab24b7e794dc1d1e81c25db
Cr-Commit-Position: refs/heads/master@{#434298}

[chaopeng, 2016-11-23 02:50:42]

Description was changed from

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
overlay scrollbar

This patch is change Document::updateHoverActiveState argument hitScrollbar to
hitOverlayScrollbar. Cause hitScrollbar will break style like this
`div:hover::-webkit-scrollbar-thumb`.

BUG=667206
==========

to

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
overlay scrollbar

This patch is change Document::updateHoverActiveState argument hitScrollbar to
hitOverlayScrollbar. Cause hitScrollbar will break style like this
`div:hover::-webkit-scrollbar-thumb`.

BUG=667206
==========

[chaopeng, 2016-11-23 02:50:42]

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

[chaopeng, 2016-11-23 02:51:25]

Description was changed from

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
overlay scrollbar

This patch is change Document::updateHoverActiveState argument hitScrollbar to
hitOverlayScrollbar. Cause hitScrollbar will break style like this
`div:hover::-webkit-scrollbar-thumb`.

BUG=667206
==========

to

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
overlay scrollbar

This patch is change Document::updateHoverActiveState argument hitScrollbar to
hitOverlayScrollbar. Cause hitScrollbar will break style like this
`div:hover::-webkit-scrollbar-thumb`.

BUG=667206
==========

[bokan, 2016-11-23 15:07:50]

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:6025: bool hitOverlayScrollbar)
{
I think the distinction is native vs custom rather than overlay vs non-overlay,
right? I'm guessing what's happening is that a custom scrollbar takes the
document's :hover state but we clear that below if we hit a scrollbar. A native
scrollbar doesn't take :hover states so we should be checking for custom
scrollbars.

In any case, please replace this bool with Scrollbar* hitScrollbar. You can then
check in this method if its non-null and whether it's custom or not.

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10571: [](WebSettings*
settings) { settings->setDeviceSupportsMouse(true); });
This is unneeded, deviceSupportsMouse=true is the default.

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10593:
EXPECT_TRUE(document->hoverNode());
Make sure this is false before the mouse event. Also, make sure the hoverNode is
actually the one you think it is.

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10610:
EXPECT_TRUE(document->hoverNode());
Isn't the bug that the scrollbar doesn't get a hover effect? We should check for
that too.

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/data/custom-scrollbar-hover.html
(right):

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/data/custom-scrollbar-hover.html:22: <div
id="scro">
nit: scro -> scroller

[chaopeng, 2016-11-23 16:58:16]

Description was changed from

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
overlay scrollbar

This patch is change Document::updateHoverActiveState argument hitScrollbar to
hitOverlayScrollbar. Cause hitScrollbar will break style like this
`div:hover::-webkit-scrollbar-thumb`.

BUG=667206
==========

to

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
native scrollbar

This patch is change Document::updateHoverActiveState argument bool hitScrollbar
to Scrollbar hitScrollbar and check hitScrollbar is native scrollbar. This patch
can make `div:hover::-webkit-scrollbar-thumb` working correctly.

BUG=667206
==========

[chaopeng, 2016-11-23 16:58:36]

Updated, PTAL.

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2523903002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10610:
EXPECT_TRUE(document->hoverNode());
On 2016/11/23 15:07:50, bokan wrote:
> Isn't the bug that the scrollbar doesn't get a hover effect? We should check
for
> that too.

No, This bug is the DIV get hover and change the thumb's color.

[bokan, 2016-11-23 17:09:52]

https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:6025: bool hitNativeScrollbar =
hitScrollbar && !hitScrollbar->isCustomScrollbar();
Please add a comment here on why custom scrollbars are special (because they
participate in hover activation with the content).

https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.h:62: #include
"platform/scroll/Scrollbar.h"
Nit: this can just be a forward declaration below

https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10612:
EXPECT_EQ(document->hoverNode(), scrollbarDiv);
Can you also check that the scrollbar thumb has the :hover state. I'm not sure
how to do this but there's a few avenues to try:

A custom scrollbar is represented by a LayoutScrollbar which has a LayoutPart
for the thumb. You should be able to get the ComputedStyle for that. Not sure
how you check for hover state there but code search might be helpful.

LayoutTheme has isHovered helper. Not sure if it works for custom scrollbars
though.

Maybe Scrollbar::hoveredPart is simple enough?

[chaopeng, 2016-11-23 20:58:28]

On 2016/11/23 17:09:52, bokan wrote:
>
https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.cpp:6025: bool hitNativeScrollbar
=
> hitScrollbar && !hitScrollbar->isCustomScrollbar();
> Please add a comment here on why custom scrollbars are special (because they
> participate in hover activation with the content).
> 
>
https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.h:62: #include
> "platform/scroll/Scrollbar.h"
> Nit: this can just be a forward declaration below
> 
>
https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):
> 
>
https://codereview.chromium.org/2523903002/diff/2/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10612:
> EXPECT_EQ(document->hoverNode(), scrollbarDiv);
> Can you also check that the scrollbar thumb has the :hover state. I'm not sure
> how to do this but there's a few avenues to try:
> 
> A custom scrollbar is represented by a LayoutScrollbar which has a LayoutPart
> for the thumb. You should be able to get the ComputedStyle for that. Not sure
> how you check for hover state there but code search might be helpful.
> 
> LayoutTheme has isHovered helper. Not sure if it works for custom scrollbars
> though.
> 
> Maybe Scrollbar::hoveredPart is simple enough?

Updated, PTAL. Thank you.

[bokan, 2016-11-23 21:05:19]

lgtm % comments

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

https://codereview.chromium.org/2523903002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6026: // Only cancel hover state
when hitting native scrollbar cause custom
nit: cause -> because

https://codereview.chromium.org/2523903002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6027: // scrollbar can be
changed style by the ancestor element's hover state
nit: "scrollbar can be..." -> "scrollbar's style depends on the owner element's
hover state."

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

https://codereview.chromium.org/2523903002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10603: // Mouse over DIV
This would be more accurately described as "Mouse over scrollbar"

[chaopeng, 2016-11-23 21:15:18]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2016-11-23 21:15:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2523903002/#ps90001
(title: "comment update")

[commit-bot: I haz the power, 2016-11-23 21:16:15]

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, 2016-11-24 00:43:56]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1479935718786950,
"parent_rev": "2288c67728a0f260be3f4e30e9706f2bd238f8c8", "commit_rev":
"0be728640d9140ec78503f7fbfa11e7174bb71d5"}

[commit-bot: I haz the power, 2016-11-24 00:44:22]

Description was changed from

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
native scrollbar

This patch is change Document::updateHoverActiveState argument bool hitScrollbar
to Scrollbar hitScrollbar and check hitScrollbar is native scrollbar. This patch
can make `div:hover::-webkit-scrollbar-thumb` working correctly.

BUG=667206
==========

to

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
native scrollbar

This patch is change Document::updateHoverActiveState argument bool hitScrollbar
to Scrollbar hitScrollbar and check hitScrollbar is native scrollbar. This patch
can make `div:hover::-webkit-scrollbar-thumb` working correctly.

BUG=667206
==========

[commit-bot: I haz the power, 2016-11-24 00:44:24]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-11-24 00:46:27]

Description was changed from

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
native scrollbar

This patch is change Document::updateHoverActiveState argument bool hitScrollbar
to Scrollbar hitScrollbar and check hitScrollbar is native scrollbar. This patch
can make `div:hover::-webkit-scrollbar-thumb` working correctly.

BUG=667206
==========

to

==========
Document::updateHoverActiveState only cancel hover and active state when hitting
native scrollbar

This patch is change Document::updateHoverActiveState argument bool hitScrollbar
to Scrollbar hitScrollbar and check hitScrollbar is native scrollbar. This patch
can make `div:hover::-webkit-scrollbar-thumb` working correctly.

BUG=667206

Committed: https://crrev.com/bb7a6b0e1d0b5ca9aab24b7e794dc1d1e81c25db
Cr-Commit-Position: refs/heads/master@{#434298}
==========

[commit-bot: I haz the power, 2016-11-24 00:46:28]

Patchset 6 (id:??) landed as
https://crrev.com/bb7a6b0e1d0b5ca9aab24b7e794dc1d1e81c25db
Cr-Commit-Position: refs/heads/master@{#434298}