Conversation
|
@claude please review this PR |
|
@claude review |
|
|
||
| export { calcNewScaledOffsetForZoomCentering } from './calcNewScaledOffsetForZoomCentering'; | ||
|
|
||
| // Minimum distance between two touch points to be considered a valid pinch gesture | ||
| // Uses Apple's Human Interface Guidelines recommended minimum touch target size | ||
| // https://developer.apple.com/design/human-interface-guidelines/accessibility#Mobility | ||
| const MIN_PINCH_DISTANCE = 44; | ||
|
|
||
| /** | ||
| * Calculates the gesture center point relative to the page coordinate system | ||
| * |
There was a problem hiding this comment.
🔴 The Math.max(MIN_PINCH_DISTANCE, actualDistance) clamping introduces a zoom-out artifact: on the frame when fingers first cross below 44px, the ratio clampedDistance / previousUnclampedDistance produces an incorrect zoom jump, then all subsequent frames freeze at ratio 1.0 (since lastGestureTouchDistance is set to 44 and every subsequent call also returns 44). The correct fix is to return null when actualDistance < MIN_PINCH_DISTANCE so the existing if (!distance) return guard in _handlePinching cleanly freezes zoom without any artifact.
Extended reasoning...
What the bug is and how it manifests
This PR introduces Math.max(MIN_PINCH_DISTANCE, actualDistance) clamping inside calcGestureTouchDistance. While the intent is to ignore tiny finger separations (below 44px), the implementation causes a visible zoom-out artifact followed by a complete zoom freeze whenever fingers travel from above 44px to below 44px during a pinch.
The specific code path that triggers it
In _handlePinching, the zoom growth ratio is computed as:
zoomGrowthFromLastGestureState = distance / this.lastGestureTouchDistance
where distance comes from calcGestureTouchDistance and lastGestureTouchDistance is the previous frame's distance. Consider this sequence:
- Frame N: fingers are 80px apart →
calcGestureTouchDistancereturns 80,lastGestureTouchDistance = 80 - Frame N+1: fingers move to 30px apart →
calcGestureTouchDistancereturnsMath.max(44, 30) = 44- ratio = 44 / 80 = 0.55 — a large unexpected zoom-out jump
lastGestureTouchDistanceis then set to 44
- Frame N+2: fingers stay at 30px → returns 44 again
- ratio = 44 / 44 = 1.0 — zoom frozen
- All further frames below 44px: ratio = 1.0, zoom frozen
Why existing code does not prevent it
The existing if (!distance) return guard in _handlePinching correctly handles a null return by freezing zoom cleanly. However, Math.max(...) never returns null — it always returns a positive number (minimum 44). This bypasses the null-guard entirely, forcing the incorrect ratio calculation on every frame.
Impact
Users performing a slow pinch-in gesture will see a sudden zoom-out jump at the threshold crossing (frame N+1), followed by the zoom being completely locked until fingers separate past 44px again. This is the opposite of the intended behavior — instead of gracefully ignoring small separations, the PR actively worsens the UX at the boundary.
How to fix it
Return null instead of clamping:
const actualDistance = Math.sqrt(dx * dx + dy * dy);
if (actualDistance < MIN_PINCH_DISTANCE) return null;
return actualDistance;This causes _handlePinching to hit the if (!distance) return guard, preserving the current zoom level exactly — no jump, no artifact, clean freeze until fingers re-separate beyond the threshold.
Step-by-step proof
- User begins pinch with fingers 100px apart.
calcGestureTouchDistancereturns 100.lastGestureTouchDistance = 100. Zoom ratio = 1.0 (steady). - User moves fingers to 60px apart. Returns 60. Ratio = 60/100 = 0.6 (zoom out). Expected and correct.
- User moves fingers to 30px apart (below MIN_PINCH_DISTANCE=44). With the PR's clamping: returns 44. Ratio = 44/60 ≈ 0.733. This applies an unintended zoom-out corresponding to fingers moving from 60px to 44px — but the user's fingers are actually at 30px. This is wrong.
- Next frame, fingers still at 30px. Returns 44.
lastGestureTouchDistanceis now 44. Ratio = 44/44 = 1.0. Zoom freezes. - The user sees: unexpected zoom-out artifact at step 3, then complete zoom lock at step 4 onward.
With the null fix at step 3: returns null, _handlePinching exits early, zoom stays at the value from step 2. Steps 4+ behave the same (zoom frozen). No artifact at step 3.
There was a problem hiding this comment.
Fixed. Returning null when actualDistance < MIN_PINCH_DISTANCE so the existing if (!distance) return guard in _handlePinching cleanly freezes zoom without any artifact.
| /** | ||
| * Calculates pinch distance | ||
| * | ||
| * @param e | ||
| * @param gestureState | ||
| * @private | ||
| */ | ||
| _handlePanResponderGrant = (e, gestureState) => { |
There was a problem hiding this comment.
🟡 The JSDoc comment on _handlePanResponderGrant incorrectly reads "Calculates pinch distance" — this method actually handles the pan responder grant event (sets up long-press timeout, stops animations, sets gestureStarted = true, and calls raisePin()). The misleading comment is a pre-existing issue in src/ReactNativeZoomableView.tsx line 473 that was carried into the compiled outputs regenerated by this PR.
Extended reasoning...
What the bug is: The JSDoc block immediately above _handlePanResponderGrant in the compiled outputs (lib/module/ReactNativeZoomableView.js and lib/commonjs/ReactNativeZoomableView.js) reads * Calculates pinch distance, which is factually incorrect. The method has nothing to do with calculating pinch distance.
What the method actually does: Reading the method body makes the true purpose clear — it (1) sets up a setTimeout long-press callback if onLongPress prop is provided, (2) calls this.props.onPanResponderGrant?.(...)" to notify the consumer that a touch grant started, (3) stops the pan and zoom animations via panAnim.stopAnimation()/zoomAnim.stopAnimation(), (4) sets this.gestureStarted = true, and (5) calls this.raisePin()`. This is the standard gesture-grant event handler — not a distance-calculation routine.
Where pinch distance is actually calculated: Pinch distance calculation is performed by calcGestureTouchDistance in src/helper/index.ts. The new MIN_PINCH_DISTANCE = 44 constant introduced by this PR is also in that helper file. The comment on _handlePanResponderGrant appears to be a copy-paste error from development history that was never corrected.
Why this PR carries it forward: The source of truth is src/ReactNativeZoomableView.tsx at line 473, which already contains the wrong JSDoc. When the compiled outputs in lib/module/ and lib/commonjs/ were regenerated for this PR, they faithfully reproduced the incorrect comment. Since the PR diff modifies these compiled files, it is an appropriate place to flag the pre-existing documentation error.
Impact: There is no functional impact — JSDoc comments do not affect runtime behavior. However, the misleading comment can confuse contributors trying to understand the gesture-handling flow; someone reading the comment might expect distance math inside _handlePanResponderGrant and look in the wrong place.
Concrete proof: Starting from the method signature in lib/module/ReactNativeZoomableView.js: the JSDoc says Calculates pinch distance, but examining the entire method body shows no arithmetic or distance-related logic — variables like dx, dy, or calls to calcGestureTouchDistance are absent. All method logic is grant-event bookkeeping. The fix is simply updating the comment in src/ReactNativeZoomableView.tsx line 473 to something accurate (e.g., Handles the pan responder grant event) and regenerating the compiled outputs.
There was a problem hiding this comment.
Fixed. Updated the JSDoc in src/ReactNativeZoomableView.tsx to "Handles the pan responder grant event" and regenerated compiled outputs.
Below a certain number of pixels, the math for pinching stops being accurate and the scale starts to do strange things. This PR introduces a minimum distance of 44pt between the fingers for a pinch gesture.
Note
react/jsx-runtime, adopts class fields and optional chaining, and tidies event-callback wiring without changing public APIs.AnimatedTouchFeedback,StaticPin, and animation helpers to the new runtime and minor refactors; no functional UI changes aside from the pinch threshold.Written by Cursor Bugbot for commit 0272ef6. Configure here.