Handle gesture scroll events on scrollbars from Touchpad.

Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

LayoutTest/fast/events/wheel-in-scrollbar.html LayoutTest covers testing
this when wheel event gestures are enabled.

BUG=568183
Committed: https://crrev.com/577382823d241be3ba011b9713c817afdfd5a2c3
Cr-Commit-Position: refs/heads/master@{#381988}

[dtapuska, 2016-03-15 19:55:58]

Description was changed from

==========
Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

BUG=568183
==========

to

==========
Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

BUG=568183
==========

[dtapuska, 2016-03-15 19:55:58]

dtapuska@chromium.org changed reviewers:
+ tdresser@chromium.org

[tdresser, 2016-03-16 13:52:30]

LGTM

https://codereview.chromium.org/1805063003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/1805063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:355: return true;
This default case should be ASSERT_NOT_REACHED(), shouldn't it?

[dtapuska, 2016-03-17 13:44:23]

Removed layout test since it is already covered when
https://codereview.chromium.org/1806103002/ lands

https://codereview.chromium.org/1805063003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/1805063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:355: return true;
On 2016/03/16 13:52:30, tdresser wrote:
> This default case should be ASSERT_NOT_REACHED(), shouldn't it?

Done.

[dtapuska, 2016-03-17 13:45:29]

dtapuska@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2016-03-18 13:52:02]

Looks good, just a couple questions.

Since you're not adding any test here, please mention in the CL description
which test covers this behavior.

https://codereview.chromium.org/1805063003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/1805063003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:345:
*shouldUpdateCapture = true;
It seems unfortunate to have the extra complexity of scrollbar capture behaving
differently for touchpad and touchscreen.  Conceptually I think we want the same
UX (automatic capture on scroll/drag start).  I forget why touchscreen used
GestureTapDown to set the capture instead of GestureScrollBegin (I believe
bokan@ did this years ago).  Ideally we could just handle them both on
GestureScrollBegin.

Oh maybe it's just to change the active state of the scrollbar?  We probably
want to show it as active as soon as the finger goes down, and wouldn't want to
risk that changing to something else (eg. if the DOM moved underneath) when the
finger started dragging?

Anyway if you could add a comment somewhere here summarizing why fundamentally
the behavior is different that would be nice.

https://codereview.chromium.org/1805063003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:363: if
(m_scrollableArea && m_scrollableArea->userScroll(evt.deltaUnits(),
delta).didScroll()) {
So wheeling on the scrollbar behaves the same as wheeling elsewhere in the
scroller?  Is that the previous behavior?  Naively I'd expect wheeling on the
scrollbar to scroll faster on long pages (like thumb dragging).  But if that
wasn't the case before your changes, then this is fine.

[dtapuska, 2016-03-18 14:36:52]

Description was changed from

==========
Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

BUG=568183
==========

to

==========
Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

LayoutTest/fast/events/wheel-in-scrollbar.html LayoutTest covers testing
this when wheel event gestures are enabled.

BUG=568183
==========

[dtapuska, 2016-03-18 14:37:57]

https://codereview.chromium.org/1805063003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/1805063003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:345:
*shouldUpdateCapture = true;
On 2016/03/18 13:52:02, Rick Byers wrote:
> It seems unfortunate to have the extra complexity of scrollbar capture
behaving
> differently for touchpad and touchscreen.  Conceptually I think we want the
same
> UX (automatic capture on scroll/drag start).  I forget why touchscreen used
> GestureTapDown to set the capture instead of GestureScrollBegin (I believe
> bokan@ did this years ago).  Ideally we could just handle them both on
> GestureScrollBegin.
> 
> Oh maybe it's just to change the active state of the scrollbar?  We probably
> want to show it as active as soon as the finger goes down, and wouldn't want
to
> risk that changing to something else (eg. if the DOM moved underneath) when
the
> finger started dragging?
> 
> Anyway if you could add a comment somewhere here summarizing why fundamentally
> the behavior is different that would be nice.

Yes touchscreen enters the active state; so you want that on the tap down not on
GSB. Added a comment indicating this.

https://codereview.chromium.org/1805063003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.cpp:363: if
(m_scrollableArea && m_scrollableArea->userScroll(evt.deltaUnits(),
delta).didScroll()) {
On 2016/03/18 13:52:02, Rick Byers wrote:
> So wheeling on the scrollbar behaves the same as wheeling elsewhere in the
> scroller?  Is that the previous behavior?  Naively I'd expect wheeling on the
> scrollbar to scroll faster on long pages (like thumb dragging).  But if that
> wasn't the case before your changes, then this is fine.

Yes they are the same behavior.

[Rick Byers, 2016-03-18 15:17:38]

Thanks, LGTM

[dtapuska, 2016-03-18 15:21:22]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-03-18 15:21:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1805063003/#ps80001
(title: "Add comment")

[commit-bot: I haz the power, 2016-03-18 15:21:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1805063003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1805063003/80001

[commit-bot: I haz the power, 2016-03-18 16:30:18]

Description was changed from

==========
Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

LayoutTest/fast/events/wheel-in-scrollbar.html LayoutTest covers testing
this when wheel event gestures are enabled.

BUG=568183
==========

to

==========
Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

LayoutTest/fast/events/wheel-in-scrollbar.html LayoutTest covers testing
this when wheel event gestures are enabled.

BUG=568183
==========

[commit-bot: I haz the power, 2016-03-18 16:30:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-18 16:31:31]

Description was changed from

==========
Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

LayoutTest/fast/events/wheel-in-scrollbar.html LayoutTest covers testing
this when wheel event gestures are enabled.

BUG=568183
==========

to

==========
Handle gesture scroll events on scrollbars from Touchpad.

With wheel gesture event generation wheel events inside scrollbars
were not working correctly. The scrollbar was only entering the
capturing phase on gesture tap down which doesn't occur with Touchpad.
Transition capturing state on GestureScrollBegin when over a scrollbar.

LayoutTest/fast/events/wheel-in-scrollbar.html LayoutTest covers testing
this when wheel event gestures are enabled.

BUG=568183

Committed: https://crrev.com/577382823d241be3ba011b9713c817afdfd5a2c3
Cr-Commit-Position: refs/heads/master@{#381988}
==========

[commit-bot: I haz the power, 2016-03-18 16:31:32]

Patchset 5 (id:??) landed as
https://crrev.com/577382823d241be3ba011b9713c817afdfd5a2c3
Cr-Commit-Position: refs/heads/master@{#381988}