Bug 135774 - Implement snapping behavior for Mac overflow scrolling
Summary: Implement snapping behavior for Mac overflow scrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Enhancement
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 135971
Blocks: 134283
  Show dependency treegraph
 
Reported: 2014-08-08 15:39 PDT by Wenson Hsieh
Modified: 2014-09-23 15:16 PDT (History)
14 users (show)

See Also:


Attachments
WIP (13.90 KB, patch)
2014-08-15 16:54 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (534.34 KB, application/zip)
2014-08-16 07:05 PDT, Build Bot
no flags Details
Updated the patch to build cleanly on current ToT (and iOS). (11.79 KB, patch)
2014-09-23 13:52 PDT, Brent Fulgham
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>