Make the detector frame cover transformed children - #4532
Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe Android detector layout now includes transformed child frames when calculating bounds. It also recalculates overflow insets from content bounds after child positioning. Non-Android behavior remains unchanged. ChangesAndroid detector bounds
Priority: ➖ Normal Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review effort: Lite
Findings: None
What changed in this PR
Extends the Android Fabric GestureDetector frame to include transformed children, allowing native controls outside their layout bounds to receive touches.
Changes:
- Includes transformed child bounds in detector geometry on Android.
- Recomputes overflow insets after repositioning children.
- Preserves untransformed bounds for existing gesture behavior.
| File | Description |
|---|---|
| packages/react-native-gesture-handler/shared/shadowNodes/react/renderer/components/rngesturehandler_codegen/RNGestureHandlerDetectorShadowNode.cpp | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Description
On Android, native views (
Switch,TextInput, nativeScrollView, sliders) inside aGestureDetectorstop receiving touches once a transform moves them outside the detector's frame. Gestures attached to the detector keep working, since the orchestrator does its own transform-aware traversal (#4251), but the touch never reaches the native child.The detector's frame is the bounding box of its children's layout frames. Transforms are not layout, so a transformed child can end up outside that frame. Android's
ViewGrouponly dispatches a touch to a child whose bounds contain it, so the touch is rejected at the detector before the child's transform is ever considered. This is a general RN Android limitation for any parent view tighter than its transformed content, but layout-only wrappers get flattened and the detector never is, soGestureDetectoris where users hit it. iOS is unaffected, since the detector forces a non-zerooverflowInsetthere and RN's hit test then searches outside the bounds.The fix extends the frame with each child's transformed frame and recomputes
overflowInsetagainst the resulting frame, since the default value is computed against the zero-size frame of adisplay: contentsnode. The untransformed frame stays in the union, so the orchestrator's child-space check from #4251 still covers the vacated area.The frame follows a transform only when layout runs: at mount, on React commits, and when something else re-lays out the animated subtree, such as a measurable child like
SwitchorTextInput. A purely Reanimated-driven transform with no such trigger keeps a stale frame until the next layout. Following it every frame costs a Yoga pass per detector, measured at 6x worse frame times.Fixes #4529
Performance
Performance on a Pixel 9 Pro emulator, release build, 1000 detectors animating at once, frame times from
dumpsys gfxinfoover 10 s:Text cards never re-layout, so the cost is zero. Switch cards make RN run layout every frame on main already, and this PR adds the frame and inset mutation on top. That is the only measured cost and it only applies to many detectors animating simultaneously with measurable leaves inside.
Test plan
Tested on the following code: