Bug 101516 - Handle gesture events on scrollbars.
Summary: Handle gesture events on scrollbars.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-07 15:25 PST by Robert Flack
Modified: 2012-11-16 10:18 PST (History)
2 users (show)

See Also:


Attachments
Patch (18.75 KB, patch)
2012-11-07 15:27 PST, Robert Flack
no flags Details | Formatted Diff | Diff
Patch (19.03 KB, patch)
2012-11-13 09:55 PST, Robert Flack
no flags Details | Formatted Diff | Diff
Patch (19.95 KB, patch)
2012-11-13 15:18 PST, Robert Flack
no flags Details | Formatted Diff | Diff
Patch (21.53 KB, patch)
2012-11-15 14:31 PST, Robert Flack
no flags Details | Formatted Diff | Diff
Patch (22.49 KB, patch)
2012-11-16 08:33 PST, Robert Flack
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Flack 2012-11-07 15:25:54 PST
Handle gesture events on scrollbars.
Comment 1 Robert Flack 2012-11-07 15:27:23 PST
Created attachment 172881 [details]
Patch
Comment 2 Robert Flack 2012-11-12 10:47:57 PST
This makes touching and dragging the scrollbar behave appropriately rather than scrolling the container div. in the opposite direction. Please take a look, thanks.
Comment 3 Antonio Gomes 2012-11-13 07:12:16 PST
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 .
Comment 4 Robert Flack 2012-11-13 09:55:47 PST
Created attachment 173915 [details]
Patch
Comment 5 Robert Flack 2012-11-13 09:56:23 PST
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.
Comment 6 Antonio Gomes 2012-11-13 10:10:09 PST
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...
Comment 7 Robert Flack 2012-11-13 15:18:12 PST
Created attachment 174006 [details]
Patch
Comment 8 Robert Flack 2012-11-13 15:34:37 PST
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.
Comment 9 Antonio Gomes 2012-11-14 06:40:13 PST
> > 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.
Comment 10 Robert Flack 2012-11-15 14:31:30 PST
Created attachment 174517 [details]
Patch
Comment 11 Robert Flack 2012-11-15 14:33:02 PST
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!
Comment 12 Antonio Gomes 2012-11-16 08:08:53 PST
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
Comment 13 Robert Flack 2012-11-16 08:33:56 PST
Created attachment 174694 [details]
Patch
Comment 14 Robert Flack 2012-11-16 08:34:23 PST
(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.
Comment 15 Robert Flack 2012-11-16 08:44:10 PST
Comment on attachment 174694 [details]
Patch

nit from previous review fixed.
Comment 16 WebKit Review Bot 2012-11-16 10:18:07 PST
Comment on attachment 174694 [details]
Patch

Clearing flags on attachment: 174694

Committed r134965: <http://trac.webkit.org/changeset/134965>
Comment 17 WebKit Review Bot 2012-11-16 10:18:12 PST
All reviewed patches have been landed.  Closing bug.