Bug 135774

Summary: Implement snapping behavior for Mac overflow scrolling
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebCore Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Enhancement CC: bdakin, bfulgham, buildbot, cmarcelo, commit-queue, esprehn+autocc, glenn, jonlee, kangil.han, kondapallykalyan, rniwa, simon.fraser, syoichi, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Bug Depends on: 135971    
Bug Blocks: 134283    
Attachments:
Description Flags
WIP
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Updated the patch to build cleanly on current ToT (and iOS). bdakin: review+

Description Wenson Hsieh 2014-08-08 15:39:17 PDT
Use ScrollSnapManagerMac to implement snap scrolling for both overflow and mainframe scrolling on Mac.
Comment 1 Wenson Hsieh 2014-08-14 19:05:52 PDT
(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
Comment 2 Wenson Hsieh 2014-08-15 16:54:26 PDT
Created attachment 236692 [details]
WIP
Comment 3 Jon Lee 2014-08-15 17:27:34 PDT
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.
Comment 4 Wenson Hsieh 2014-08-15 18:16:02 PDT
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.
Comment 5 Build Bot 2014-08-16 07:05:26 PDT
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
Comment 6 Build Bot 2014-08-16 07:05:31 PDT
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
Comment 7 Radar WebKit Bug Importer 2014-08-28 09:33:38 PDT
<rdar://problem/18162409>
Comment 8 Brent Fulgham 2014-09-23 13:52:51 PDT
Created attachment 238569 [details]
Updated the patch to build cleanly on current ToT (and iOS).
Comment 9 Beth Dakin 2014-09-23 14:37:56 PDT
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.
Comment 10 Brent Fulgham 2014-09-23 14:38:59 PDT
(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!
Comment 11 Brent Fulgham 2014-09-23 14:54:21 PDT
(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.
Comment 12 Brent Fulgham 2014-09-23 15:16:40 PDT
Committed r173894: <http://trac.webkit.org/changeset/173894>