Bug 141821 - [Mac] REGRESSION: Scroll snap points broken after r180018
Summary: [Mac] REGRESSION: Scroll snap points broken after r180018
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 141537
Blocks:
  Show dependency tree / graph
 
Reported: 2015-02-19 18:13 PST by Brent Fulgham
Modified: 2015-02-20 11:15 PST (History)
2 users (show)

See Also:

Attachments
Patch (5.97 KB, patch)
2015-02-20 08:33 PST, Brent Fulgham 		no flags Details | Formatted Diff | Diff
Patch v2 (fix iOS build) (5.94 KB, patch)
2015-02-20 10:14 PST, Brent Fulgham 		simon.fraser: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-02-19 18:13:36 PST 
The existing Scroll Snap code on Mac relied on the DOM event bubbling phase to make sure the Scroll Snap animations to be triggered. Now that we do not pass these "no motion" events through the DOM event handler chain, the snap animations are not being triggered and the whole feature stops working.
Comment 1 Radar WebKit Bug Importer 2015-02-19 18:14:06 PST 
<rdar://problem/19898333>
Comment 2 Brent Fulgham 2015-02-20 08:33:14 PST 
Created attachment 246972 [details]
Patch
Comment 3 Brent Fulgham 2015-02-20 10:14:01 PST 
Created attachment 246974 [details]
Patch v2 (fix iOS build)
Comment 4 Simon Fraser (smfr) 2015-02-20 10:31:30 PST 
Comment on attachment 246974 [details]
Patch v2 (fix iOS build)

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

> Source/WebCore/page/mac/EventHandlerMac.mm:991
> +    if (scrollableArea->existingScrollAnimator())
> +        scrollableArea->scrollAnimator()->processWheelEventForScrollSnap(wheelEvent);

Maybe if (ScrollAnimator* animator = scrollableArea->existingScrollAnimator()) scrollAnimator->processWheelEventForScrollSnap().
Comment 5 Brent Fulgham 2015-02-20 10:51:15 PST 
Comment on attachment 246974 [details]
Patch v2 (fix iOS build)

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

>> Source/WebCore/page/mac/EventHandlerMac.mm:991
>> +        scrollableArea->scrollAnimator()->processWheelEventForScrollSnap(wheelEvent);
> 
> Maybe if (ScrollAnimator* animator = scrollableArea->existingScrollAnimator()) scrollAnimator->processWheelEventForScrollSnap().

Sure! I'll do that.
Comment 6 zalan 2015-02-20 11:01:14 PST 
Comment on attachment 246974 [details]
Patch v2 (fix iOS build)

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

>>> Source/WebCore/page/mac/EventHandlerMac.mm:991
>>> +        scrollableArea->scrollAnimator()->processWheelEventForScrollSnap(wheelEvent);
>> 
>> Maybe if (ScrollAnimator* animator = scrollableArea->existingScrollAnimator()) scrollAnimator->processWheelEventForScrollSnap().
> 
> Sure! I'll do that.

Couldn't we use reference here instead? -EventHandler::handleWheelEvent() already has checks against scrollableArea != nullptr in some places.
Comment 7 Brent Fulgham 2015-02-20 11:07:09 PST 
Comment on attachment 246974 [details]
Patch v2 (fix iOS build)

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

>>>> Source/WebCore/page/mac/EventHandlerMac.mm:991
>>>> +        scrollableArea->scrollAnimator()->processWheelEventForScrollSnap(wheelEvent);
>>> 
>>> Maybe if (ScrollAnimator* animator = scrollableArea->existingScrollAnimator()) scrollAnimator->processWheelEventForScrollSnap().
>> 
>> Sure! I'll do that.
> 
> Couldn't we use reference here instead? -EventHandler::handleWheelEvent() already has checks against scrollableArea != nullptr in some places.

Actually, that's a good point. We should be checking scrollableArea for nullptr (or in the caller) because there are code paths where it may be null. I'll correct that before landing.
Comment 8 Brent Fulgham 2015-02-20 11:15:23 PST 
Committed r180426: <http://trac.webkit.org/changeset/180426>