Don't fade in overlay scrollbar when user interacting the content under scrollbar.

Don't fade in overlay scrollbar when user interacting the content under scrollbar.

The issue (crbug.com/719011) indicate that it is hard to interact with
the content under the the scrollbar because the scrollbar will fade in
while mouse moves in the hover fade in region of scrollbar.

In this patch, we prevent fade in overlay scrollbar when user mouse
down. 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2931703002
Cr-Commit-Position: refs/heads/master@{#478733}
Committed: https://chromium.googlesource.com/chromium/src/+/ac6ccd29b4eaa1e056c0cd0656e2fad8aa624d42

[chaopeng, 2017-06-07 20:18:49]

Description was changed from

==========
Don't fade in overlay scrollbar when user interacting the content under
scrollbar.

In this patch, we have 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
==========

to

==========
Don't fade in overlay scrollbar when user interacting the content under
scrollbar.

In this patch, we have 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chaopeng, 2017-06-07 20:19:51]

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

[chaopeng, 2017-06-07 20:19:51]

PTAL. Thank you.

[chaopeng, 2017-06-07 20:32:10]

Description was changed from

==========
Don't fade in overlay scrollbar when user interacting the content under
scrollbar.

In this patch, we have 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Don't fade in overlay scrollbar when user interacting the content under
scrollbar.

The issue (crbug.com/719011) indicate that user is hard to interact
the content under the the scrollbar because the scrollbar will fade in
while mouse move in hover fade in region of scrollbar.

In this patch, we prevent fade in overlay scrollbar when user mouse
down. 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[bokan, 2017-06-08 14:01:57]

Description was changed from

==========
Don't fade in overlay scrollbar when user interacting the content under
scrollbar.

The issue (crbug.com/719011) indicate that user is hard to interact
the content under the the scrollbar because the scrollbar will fade in
while mouse move in hover fade in region of scrollbar.

In this patch, we prevent fade in overlay scrollbar when user mouse
down. 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Don't fade in overlay scrollbar when user interacting the content under
scrollbar.

The issue (crbug.com/719011) indicate that it is hard to interact with
the content under the the scrollbar because the scrollbar will fade in
while mouse moves in the hover fade in region of scrollbar.

In this patch, we prevent fade in overlay scrollbar when user mouse
down. 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[bokan, 2017-06-08 14:02:39]

btw, your commit message comments and have improved a lot lately, thanks!

https://codereview.chromium.org/2931703002/diff/1/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2931703002/diff/1/cc/input/input_handler.h#ne...
cc/input/input_handler.h:162: bool mouse_down) = 0;
Can we just keep track of the mouse state from the ScrollbarAnimationController?
That is, we already get MouseDown and MouseUp, just keep track of the button
state using those. I realize it's slightly different behavior since if you start
a click in one scroller and drag outside of it other scrollers can fade-in their
scrollbars. But I think that's ok and not worth the added complexity. Those
faded in scrollbars wont prevent you from dragging/selecting under them.

https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.cc:259: if (!MouseIsNearAnyScrollbar()
&& !ScrollbarsHidden()) {
Nit: no braces

https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.cc:291: if (Captured())
Could you add a DCHECK that if we're Captured() the scrollbars must not be
hidden.

https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller_unittest.cc:1290: // press will not
trigger hover fade in, mouse release far will no trigger new
nit: no -> not

https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller_unittest.cc:1297: // Move mouse hover
the fade in scrollbar region of scrollbar.
nit: "Move mouse hover the fade in scrollbar" -> "Move mouse over the hover fade
in"

https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller_unittest.cc:1312: // Move mouse far from
hover the fade in scrollbar region of scrollbar with
nit: ditto here and elsewhere

[chaopeng, 2017-06-08 14:58:40]

Patchset #2 (id:20001) has been deleted

[chaopeng, 2017-06-08 15:04:36]

On 2017/06/08 14:02:39, bokan wrote:
> btw, your commit message comments and have improved a lot lately, thanks!
> 
> https://codereview.chromium.org/2931703002/diff/1/cc/input/input_handler.h
> File cc/input/input_handler.h (right):
> 
>
https://codereview.chromium.org/2931703002/diff/1/cc/input/input_handler.h#ne...
> cc/input/input_handler.h:162: bool mouse_down) = 0;
> Can we just keep track of the mouse state from the
ScrollbarAnimationController?
> That is, we already get MouseDown and MouseUp, just keep track of the button
> state using those. I realize it's slightly different behavior since if you
start
> a click in one scroller and drag outside of it other scrollers can fade-in
their
> scrollbars. But I think that's ok and not worth the added complexity. Those
> faded in scrollbars wont prevent you from dragging/selecting under them.
> 
>
https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
> File cc/input/scrollbar_animation_controller.cc (right):
> 
>
https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
> cc/input/scrollbar_animation_controller.cc:259: if (!MouseIsNearAnyScrollbar()
> && !ScrollbarsHidden()) {
> Nit: no braces
> 
>
https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
> cc/input/scrollbar_animation_controller.cc:291: if (Captured())
> Could you add a DCHECK that if we're Captured() the scrollbars must not be
> hidden.
> 
>
https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
> File cc/input/scrollbar_animation_controller_unittest.cc (right):
> 
>
https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
> cc/input/scrollbar_animation_controller_unittest.cc:1290: // press will not
> trigger hover fade in, mouse release far will no trigger new
> nit: no -> not
> 
>
https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
> cc/input/scrollbar_animation_controller_unittest.cc:1297: // Move mouse hover
> the fade in scrollbar region of scrollbar.
> nit: "Move mouse hover the fade in scrollbar" -> "Move mouse over the hover
fade
> in"
> 
>
https://codereview.chromium.org/2931703002/diff/1/cc/input/scrollbar_animatio...
> cc/input/scrollbar_animation_controller_unittest.cc:1312: // Move mouse far
from
> hover the fade in scrollbar region of scrollbar with
> nit: ditto here and elsewhere

Comments addressed. PTAL. Thank you.

[bokan, 2017-06-08 20:19:54]

Nit on the "fade in region" comments

But otherwise LGTM

https://codereview.chromium.org/2931703002/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2931703002/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:1285: // Move mouse over the
fade in scrollbar region of scrollbar.
Nit: "fade in scrollbar region of scrollbar" should be "fade in region of
scrollbar"

[chaopeng, 2017-06-09 01:19:19]

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

[chaopeng, 2017-06-09 01:19:20]

weiliangc@chromium.org: Please review changes in 

PTAL. Thank you.

[weiliangc, 2017-06-12 17:44:39]

LGTM

[chaopeng, 2017-06-12 18:02:40]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-06-12 18:02:41]

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/2931703002/#ps60001
(title: "bokan comments addressed")

[commit-bot: I haz the power, 2017-06-12 18:03:14]

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-06-12 20:03:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497290560928090,
"parent_rev": "2cf1510ef49539b5a60eb73d984e2089f3bc52bb", "commit_rev":
"ac6ccd29b4eaa1e056c0cd0656e2fad8aa624d42"}

[commit-bot: I haz the power, 2017-06-12 20:04:01]

Description was changed from

==========
Don't fade in overlay scrollbar when user interacting the content under
scrollbar.

The issue (crbug.com/719011) indicate that it is hard to interact with
the content under the the scrollbar because the scrollbar will fade in
while mouse moves in the hover fade in region of scrollbar.

In this patch, we prevent fade in overlay scrollbar when user mouse
down. 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Don't fade in overlay scrollbar when user interacting the content under
scrollbar.

The issue (crbug.com/719011) indicate that it is hard to interact with
the content under the the scrollbar because the scrollbar will fade in
while mouse moves in the hover fade in region of scrollbar.

In this patch, we prevent fade in overlay scrollbar when user mouse
down. 3 behaviors changed:

1. When mouse move in hover fade in region of scrollbar with mouse
   press, we don't trigger delay fade in.
2. When mouse down after a delay fade in triggered, cancel the delay
   fade in.
3. When mouse up in hover fade in region of scrollbar, trigger a delay
   fade in.

BUG=719011
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2931703002
Cr-Commit-Position: refs/heads/master@{#478733}
Committed:
https://chromium.googlesource.com/chromium/src/+/ac6ccd29b4eaa1e056c0cd0656e2...
==========

[commit-bot: I haz the power, 2017-06-12 20:04:02]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ac6ccd29b4eaa1e056c0cd0656e2...