fix(roam): proper touch pinch support for zoom & pan - #21655
Conversation
|
Thanks for your contribution! Please DO NOT commit the files in dist, i18n, and ssr/client/dist folders in a non-release pull request. These folders are for release use only. |
NightRa
changed the title
|
@100pah I'd love a review by you or by anyone you see fit. |
|
@plainheart Can you take a look please? I would like to get this merged :) |
plainheart
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates RoamController’s touch pinch handling to use the real ZRender-reported pinchScale, add two-finger pan during pinch (by tracking pinch-center movement), and make pinch capture behave more like mouse drag (capture once, then continue handling outside the roam area).
Changes:
- Use
e.pinchScaledirectly for zoom instead of a fixed 1.1 step. - Track pinch center (
_pinchX/_pinchY) to emitpanduring a pinch when the center moves. - Split roam capability into
_moveEnabled/_zoomEnabledand only attach thepinchlistener when either is enabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Gate only the beginning of a pinch capture. | ||
| // Once captured, keep handling movement outside the roam area. | ||
| // Requiring a native touchstart for a new capture also | ||
| // prevents another RoamController from picking up an already-moving pinch. | ||
| const isTouchStart = e.event.type === 'touchstart'; | ||
| const isPinchStart = !this._pinching || isTouchStart; | ||
| if (isPinchStart) { | ||
| if (!isTouchStart || !this._checkPointer(e, originX, originY)) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
ZRender does emit a pinch during touchstart once the event contains two touches. GestureMgr falls back to using the current sample as pinchPre, producing pinchScale === 1, and Handler.processGesture dispatches that event while the native event type remains touchstart.
I verified this against ZRender 6.1.0 and the current ZRender source.
Requiring touchstart here is intentional: it ensures pinch ownership is decided only when the gesture begins. Allowing an uncaptured controller to start on touchmove would let a pinch be acquired after entering another roam area, potentially causing unexpected mid-gesture zoom or transferring ownership between overlapping controllers.
There was a problem hiding this comment.
And empirically this works well too - you may take a look at the attached videos
| } | ||
| this._moveEnabled = moveEnabled; | ||
| this._zoomEnabled = zoomEnabled; |
There was a problem hiding this comment.
Thanks, you’re correct that _controlType is never assigned, so the existing idempotency check always removes and re-adds the listeners, but this behavior predates this PR.
Assigning _controlType alone may not be safe, however. Listener registration also captures component, zlevel, z, and z2, which determine precedence between overlapping controllers. These values are recalculated by every enable() call; skipping registration solely because controlType is unchanged could leave stale ordering metadata when a component’s z-order changes.
A complete fix should invalidate or update the registration when either controlType or that z-order metadata changes. Since this is an existing issue rather than a regression from the pinch changes, I suggest handling it separately.
There was a problem hiding this comment.
@plainheart / @100pah - do you want me to implement this as well here? It's a bit out of scope and will make the PR quite larger
Brief Information
This pull request is in the type of:
What does this PR do?
Improve
RoamControllerpinch handling by using the real touch pinch scale, supporting two-finger pan, and capturing pinch gestures consistently.Fixed issues
Fixes #18949
Fixes #18113
Related #21417
Related #20939
Details
Before: What was the problem?
Touch pinch zoom used a fixed zoom step:
This made small frame-to-frame pinch changes to zoom exponentially instead of according to the pinch distance.
Pinch also emitted only zoom events. When the pinch center moved, roamable views did not pan with the fingers. Additionally, pinch hit-testing was repeated on every frame, so an active pinch could stop when moving outside the roam area, or be picked up mid-gesture by another overlapping
RoamController.After: How does it behave after the fixing?
RoamControllernow uses the actuale.pinchScalereported by ZRender.Pinch is treated as a two-finger roam gesture:
roam: true: pinch can pan and zoom.roam: 'move'/'pan': pinch can pan without zooming.roam: 'scale'/'zoom': pinch can zoom without panning.A pinch is captured only when it starts from a native
touchstartinside the valid roam area. Once captured, it continues outside the area like mouse drag, and anotherRoamControllercannot pick up the already-moving pinch.Document Info
Misc
Security Checking
ZRender Changes
Related test cases or examples to use the new APIs
Manual examples:
test/dataZoom-timeAxis.htmldataZoom-rainfall-inside.htmltest/geo-map-roam.htmltest/graph-layout-roam.htmltest/tree-roam.htmltest/sankey-roam.htmltest/treemap-simple.htmltest/treemap-scaleLimit.htmltest/graph-thumbnail.htmlI've tested the changes in all the mentioned test examples on an Android phone in Chrome.
Merging options
Before & After videos
dataZoom-Before.mp4
dataZoom-After.mp4
Geo-Before.mp4
Geo-After.mp4