Bug 101516 – Handle gesture events on scrollbars.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Robert Flack]

Handle gesture events on scrollbars.

[Robert Flack]

Created attachment 172881 [details]
Patch

[Robert Flack]

This makes touching and dragging the scrollbar behave appropriately rather than scrolling the container div. in the opposite direction. Please take a look, thanks.

[Antonio Gomes]

Comment on attachment 172881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172881&action=review

Looks generally good. Some requests for a final round.

> Source/WebCore/ChangeLog:10
> +        Handle gesture events on scrollbars.
> +        https://bugs.webkit.org/show_bug.cgi?id=101516
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/events/touch/gesture/gesture-scrollbar.html
> +
> +        * page/EventHandler.cpp:

Please improve your changelog with some more details about motivation, advantages, etc of your change.

> Source/WebCore/page/EventHandler.cpp:387
> +    m_scrollGestureHandlingScrollbar = 0;

I think it would read better a m_scrollbarHandlingScrollGesture .

[Robert Flack]

Created attachment 173915 [details]
Patch

[Robert Flack]

Thanks for the review! Comments inline.

(In reply to comment #3)
> (From update of attachment 172881 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172881&action=review
> 
> Looks generally good. Some requests for a final round.
> 
> > Source/WebCore/ChangeLog:10
> > +        Handle gesture events on scrollbars.
> > +        https://bugs.webkit.org/show_bug.cgi?id=101516
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Test: fast/events/touch/gesture/gesture-scrollbar.html
> > +
> > +        * page/EventHandler.cpp:
> 
> Please improve your changelog with some more details about motivation, advantages, etc of your change.

Done.

>
> > Source/WebCore/page/EventHandler.cpp:387
> > +    m_scrollGestureHandlingScrollbar = 0;
> 
> I think it would read better a m_scrollbarHandlingScrollGesture .

Done.

[Antonio Gomes]

Comment on attachment 173915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173915&action=review

Did you check how does this added test behavior for ports that disable scrollbars?

> Source/WebCore/platform/Scrollbar.cpp:377
> +    if (evt.type() == PlatformEvent::GestureTapDown) {
> +        setPressedPart(theme()->hitTest(this, evt.position()));
> +        m_pressedPos = (orientation() == HorizontalScrollbar ? convertFromContainingWindow(evt.position()).x() : convertFromContainingWindow(evt.position()).y());
> +        return true;
> +    }
> +    if (m_pressedPart == ThumbPart && (evt.type() == PlatformEvent::GestureTapDownCancel || evt.type() == PlatformEvent::GestureScrollBegin))
> +        return true;
> +    if (m_pressedPart == ThumbPart && evt.type() == PlatformEvent::GestureScrollUpdate) {
> +        moveThumb(m_pressedPos + (m_orientation == HorizontalScrollbar ? evt.deltaX() : evt.deltaY()), false);
> +        return true;
> +    }
> +    if (evt.type() == PlatformEvent::GestureTap) {
> +        if (m_pressedPart == ThumbPart || m_pressedPart == NoPart)
> +            return false;
> +        if (m_scrollableArea)
> +            return m_scrollableArea->scroll(pressedPartScrollDirection(), pressedPartScrollGranularity());
> +    }

By looking at this, would it look nicer if you handle all type's in a switch/case block instead?

> LayoutTests/fast/events/touch/gesture/gesture-scrollbar.html:51
> +function runTest()
> +{
> +    internals.settings.setMockScrollbarsEnabled(true);

Did you check how this added test behavior for ports that disable scrollbars? AH, maybe setMockScrollbarsEnabled take care of it...

[Robert Flack]

Created attachment 174006 [details]
Patch

[Robert Flack]

Thanks again.

(In reply to comment #6)
> (From update of attachment 173915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173915&action=review
> 
> Did you check how does this added test behavior for ports that disable scrollbars?
> 
> > Source/WebCore/platform/Scrollbar.cpp:377
> > +    if (evt.type() == PlatformEvent::GestureTapDown) {
> > +        setPressedPart(theme()->hitTest(this, evt.position()));
> > +        m_pressedPos = (orientation() == HorizontalScrollbar ? convertFromContainingWindow(evt.position()).x() : convertFromContainingWindow(evt.position()).y());
> > +        return true;
> > +    }
> > +    if (m_pressedPart == ThumbPart && (evt.type() == PlatformEvent::GestureTapDownCancel || evt.type() == PlatformEvent::GestureScrollBegin))
> > +        return true;
> > +    if (m_pressedPart == ThumbPart && evt.type() == PlatformEvent::GestureScrollUpdate) {
> > +        moveThumb(m_pressedPos + (m_orientation == HorizontalScrollbar ? evt.deltaX() : evt.deltaY()), false);
> > +        return true;
> > +    }
> > +    if (evt.type() == PlatformEvent::GestureTap) {
> > +        if (m_pressedPart == ThumbPart || m_pressedPart == NoPart)
> > +            return false;
> > +        if (m_scrollableArea)
> > +            return m_scrollableArea->scroll(pressedPartScrollDirection(), pressedPartScrollGranularity());
> > +    }
> 
> By looking at this, would it look nicer if you handle all type's in a switch/case block instead?

Done. Also, I there was some incorrect handling of the gesture events. Originally I had used the event position, but when running LayoutTests sending scroll updates doesn't change the test event position and this wouldn't work well if we wanted to do things like fling. Deltas are however incremental so I have to accumulate these.

> 
> > LayoutTests/fast/events/touch/gesture/gesture-scrollbar.html:51
> > +function runTest()
> > +{
> > +    internals.settings.setMockScrollbarsEnabled(true);
> 
> Did you check how this added test behavior for ports that disable scrollbars? AH, maybe setMockScrollbarsEnabled take care of it...

Are there ports which disable scrollbars or are we talking about overlay scrollbars (in which case cr-android seems to be passing)? It seems there is a mouse thumb dragging test in scrollbars/scrollbar-drag-thumb-with-large-content.html and no platform specific expectations / exclusions. setMockScrollbarsEnabled as far as I can see from the layout test it only changes the style of the root scrollbars, so I've added a style to the scrollbars to ensure consistent sizing in all platforms.

[Antonio Gomes]

> > Did you check how this added test behavior for ports that disable scrollbars? AH, maybe setMockScrollbarsEnabled take care of it...
> 
> Are there ports which disable scrollbars or are we talking about overlay scrollbars (in which case cr-android seems to be passing)? It seems there is a mouse thumb dragging test in scrollbars/scrollbar-drag-thumb-with-large-content.html and no platform specific expectations / exclusions. setMockScrollbarsEnabled as far as I can see from the layout test it only changes the style of the root scrollbars, so I've added a style to the scrollbars to ensure consistent sizing in all platforms.

BlackBerry port does completely disable scrollbars, and delegating drawing it to the client, afaict.

[Robert Flack]

Created attachment 174517 [details]
Patch

[Robert Flack]

In the 4th patch I added cancelling flings. Since EventHandler knows if a scrollbar is handling gestures we can query that on GestureFlingStart to know whether to cancel the fling. Alternately if we know the scrollbar direction we could rail and amplify the fling but I think this will work as a first pass. Please take a look, thanks!

[Antonio Gomes]

Comment on attachment 174517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174517&action=review

> Source/WebCore/page/EventHandler.cpp:2681
> +bool EventHandler::isScrollbarHandlingGestures()

const

[Robert Flack]

Created attachment 174694 [details]
Patch

[Robert Flack]

(In reply to comment #12)
> (From update of attachment 174517 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174517&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2681
> > +bool EventHandler::isScrollbarHandlingGestures()
> 
> const

Done.

[Robert Flack]

Comment on attachment 174694 [details]
Patch

nit from previous review fixed.

[WebKit Review Bot]

Comment on attachment 174694 [details]
Patch

Clearing flags on attachment: 174694

Committed r134965: <http://trac.webkit.org/changeset/134965>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.