Bug 209729 – [WPE] Add axis-locking to kinetic scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Chris Lord]

Currently kinetic scrolling on WPE is completely free so it's very hard to scroll exactly vertically or horizontally. GTK appears to constrain to an axis based on some threshold before sending events (at least from a touchpad), so we need to look at doing something similar in WPE's ScrollGestureController for touchscreen events.

[Chris Lord]

Created attachment 395562 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Adrian Perez]

Comment on attachment 395562 [details]
Patch

The logic for the axis locking looks good but I think it would be
better to avoid adding new API if possible. Also the patch needs
a small rebase to be able to be applied to trunk right now.

Thanks a lot for working on this!

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

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:184
> +#endif

Do we really want to allow customization of these values? If there are
some reasonable fixed values which provide a better experience overall,
I would go for those and remove the API bits that allow changing them.
For example I see a fixed “#define SCROLL_CAPTURE_THRESHOLD_MS 150” in
the GTK code (that's in “gtkeventcontrollerscroll.c”).

We can always add the new API later if we find out that people routinely
need to tweak those for different touch{screens,panels}.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1594
> +            0, G_MAXUINT, 200,

If we end up keeping the properties (instead of using constants) it
seems a bit ridiculous to allow “G_MAXUINT” as the maximum value
here. Anything more than a few hundreds of milliseconds will feel
sluggish to the user, so we can cap this at e.g. 1 second (1000ms).

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1611
> +            0, G_MAXUINT, 8,

Same here, being able to specify a count of pixels bigger than a few hundreds
is kind of pointless as well :D

Related issue: how is this value treated for an output device which uses a
scaling factor different than 1x? Will this be scaled as well (meaning that
the value is in logical pixels) or not (the value is physical pixels)? It
should be at least documented if we keep the new public API.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1629
> +            0, G_MAXUINT, 15,

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1647
> +            0, G_MAXUINT, 30,

Ditto.

> Source/WebKit/UIProcess/API/wpe/WPEView.h:69
> +    static View* create(struct wpe_view_backend* backend, const API::PageConfiguration& configuration, WebKitSettings* settings)

Using constant values for the scroll axis locking would prevent needing
to pass the WebKitSettings instance around.

[Chris Lord]

(In reply to Adrian Perez from comment #3)
> Comment on attachment 395562 [details]
> Patch
> 
> The logic for the axis locking looks good but I think it would be
> better to avoid adding new API if possible. Also the patch needs
> a small rebase to be able to be applied to trunk right now.
> 
> Thanks a lot for working on this!
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395562&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:184
> > +#endif
> 
> Do we really want to allow customization of these values? If there are
> some reasonable fixed values which provide a better experience overall,
> I would go for those and remove the API bits that allow changing them.
> For example I see a fixed “#define SCROLL_CAPTURE_THRESHOLD_MS 150” in
> the GTK code (that's in “gtkeventcontrollerscroll.c”).
> 
> We can always add the new API later if we find out that people routinely
> need to tweak those for different touch{screens,panels}.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1594
> > +            0, G_MAXUINT, 200,
> 
> If we end up keeping the properties (instead of using constants) it
> seems a bit ridiculous to allow “G_MAXUINT” as the maximum value
> here. Anything more than a few hundreds of milliseconds will feel
> sluggish to the user, so we can cap this at e.g. 1 second (1000ms).
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1611
> > +            0, G_MAXUINT, 8,
> 
> Same here, being able to specify a count of pixels bigger than a few hundreds
> is kind of pointless as well :D
> 
> Related issue: how is this value treated for an output device which uses a
> scaling factor different than 1x? Will this be scaled as well (meaning that
> the value is in logical pixels) or not (the value is physical pixels)? It
> should be at least documented if we keep the new public API.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1629
> > +            0, G_MAXUINT, 15,
> 
> Ditto.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1647
> > +            0, G_MAXUINT, 30,
> 
> Ditto.
> 
> > Source/WebKit/UIProcess/API/wpe/WPEView.h:69
> > +    static View* create(struct wpe_view_backend* backend, const API::PageConfiguration& configuration, WebKitSettings* settings)
> 
> Using constant values for the scroll axis locking would prevent needing
> to pass the WebKitSettings instance around.

Given that (most of) these values are in pixels, I feel they do need to be configurable. The values I've chosen work well on a 10" 1080p touch-screen, but I suppose as the ppi changes, or the accuracy of the touch-screen, these values probably wouldn't be satisfactory. Indeed, they also don't take pixel scale into account, so this is also a factor (though if that value is accessible, I could certainly modify it).

I can submit a version of the patch with these as constants if you like, but it seems we're going to need configuration down the line at some point anyway?

[Chris Lord]

Created attachment 412655 [details]
Patch

[Adrian Perez]

Comment on attachment 412655 [details]
Patch

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

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:62
> +                || deltaTime >= scrollCaptureThreshold;

Much nicer to have this as a symbolic constant instead of a magic number 👍️

[Chris Lord]

Created attachment 412659 [details]
Patch

[EWS]

Committed r269151: <https://trac.webkit.org/changeset/269151>

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

[Zan Dobersek]

Comment on attachment 412659 [details]
Patch

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

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:73
> +            auto xOffset = std::abs(touchPoint->x - m_start.x);
> +            auto yOffset = std::abs(touchPoint->y - m_start.y);

These values should be explicitly initialized as unsigned integers. Right now they are signed, causing compiler warnings (with GCC 10) about comparing signed and unsigned integers down below.