-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Support auto switching wheel behavior #4612
Support auto switching wheel behavior #4612
Conversation
@trygveaa is attempting to deploy a commit to the tldraw Team on Vercel. A member of the Team first needs to authorize it. |
2 out of 2 authors have signed the CLA. Thanks! |
@@ -4,7 +4,7 @@ import { EASINGS } from './primitives/easings' | |||
/** @internal */ | |||
export const DEFAULT_CAMERA_OPTIONS: TLCameraOptions = { | |||
isLocked: false, | |||
wheelBehavior: 'pan', | |||
wheelBehavior: 'auto', |
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 changed the default to auto
because I think that's the most intuitive behavior, but that's of course up to you if you want to do.
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.
could we leave this as the current behaviour for now?
42f7653
to
2795d02
Compare
don't mind my test commits, we're testing something internally :) sry for the noise! |
No worries. And sorry for not signing the CLA yet, I had to talk to legal in my company (nothing particular about your CLA, just general policy for CLAs), but hopefully we'll get it sorted soon. |
@@ -9165,6 +9165,9 @@ export class Editor extends EventEmitter<TLEventMap> { | |||
/** @internal */ | |||
private _restoreToolId = 'select' | |||
|
|||
/** @internal */ | |||
private _autoWheelBehavior: 'pan' | 'zoom' = 'pan' |
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.
Whether the auto behavior starts off as pan
or zoom
should perhaps also be configurable.
It would also be nice to remember the last state the user was in in localstorage, so the same one can be used initially when loading the page again. Not sure if this is something tldraw should do automatically, or if it should emit an event on changes and allow this state to be set, so it can be implemented outside of tldraw?
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 added a config option for auto behavior and I added an event that is emitted when it's changed.
This implements support for setting wheelBehavior to auto. What this means is that it will try to detect if you're using a mouse or a touchpad, and use wheelBehavior zoom when a mouse is used and pan when a touchpad is used. This is more intuitive behavior than always having it set to wheel or pan in my opinion. As far as I know, it's unfortunately not possible to know if wheel events are generated from a mouse or a touchpad. The heuristic implemented is that if you scroll horizontally it assumes you use a touchpad and switch to pan, and if you pan with the middle mouse button it assumes you're using a mouse and switches to zoom. This seems to be similar to what other similar software does. Fixes tldraw#4579
9de45ae
to
c5caecb
Compare
wheelBehavior: 'zoom' | 'pan' | 'none' | ||
wheelBehavior: 'auto' | 'zoom' | 'pan' | 'none' | ||
/** The behavior used for the wheel when `wheelBehavior` is set to auto. */ | ||
autoWheelBehavior?: 'zoom' | 'pan' |
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 you think it makes sense to have this in camera options?
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 looks like we're using this as a sort of piece of internal state - is that right? In that case, I'd rather we move it to the instance state. I don't love the way we're updating camera options from inside _flushEvent
right now, it's really not meant to hold application state like this
f7f9f55
to
7e1d4a1
Compare
When `wheelBehavior` is set to `'auto'`, this controls whether the behavior used is `'pan'` or `'zoom'`. The value will automatically be changed on certain user interactions. Specifically it will change to `'pan'` when scrolling horizontally and change to `'zoom'` when the canvas is panned by either spacebar dragging or middle mouse dragging.
This allows you to listen to changes to `autoWheelBehavior` and store it in e.g. local storage so you can initialize it to the same value the next time tldraw is laoded.
The reason for this is that pinch to zoom on a touchpad generates vertical scroll events with ctrlKey set to true.
7e1d4a1
to
b590dea
Compare
/huppy check cla |
Sorry for some spam here, testing out the logic to skip some of our checks for external contributors. |
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 have a sort of a design principle for tldraw that whilst we do our best to extract intention from user actions, in places where that's not possible we aim for predictability instead. I'm feeling cautious about introducing this because i think that changing the behaviour of the mouse wheel during use violates that principle.
This works well on a mac, but testing this on windows/chromebook devices shows some pretty weird behaviour where after space-panning, scrolling the trackpad zooms initially then suddenly transforms into a pan when the x-axis passes a certain threshold, for example.
At the very least, I wouldn't like us to change the default behaviour. I'm hesitant to include this in the SDK at all though because I don't think we can guarantee a consistently good experience with it.
@@ -4,7 +4,7 @@ import { EASINGS } from './primitives/easings' | |||
/** @internal */ | |||
export const DEFAULT_CAMERA_OPTIONS: TLCameraOptions = { | |||
isLocked: false, | |||
wheelBehavior: 'pan', | |||
wheelBehavior: 'auto', |
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.
could we leave this as the current behaviour for now?
wheelBehavior: 'zoom' | 'pan' | 'none' | ||
wheelBehavior: 'auto' | 'zoom' | 'pan' | 'none' | ||
/** The behavior used for the wheel when `wheelBehavior` is set to auto. */ | ||
autoWheelBehavior?: 'zoom' | 'pan' |
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 looks like we're using this as a sort of piece of internal state - is that right? In that case, I'd rather we move it to the instance state. I don't love the way we're updating camera options from inside _flushEvent
right now, it's really not meant to hold application state like this
I think after discussing internally a bit we don't think we can get this to a good enough quality across browsers/devices right now that we'd be keen to include it in the SDK. What are you missing to be able to implement this outside of the SDK? |
Yeah, the idea is that when you're using a trackpad there's not really any reason to use space-panning. But I agree that it in some cases behaves a bit weirdly since we have to guess if a user is using a mouse or a trackpad. For us the trade-off of having a better mouse and touchpad experience in most cases (in our opinion) makes it worth it, but I can understand you have stricter requirements in your SDK.
Alright, implementing it on our side works for us. When I opened this PR I thought it wasn't possible to do that, and it still looks like it's not possible in a good/nice way, but looking at it again now I realize that it is possible if we override
It would be nice to be able to do this in a proper way though, without having to override |
we have our "event" event but that runs after this stuff. Maybe we should introduce a "before-event" event instead? |
I appreciate you understanding though and i'm glad you were able to come to a solution here! We did dig into this quite a bit and I'd like to revisit it in the future, but I think it would involve a bunch of carefully figuring out heuristics for individual devices/operating systems for us to be happy with it |
That might make sense. Though events are async, while we would preferably need to have updated camera options before _flushEventForTick runs. Also note that the
Well, I'm not really happy with that way of overwriting dispatch as it is dependent on the internals of your code. It was more an example of what we need to do. So I really hope we can come up with a proper way to hook into your event handling.
Thanks for looking into it. Given the state of touchpad handling in browsers I'm not sure it's possible to do in a good way unfortunately, unless there's something I've missed. |
Actually I see that eventemitter3 is calling the events synchronously, so this should be fine. I created PR #5319 which seem to be working fine for my use case and should be enough. Would love it if you could take a look at that. |
This implements support for setting wheelBehavior to auto. What this means is that it will try to detect if you're using a mouse or a touchpad, and use wheelBehavior zoom when a mouse is used and pan when a touchpad is used. This is more intuitive behavior than always having it set to wheel or pan in my opinion.
As far as I know, it's unfortunately not possible to know if wheel events are generated from a mouse or a touchpad. The heuristic implemented is that if you scroll horizontally it assumes you use a touchpad and switch to pan, and if you pan with the middle mouse button it assumes you're using a mouse and switches to zoom. This seems to be similar to what other similar software does.
Fixes #4579
Change type
bugfix
improvement
feature
api
other
Test plan
Release notes
pan
when using a touchpad andzoom
when using a mouse.