Bug 135774 – Implement snapping behavior for Mac overflow scrolling

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

Use ScrollSnapManagerMac to implement snap scrolling for both overflow and mainframe scrolling on Mac.

[Wenson Hsieh]

(In reply to comment #0)
> Use ScrollSnapManagerMac to implement snap scrolling for both overflow and mainframe scrolling on Mac.

Note: ScrollSnapManagerMac will no longer be used, since snap point storage and snap animation should be kept separate. This bug has also been limited to cover overflow scrolling, while it's mainframe counterpart can be found here: https://bugs.webkit.org/show_bug.cgi?id=135967

[Wenson Hsieh]

Created attachment 236692 [details]
WIP

[Jon Lee]

Comment on attachment 236692 [details]
WIP

This patch is incomplete. There are known regressions that would be introduced if this patch was checked in. Wenson will explain what needs to be altered in this patch.

[Wenson Hsieh]

Comment on attachment 236692 [details]
WIP

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.

[Build Bot]

Comment on attachment 236692 [details]
WIP

Attachment 236692 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4872161885945856

New failing tests:
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-select-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-iframe-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-mainframe-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-div-with-handler.html

[Build Bot]

Created attachment 236714 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Radar WebKit Bug Importer]

<rdar://problem/18162409>

[Brent Fulgham]

Created attachment 238569 [details]
Updated the patch to build cleanly on current ToT (and iOS).

[Beth Dakin]

Comment on attachment 238569 [details]
Updated the patch to build cleanly on current ToT (and iOS).

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

> Source/WebCore/platform/ScrollAnimator.cpp:41
> +#if ENABLE(CSS_SCROLL_SNAP) && PLATFORM(MAC)

This include should not be necessary since Timer.h is included from the header.

> Source/WebCore/platform/ScrollAnimator.h:42
> +#include "Timer.h"

Is it necessary to include Timer.h in this header? I know I previously said to remove the include from the implementation file, and that should be done if this is necessary. But if this is not necessary, it should be removed, and the implementation file's include should be maintained.

[Brent Fulgham]

(In reply to comment #9)
> (From update of attachment 238569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238569&action=review
> 
> > Source/WebCore/platform/ScrollAnimator.cpp:41
> > +#if ENABLE(CSS_SCROLL_SNAP) && PLATFORM(MAC)
> 
> This include should not be necessary since Timer.h is included from the header.
> 
> > Source/WebCore/platform/ScrollAnimator.h:42
> > +#include "Timer.h"
> 
> Is it necessary to include Timer.h in this header? I know I previously said to remove the include from the implementation file, and that should be done if this is necessary. But if this is not necessary, it should be removed, and the implementation file's include should be maintained.

Good point. I'll see if a fwd declaration in the header is enough, and try to limit it to the implementation file. Thanks!

[Brent Fulgham]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 238569 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=238569&action=review
> > 
> > > Source/WebCore/platform/ScrollAnimator.cpp:41
> > > +#if ENABLE(CSS_SCROLL_SNAP) && PLATFORM(MAC)
> > 
> > This include should not be necessary since Timer.h is included from the header.
> > 
> > > Source/WebCore/platform/ScrollAnimator.h:42
> > > +#include "Timer.h"
> > 
> > Is it necessary to include Timer.h in this header? I know I previously said to remove the include from the implementation file, and that should be done if this is necessary. But if this is not necessary, it should be removed, and the implementation file's include should be maintained.
> 
> Good point. I'll see if a fwd declaration in the header is enough, and try to limit it to the implementation file. Thanks!

Ah -- The Timer<> is a template, so we need the template definition when we parse the file. So I think I'm stuck including it in the header file.

[Brent Fulgham]

Committed r173894: <http://trac.webkit.org/changeset/173894>