Skip to content
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

Conversation

trygve-aaberge-adsk
Copy link
Contributor

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

  1. Use a touchpad to pan the board by scrolling
  2. Switch to a mouse and pan the board by dragging with the middle mouse button
  3. Scroll and observe that it now zooms on scroll
  4. Switch back to the touchpad, scroll horizontally and observe that it pans again
  • Unit tests
  • End to end tests

Release notes

  • Support automatically switching wheelBehavior when switching between touchpad and mouse (as determined by a heuristic). Use pan when using a touchpad and zoom when using a mouse.

Sorry, something went wrong.

Copy link

vercel bot commented Sep 26, 2024

@trygveaa is attempting to deploy a commit to the tldraw Team on Vercel.

A member of the Team first needs to authorize it.

@huppy-bot huppy-bot bot added the improvement Product improvement label Sep 26, 2024
@huppy-bot
Copy link
Contributor

huppy-bot bot commented Sep 26, 2024

2 out of 2 authors have signed the CLA.

Thanks!

Sorry, something went wrong.

@@ -4,7 +4,7 @@ import { EASINGS } from './primitives/easings'
/** @internal */
export const DEFAULT_CAMERA_OPTIONS: TLCameraOptions = {
isLocked: false,
wheelBehavior: 'pan',
wheelBehavior: 'auto',
Copy link
Contributor Author

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.

Copy link
Contributor

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?

@mimecuvalo
Copy link
Member

don't mind my test commits, we're testing something internally :) sry for the noise!

@trygve-aaberge-adsk
Copy link
Contributor Author

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'
Copy link
Contributor Author

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?

Copy link
Contributor Author

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
wheelBehavior: 'zoom' | 'pan' | 'none'
wheelBehavior: 'auto' | 'zoom' | 'pan' | 'none'
/** The behavior used for the wheel when `wheelBehavior` is set to auto. */
autoWheelBehavior?: 'zoom' | 'pan'
Copy link
Contributor Author

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?

Copy link
Contributor

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

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.
@trygve-aaberge-adsk
Copy link
Contributor Author

/huppy check cla

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@MitjaBezensek
Copy link
Contributor

Sorry for some spam here, testing out the logic to skip some of our checks for external contributors.

Copy link
Contributor

@SomeHats SomeHats left a 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',
Copy link
Contributor

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'
Copy link
Contributor

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

@SomeHats
Copy link
Contributor

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?

@trygve-aaberge-adsk
Copy link
Contributor Author

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.

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.

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?

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 editor.dispatch something like this:

  const onMount = useCallback((editor: Editor) => {
    if (!editor._originalDispatch) {
      editor._originalDispatch = editor.dispatch
    }
    editor.dispatch = (info: TLEventInfo) => {
      const cameraOptions = editor.getCameraOptions()

      if (
        info.name === "wheel" &&
        cameraOptions.wheelBehavior !== "pan" &&
        !editor.inputs.isPanning &&
        (info.delta.x !== 0 || (info.delta.y !== 0 && editor.inputs.ctrlKey))
      ) {
        editor.setCameraOptions({ wheelBehavior: "pan" })
      }

      if (
        (info.name === "pointer_down" || info.name === "pointer_move") &&
        editor.inputs.isPanning &&
        cameraOptions.wheelBehavior !== "zoom"
      ) {
        editor.setCameraOptions({ wheelBehavior: "zoom" })
      }

      editor._originalDispatch(info)
      return editor
    }
  }, [])

  return <TldrawEditor onMount={onMount}>...</TldrawEditor>

It would be nice to be able to do this in a proper way though, without having to override editor.dispatch. In other words, what we need is a way to receive the events processed in _flushEventForTick (to determine which wheelBehavior should be used) and then be able to update the camera options before it runs, or in some other way choose which wheelBehavior that function uses.

@SomeHats
Copy link
Contributor

we have our "event" event but that runs after this stuff. Maybe we should introduce a "before-event" event instead?

@SomeHats
Copy link
Contributor

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

@SomeHats SomeHats closed this Jan 29, 2025
@trygve-aaberge-adsk
Copy link
Contributor Author

we have our "event" event but that runs after this stuff. Maybe we should introduce a "before-event" event instead?

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 event event isn't sent for everything. E.g. for wheel events it's not sent because they return before the event is sent: https://github.com/tldraw/tldraw/blob/v3.7.2/packages/editor/src/lib/editor/Editor.ts#L9479

i'm glad you were able to come to a solution here!

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.

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

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.

@trygve-aaberge-adsk
Copy link
Contributor Author

That might make sense. Though events are async, while we would preferably need to have updated camera options before _flushEventForTick runs.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Product improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Make scroll zoom with mouse, but pan with trackpad
4 participants