Skip to content

Add contentX/contentY to ZoomableViewEvent#181

Merged
elliottkember merged 2 commits into
masterfrom
add-content-coordinates-to-zoomable-view-event
May 19, 2026
Merged

Add contentX/contentY to ZoomableViewEvent#181
elliottkember merged 2 commits into
masterfrom
add-content-coordinates-to-zoomable-view-event

Conversation

@elliottkember

Copy link
Copy Markdown
Collaborator

Summary

  • Add optional contentX / contentY to ZoomableViewEvent carrying the gesture's first touch in content (bitmap) coordinates — so consumers can tell where the user tapped on the contents.
  • Populated when the callback has an associated GestureTouchEvent AND both contentWidth and contentHeight props are set. Computed via the existing viewportPositionToImagePosition helper (same contain resize-mode assumption).
  • Undefined for non-gesture call sites (onTransformWorklet, onZoomEnd after a programmatic zoomTo() completes naturally) and when content dimensions aren't configured.
  • SPECS.md updated to reflect the new event shape and document when the fields are populated.

Test plan

  • Verify a single tap fires onSingleTap with contentX/contentY set when contentWidth/contentHeight are configured.
  • Verify contentX/contentY are undefined when contentWidth/contentHeight are not set.
  • Verify contentX/contentY are undefined on onZoomEnd fired from programmatic zoomTo() completion (no gesture event).
  • Verify values are reasonable across zoom levels (taps near edges and after pan/zoom).

🤖 Generated with Claude Code

Expose the gesture's first touch in content (bitmap) coordinates on
ZoomableViewEvent so consumers can tell where the user tapped on the
contents. Populated when the callback has an associated gesture event
and both contentWidth/contentHeight props are set; undefined otherwise.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented May 18, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Low risk additive change: event payloads gain optional contentX/contentY computed only when a gesture event and contentWidth/contentHeight are available, with guards to avoid emitting invalid values.

Overview
Adds optional content-space touch coordinates to callback event payloads by extending ZoomableViewEvent with contentX/contentY.

Updates ReactNativeZoomableView to compute these fields from the first touch via viewportPositionToImagePosition and to pass the gesture event into _getZoomableViewEventObject for gesture-driven callbacks, while leaving the fields undefined for non-gesture call sites and when content dimensions are unknown. Documentation in SPECS.md is updated to describe the new fields and when they are populated.

Reviewed by Cursor Bugbot for commit 2903b19. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 2903b19. Configure here.

Comment thread src/ReactNativeZoomableView.tsx Outdated
Comment thread src/ReactNativeZoomableView.tsx Outdated
`{}` placeholders showed up at almost every call site after adding the
gesture-event parameter — a signal the override slot was carrying its
weight for exactly one caller (`onDoubleTapAfter`, which substitutes the
target `zoomLevel`). Let that one caller spread the override externally
and simplify the helper to just `(gestureEvent?)`.

Also drops the redundant override in `_staticPinPosition` that re-passed
the same SharedValues the function already reads.

Side effect: at `onDoubleTapAfter`, `contentX`/`contentY` are now
computed using the current zoom level rather than the target, which
matches the touch's actual content-space position (the touch occurred
before the zoom animation started).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
// size, so it returns null pre-measurement or with no content size set —
// leave `contentX`/`contentY` undefined in that case rather than emit
// NaN/Infinity to consumers.
const touch = gestureEvent?.allTouches[0];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 we only need x and y from the touch here. Passing the whole event could lead to misinterpretation of the event when the first touch is not intended to be the source for the x and y.

@elliottkember elliottkember May 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by passing the whole event? We're only passing this touch to viewportPositionToImagePosition to calculate the content position?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I meant passing the whole event into the getZoomableViewObject function

offsetY: offsetY.value,
zoomLevel: zoom.value,
}),
zoomableEvent: _getZoomableViewEventObject(),

@thomasttvo thomasttvo May 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 _staticPinPosition can probably be replaced by _getZoomableViewEventObject(staticPinPosition.value) making the interface for static pin more standardized

@elliottkember elliottkember May 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not without changing the _getZoomableViewEventObject because it needs a GestureTouchEvent we don't have?

Comment thread src/typings/index.ts
* programmatic `zoomTo()`) and when content dimensions are unknown.
*/
contentX?: number;
contentY?: number;

@thomasttvo thomasttvo May 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 this might create confusion from the client in the form of: "is there a time where we have contentX but not contentY?" it's probably best to use TS to make them either both undefined or exists, or even better, group them in a property

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe all this complexity is an indicator that ZoomableViewEvent is doing too much. Thinking about it - why would onSingleTap need to return originalHeight and originalWidth. This is probably a flawed legacy design we need to move away in v3.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, but I imagine it's so that the consumer can have all the info it needs packed into the event. Which I think is fairly reasonable if you already have an event object. It could definitely be better arranged, that's for sure. I don't really know why most of these attributes are exposed to the client

@elliottkember elliottkember May 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate optional values is okay here, it's consistent with contentWidth? / contentHeight? which are both optional. They're all flat in this interface:

  initialOffsetX?: number;
  initialOffsetY?: number;
  contentWidth?: number;
  contentHeight?: number;

@elliottkember elliottkember merged commit d549867 into master May 19, 2026
1 check passed
@elliottkember elliottkember deleted the add-content-coordinates-to-zoomable-view-event branch May 19, 2026 01:05
@elliottkember

elliottkember commented May 19, 2026

Copy link
Copy Markdown
Collaborator Author

@thomasttvo After trying to integrate this, I think these attributes probably should live on the TouchData event instead

Scratch that — the touchData is part of react-native-gesture-handler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants