Handle gesture events on scrollbars.
Created attachment 172881 [details] Patch
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 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 .
Created attachment 173915 [details] Patch
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 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...
Created attachment 174006 [details] Patch
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.
> > 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.
Created attachment 174517 [details] Patch
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 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
Created attachment 174694 [details] Patch
(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 on attachment 174694 [details] Patch nit from previous review fixed.
Comment on attachment 174694 [details] Patch Clearing flags on attachment: 174694 Committed r134965: <http://trac.webkit.org/changeset/134965>
All reviewed patches have been landed. Closing bug.