Bug 222594 - Add basic (non-momentum) wheel event handling for scroll snap
Summary: Add basic (non-momentum) wheel event handling for scroll snap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: InRadar
Depends on: 224080 224176
Blocks: 221461
  Show dependency treegraph
 
Reported: 2021-03-02 05:03 PST by Martin Robinson
Modified: 2021-04-21 05:16 PDT (History)
4 users (show)

See Also:


Attachments
Patch (14.95 KB, patch)
2021-03-02 10:24 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (16.34 KB, patch)
2021-03-02 22:16 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (18.12 KB, patch)
2021-03-09 04:35 PST, Martin Robinson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (18.99 KB, patch)
2021-03-09 08:24 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (18.97 KB, patch)
2021-03-12 10:49 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (18.93 KB, patch)
2021-03-13 00:53 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (19.03 KB, patch)
2021-04-05 07:05 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (18.98 KB, patch)
2021-04-20 05:09 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (19.03 KB, patch)
2021-04-21 01:03 PDT, Martin Robinson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2021-03-02 05:03:46 PST
Basic wheel event handling is the last piece necessary for turning scroll snap on for GTK+ and WPE.
Comment 1 Martin Robinson 2021-03-02 09:55:33 PST
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.
Comment 2 Martin Robinson 2021-03-02 10:24:17 PST
Created attachment 421961 [details]
Patch
Comment 3 Martin Robinson 2021-03-02 22:16:33 PST
Created attachment 422047 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-03-08 08:56:53 PST
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?
Comment 5 Martin Robinson 2021-03-09 04:35:10 PST
Created attachment 422686 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2021-03-09 05:04:16 PST
<rdar://problem/75213351>
Comment 7 Martin Robinson 2021-03-09 08:24:10 PST
Created attachment 422701 [details]
Patch
Comment 8 Martin Robinson 2021-03-09 11:28:28 PST
(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.
Comment 9 Simon Fraser (smfr) 2021-03-12 10:07:16 PST
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
Comment 10 Martin Robinson 2021-03-12 10:49:11 PST
Created attachment 423064 [details]
Patch
Comment 11 EWS 2021-03-12 15:20:34 PST
commit-queue failed to commit attachment 423064 [details] to WebKit repository. To retry, please set cq+ flag again.
Comment 12 Martin Robinson 2021-03-13 00:53:58 PST
Created attachment 423099 [details]
Patch
Comment 13 EWS 2021-03-13 01:31:23 PST
Committed r274381: <https://commits.webkit.org/r274381>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423099 [details].
Comment 14 WebKit Commit Bot 2021-04-01 14:58:48 PDT
Re-opened since this is blocked by bug 224080
Comment 15 Martin Robinson 2021-04-05 07:05:06 PDT
Created attachment 425148 [details]
Patch
Comment 16 Martin Robinson 2021-04-20 05:09:20 PDT
Created attachment 426538 [details]
Patch
Comment 17 Darin Adler 2021-04-20 09:32:17 PDT
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.
Comment 18 Darin Adler 2021-04-20 09:32:43 PDT
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.
Comment 19 Darin Adler 2021-04-20 09:35:50 PDT
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!
Comment 20 Martin Robinson 2021-04-21 01:03:35 PDT
Created attachment 426659 [details]
Patch
Comment 21 Martin Robinson 2021-04-21 01:07:15 PDT
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.
Comment 22 EWS 2021-04-21 02:04:33 PDT
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].