-
-
Notifications
You must be signed in to change notification settings - Fork 978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[web] Add support for two finger pan #3163
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked if the behavior is the same as on iPad?
example/App.tsx
Outdated
{ | ||
name: 'Two finger Pan', | ||
component: TwoFingerPan, | ||
unsupportedPlatforms: new Set(['android', 'macos', 'ios']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsupportedPlatforms: new Set(['android', 'macos', 'ios']), | |
unsupportedPlatforms: new Set(['android', 'macos']), |
Shouldn't this prop also work on iPads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brought back in dc59483
r.value = clampColor(r); | ||
b.value = clampColor(b); | ||
|
||
console.table({ red: r.value, blue: b.value }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this log needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say it is needed, but it doesn't hurt to leave it either. I left it on purpose to see how values change, but we can remove it as it doesn't tell much more.
if (this.receivedWheelEvent) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed when the timeout is cleared in onWheel
? And why not clean it here, before scheduling a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I wondered if the situation below is possible:
this.receivedWheelEvent = true;
// execute `scheduleWheelEnd` here
clearTimeout(this.endWheelTimeout);
but we believe that it is not possible because of how event loop works.
So yes, it is not necessary and I've removed it in a640c0c
this.currentState = State.UNDETERMINED; | ||
|
||
this.wheelDevice = WheelDevice.UNDETERMINED; | ||
}, 30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, should it? I was to move it to a constant but it seems that I've missed it. Either way, do we want to give users permission to change this value? I think we can stick to one that works ok, but leaving it open for customization also makes sense.
: WheelDevice.MOUSE; | ||
|
||
if (this.wheelDevice === WheelDevice.MOUSE) { | ||
this.scheduleWheelEnd(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do we need to schedule in this case instead of ending outright?
- Do we need to end here at all?
begin
is called after this condition, so we shouldn't even get there with mouse, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't schedule cleanup, wheelDevice won't be set to UNDETERMINED
. I'm not sure if we want to set it here though. However, we can call end
only if handler was active
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also changed in a640c0c
src/web/tools/PointerEventManager.ts
Outdated
): AdaptedEvent { | ||
return event instanceof PointerEvent | ||
? this.mapPointerEvent(event, eventType) | ||
: this.mapWheelEvent(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to redesign the event manager system in the future. We now have PointerEventManager
that needs to check whether the event it receives is a PointerEvent
which seems wrong to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing wrong with adding another event manager, but I'm not exactly sure how would it work. Adding MouseEventManager
doesn't make much sense at this point for me. Also, this manager would have only one method - onWheel
.
On the other hand, KeyboradManager
also has one callback, so maybe it is fine 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I see, Also, it seems like on Also, on Overall, would it be possible to detect that mouse is not being used instead of checking if it is? |
Co-authored-by: Jakub Piasecki <jakub.piasecki@swmansion.com>
As discussed, I think there's no better way to check that (but I'm open to discussion if you do find a better approach).
Values of
Activating on mouse wheel is a side effect of this feature that is not affecting main functionality. |
I did and indeed it does (at least the example does) 😅 |
Description
This PR adds support for two finger panning on touchpad on
web
.Warning
Two finger gestures may be used as system/browser gestures (for example
swipe
to go back). This PR doesn't handle these cases.Implementation
Implementation is based on WheelEvents. This leads to some limitations - whole state flow of
Pan
has to be managed inside one callback (onWheel
).Ending gesture
Starting gesture is easy, in contrast to ending it. To finish gesture we use
timeout
- if nowheel event
was received sincesetTimeout
was called, we can end gesture.Mouse
vsTouchpad
It is hard to determine whether
mouse
ortouchpad
was used to start the gesture.WheelEvent
doesn't have any information about the device, therefore we have to use heuristics to check that.Note
You cannot start gesture with mouse scroll.
To see if events were generated with mouse, we check whether
wheelDeltaY
property (which is now deprecated) of event is multiple of120
. In short, this is standardwheel delta
for mouse. Here you can find useful links that will tell more about why it works:Caution
While this will work most of the times, it is possible that user will somehow generate this specific
wheel delta
with touchpad and gesture will not be recognized correctly.Closes #800
Test plan
Tested on new Two finger Pan example