Bug 222594 – Add basic (non-momentum) wheel event handling for scroll snap

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Martin Robinson]

Basic wheel event handling is the last piece necessary for turning scroll snap on for GTK+ and WPE.

[Martin Robinson]

It seems this is useful for the Mac port as well. I have verified this by plugging in a non-momentum based mouse. Scroll snap doesn't seem to be working in this case.

[Martin Robinson]

Created attachment 421961 [details]
Patch

[Martin Robinson]

Created attachment 422047 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 422047 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimator.cpp:214
> +        ScrollBehavior behavior = ScrollBehavior::DoDirectionalSnapping;
> +        if (e.hasPreciseScrollingDeltas())
> +            behavior = static_cast<ScrollBehavior>(behavior & ScrollBehavior::NeverAnimate);

Don't you want an OptionSet<> here?

[Martin Robinson]

Created attachment 422686 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75213351>

[Martin Robinson]

Created attachment 422701 [details]
Patch

[Martin Robinson]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 422047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422047&action=review
> 
> > Source/WebCore/platform/ScrollAnimator.cpp:214
> > +        ScrollBehavior behavior = ScrollBehavior::DoDirectionalSnapping;
> > +        if (e.hasPreciseScrollingDeltas())
> > +            behavior = static_cast<ScrollBehavior>(behavior & ScrollBehavior::NeverAnimate);
> 
> Don't you want an OptionSet<> here?

Oh nice. I wasn't aware of OptionSet<>. I've modified the patch to use that everywhere applicable.

[Simon Fraser (smfr)]

Comment on attachment 422701 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimator.cpp:181
> +        if (processWheelEventForScrollSnap(e))
> +            return false;

Double indentation

[Martin Robinson]

Created attachment 423064 [details]
Patch

[EWS]

commit-queue failed to commit attachment 423064 [details] to WebKit repository. To retry, please set cq+ flag again.

[Martin Robinson]

Created attachment 423099 [details]
Patch

[EWS]

Committed r274381: <https://commits.webkit.org/r274381>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423099 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 224080

[Martin Robinson]

Created attachment 425148 [details]
Patch

[Martin Robinson]

Created attachment 426538 [details]
Patch

[Darin Adler]

Comment on attachment 426538 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimator.h:72
> +        Default = 0,

When we use OptionSet, we don’t need a named constant for 0.

[Darin Adler]

Comment on attachment 426538 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimator.h:74
> +        DoDirectionalSnapping = 1 << 1,
> +        NeverAnimate = 1 << 2,

This should start with 1 << 0, not skip it and move on to to 1 << 1.

[Darin Adler]

Comment on attachment 426538 [details]
Patch

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

Looks fine.

> Source/WebCore/platform/ScrollAnimator.cpp:182
>  #if PLATFORM(MAC)
> -bool ScrollAnimator::processWheelEventForScrollSnap(const PlatformWheelEvent& wheelEvent)
> -{
> -    return m_scrollController.processWheelEventForScrollSnap(wheelEvent);
> -}
>  #endif

Since we’re deleting this code, please delete the #if/#endif pair too!

[Martin Robinson]

Created attachment 426659 [details]
Patch

[Martin Robinson]

https://bugs.webkit.org/show_bug.cgi?id=224643(In reply to Darin Adler from comment #18)
> Comment on attachment 426538 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426538&action=review
> 
> > Source/WebCore/platform/ScrollAnimator.h:74
> > +        DoDirectionalSnapping = 1 << 1,
> > +        NeverAnimate = 1 << 2,
> 
> This should start with 1 << 0, not skip it and move on to to 1 << 1.

Okay. I've removed Default from the enum and also have started it from 1 << 0.

(In reply to Darin Adler from comment #19)
> > Source/WebCore/platform/ScrollAnimator.cpp:182
> >  #if PLATFORM(MAC)
> > -bool ScrollAnimator::processWheelEventForScrollSnap(const PlatformWheelEvent& wheelEvent)
> > -{
> > -    return m_scrollController.processWheelEventForScrollSnap(wheelEvent);
> > -}
> >  #endif
> 
> Since we’re deleting this code, please delete the #if/#endif pair too!

Oh geez! Thanks for pointing this out. I've fixed it now.

[EWS]

Committed r276353 (236831@main): <https://commits.webkit.org/236831@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426659 [details].