Bug 141821

Summary:

[Mac] REGRESSION: Scroll snap points broken after r180018

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

Layout and Rendering

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

All

Bug Depends on:

141537

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch v2 (fix iOS build)	simon.fraser: review+

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>