Bug 219942 – [WPE] Enable smooth-motion and kinetic scrolling on touchpads

• Component: Scrolling
• Status: RESOLVED FIXED

[Chris Lord]

Currently, smooth motion events (not to be confused with smooth scrolling) are only available via touch-screen scrolling on WPE when using the minibrowser. In addition, kinetic scrolling is only available via the touch-screen at all, regardless of how you interface with WPE (unless you synthesise touch events, though that's not really feasible in a reliable way for this specific use-case). Wayland supports these events fine and both work on the GTK backend (both with X11 and Wayland), we should enable this behaviour on WPE.

[Chris Lord]

Created attachment 416328 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 416328 [details]
Patch

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

Left some inofficial review comments.

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:164
> +            // FIXME: We shouldn't hard-code this.

Do you have something in mind for a proper fix? Needs to be exposed on libwpe?

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:180
> +            if (event->value)

It's not immediately clear why this is the right condition to detect 'PhaseChanged'.... can you enlighten me?

> Source/WebKit/UIProcess/API/wpe/WPEView.h:137
> +    bool m_horizontalScrollActive { false };

Are these mutally exclusive? Looks like from handle_axis_event...
If so, you could merge the two members into one enum.

> Tools/wpe/backends/WindowViewBackend.cpp:241
> +        if (axis != WL_POINTER_AXIS_HORIZONTAL_SCROLL && axis != WL_POINTER_AXIS_VERTICAL_SCROLL)

hmm, what else can 'axis' be? Are we not handling something, or are you protecting against invalid values? Does that happen in the wild?

> Tools/wpe/backends/WindowViewBackend.cpp:297
> +        default:

I'd rather avoid a default here, and handle what exists, to protect against potential changes in future where we need to act, but forget...

[Chris Lord]

(In reply to Nikolas Zimmermann from comment #2)
> Comment on attachment 416328 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416328&action=review
> 
> Left some inofficial review comments.
> 
> > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:164
> > +            // FIXME: We shouldn't hard-code this.
> 
> Do you have something in mind for a proper fix? Needs to be exposed on
> libwpe?

WPE needs to have an enum for axis that just re-defines the Wayland constants, I think. I wanted to avoid changing libwpe though, maybe there are reasons that it doesn't already have this(?) This FIXME already exists elsewhere in WebKit too, I'm not sure if there are libwpe plans for this or not, I should file a bug...

> > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:180
> > +            if (event->value)
> 
> It's not immediately clear why this is the right condition to detect
> 'PhaseChanged'.... can you enlighten me?

Normally you can't receive an axis event with a value of zero, so I've repurposed this to be the equivalent of an axis stop event, to avoid needing to change API/libwpe. An axis event with a value of zero wouldn't mean anything and would have no practical effect prior to this change - this feels reasonably intuitive to me, though a comment would certainly help... Will add one :)

> > Source/WebKit/UIProcess/API/wpe/WPEView.h:137
> > +    bool m_horizontalScrollActive { false };
> 
> Are these mutally exclusive? Looks like from handle_axis_event...
> If so, you could merge the two members into one enum.

They aren't mutually exclusive, no - it could be replaced with a bit-mask or enum I suppose, but that feels like overcomplicating for little benefit.

For example use-cases, with mouse-wheel scrolling, vertical scrolling will be active while horizontal scrolling is inactive, but both can flip to being both true or one true during 2d scrolling. The important thing is to identify when both have stopped, but we can't do that without tracking them independently, because we receive per-axis stop events.

> > Tools/wpe/backends/WindowViewBackend.cpp:241
> > +        if (axis != WL_POINTER_AXIS_HORIZONTAL_SCROLL && axis != WL_POINTER_AXIS_VERTICAL_SCROLL)
> 
> hmm, what else can 'axis' be? Are we not handling something, or are you
> protecting against invalid values? Does that happen in the wild?

Just protecting against invalid values, these are the only two values specified in the Wayland standard right now. I could imagine arbitrary axes in the future though, such a thing already exists (consider joysticks/joypads and things like the Microsoft Surface Studio and Wacom Cintiq) - in those cases, we're only interested in horizontal/vertical.

> > Tools/wpe/backends/WindowViewBackend.cpp:297
> > +        default:
> 
> I'd rather avoid a default here, and handle what exists, to protect against
> potential changes in future where we need to act, but forget...

There's actually no need for a default here at all - it's not an enum in the switch and those are the only two valid values right now. Will remove.

[Chris Lord]

Created attachment 416331 [details]
Patch

[Nikolas Zimmermann]

(In reply to Chris Lord from comment #3) 
> WPE needs to have an enum for axis that just re-defines the Wayland
> constants, I think. I wanted to avoid changing libwpe though, maybe there
> are reasons that it doesn't already have this(?) This FIXME already exists
> elsewhere in WebKit too, I'm not sure if there are libwpe plans for this or
> not, I should file a bug...
Fair enough, if there are previous cases with the same 'FIXME' - okay to leave it as-is. Let's track a libwpe bug to adress this -- it's for sure no show stopper in any way :-)

> 
> > > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:180
> > > +            if (event->value)
> > 
> > It's not immediately clear why this is the right condition to detect
> > 'PhaseChanged'.... can you enlighten me?
> 
> Normally you can't receive an axis event with a value of zero, so I've
> repurposed this to be the equivalent of an axis stop event, to avoid needing
> to change API/libwpe. An axis event with a value of zero wouldn't mean
> anything and would have no practical effect prior to this change - this
> feels reasonably intuitive to me, though a comment would certainly help...
> Will add one :)
Indeed - in this case a commented helped IMHO.

> 
> > > Source/WebKit/UIProcess/API/wpe/WPEView.h:137
> > > +    bool m_horizontalScrollActive { false };
> > 
> > Are these mutally exclusive? Looks like from handle_axis_event...
> > If so, you could merge the two members into one enum.
> 
> They aren't mutually exclusive, no - it could be replaced with a bit-mask or
> enum I suppose, but that feels like overcomplicating for little benefit.
I personally would always go with an OptionSet<> and an enum, as it's easier to extent, and less likely to make a mistake + type-safety is guaranteed (that would be a compelling use case if you'd ever pass this state to a function call) -- but I agree it is not necessary for your specific use case here.

> 
> For example use-cases, with mouse-wheel scrolling, vertical scrolling will
> be active while horizontal scrolling is inactive, but both can flip to being
> both true or one true during 2d scrolling. The important thing is to
> identify when both have stopped, but we can't do that without tracking them
> independently, because we receive per-axis stop events.
Thanks for the explaination.

> 
> > > Tools/wpe/backends/WindowViewBackend.cpp:241
> > > +        if (axis != WL_POINTER_AXIS_HORIZONTAL_SCROLL && axis != WL_POINTER_AXIS_VERTICAL_SCROLL)
> > 
> > hmm, what else can 'axis' be? Are we not handling something, or are you
> > protecting against invalid values? Does that happen in the wild?
> 
> Just protecting against invalid values...
Ok.

> 
> > > Tools/wpe/backends/WindowViewBackend.cpp:297
> > > +        default:
> > 
> > I'd rather avoid a default here, and handle what exists, to protect against
> > potential changes in future where we need to act, but forget...
> 
> There's actually no need for a default here at all - it's not an enum in the
> switch and those are the only two valid values right now. Will remove.
Ah I didn't realize it's not an enum :-)

[Chris Lord]

Created attachment 417260 [details]
Patch

[EWS]

Committed r271292: <https://trac.webkit.org/changeset/271292>

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

[Radar WebKit Bug Importer]

<rdar://problem/72927760>