[Webkit-unassigned] [Bug 135774] Implement snapping behavior for Mac overflow scrolling

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135774





--- Comment #4 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-08-15 18:16:02 PST ---
(From update of attachment 236692)
View in context: https://bugs.webkit.org/attachment.cgi?id=236692&action=review

> Source/WebCore/dom/Element.cpp:266
> +    if (!(event.deltaX() || event.deltaY()) && event.phase() != PlatformWheelEventPhaseEnded && event.momentumPhase() != PlatformWheelEventPhaseEnded)

See below (EventHandler.cpp)

> Source/WebCore/page/EventHandler.cpp:300
> +    if (!startNode->renderer())

Changes to Element.cpp and EventHandler.cpp will cause regressions for certain tests in LayoutTests/platform/mac-wk2/tiled-drawing/ that check the exact number of wheel events handled. This is because wheel events indicating momentum/scrolling end are propagated down to the ScrollAnimator, instead of being ignored by an early return.

> Source/WebCore/platform/ScrollAnimator.cpp:89
> +#if ENABLE(CSS_SCROLL_SNAP)

See comments below. We'll need to consolidate this into a single scroll snap animator

> Source/WebCore/platform/ScrollAnimator.h:129
> +    void horizontalScrollSnapTimerFired(Timer<ScrollAnimator>&);

This is pretty gross :( When we refactor to only use one animator though, we'll only require 1 function here.

> Source/WebCore/platform/ScrollAnimator.h:137
> +    std::unique_ptr<AxisScrollSnapAnimator> m_horizontalScrollSnapAnimator;

I'll need to change this to use a single scroll animator for both axes. As Beth and Brent mentioned, this new animator should only be handling scroll snap events in a single axis until scrolling stops.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:-68
> -    AxisScrollSnapAnimator(AxisScrollSnapAnimatorClient*, Vector<LayoutUnit>* snapOffsets, ScrollEventAxis);

snapOffsets shouldn't be a pointer in the first place, since it should never be null anyways. We should pass a const reference to the snap offsets instead.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:85
> +AxisScrollSnapAnimator::AxisScrollSnapAnimator(AxisScrollSnapAnimatorClient* client, const Vector<LayoutUnit>* snapOffsets, ScrollEventAxis axis)

snapOffsets shouldn't be a pointer in the first place, since it should never be null anyways. We should pass a const reference to the snap offsets instead.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:187
> +    m_client->startScrollSnapTimer(m_axis);

When we fix the scroll snap animator to only use one axis, we won't need two timers for horizontal/vertical axes anymore, so startScrollSnapTimer will not need to take arguments indicating which axis to handle.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:197
> +    m_client->stopScrollSnapTimer(m_axis);

When we fix the scroll snap animator to only use one axis, we won't need two timers for horizontal/vertical axes anymore, so startScrollSnapTimer will not need to take arguments indicating which axis to handle.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.