Skip to content

fix(Android,Fabric): pressables losing focus on screens with formSheet presentation #2788

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

Merged
merged 11 commits into from
Mar 18, 2025

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Mar 18, 2025

Description

This PR relates to new architecture only.

Currently, the pressables on form sheet lose focus on move action. This is the same problem we had with Pressables in screen & header on new architecture.
See: #2466

Basically the information about sheet position is different between ShadowTree (ST) and HostTree (HT), which leads to losing focus due to how pressables
are now handled on new architecture.

Simplified description of basically what happens when you click a pressable on new arch is as follows (Android):

  1. The gesture is detected after the host platform dispatches touch event (touch events on Android are dispatched in top-down manner) (link)
  2. The JS responder is set & touch event dispatched - therefore pressable is always clickable,
  3. Moreover, the JS responder is measured IN THE ST
  4. When you start moving your finger, the platform is still the source of motion events, and target coordinates are read from HOST TREE
  5. Motion event (with platform measurement (HT)!!!) is then compared in JS with responder measurements based on ST

Therefore, any inconsistency here leads to responder losing focus.

Reference:

  1. TouchesHelper.createPointersArray
  2. Responder measurement
  3. Gesture start detection (Android)
  4. Gesture move handling (Android)
  5. Checking whether native touch is in responder region

What is bewildering is that it works on iOS, despite the fact, that e.g. when using React inspector the views are completely out of place in ShadowTree.
I haven't debugged the iOS part to the very bottom, but my suspicion is as follows:

  1. The form sheet on Android is mounted in subtree under react root view, while on iOS it is mounted under separate UITransitionView (different subtree of window),
  2. additionally we use RNSModalScreen component on iOS, which has shadow node with RootNodeKind (measurements in subtree of its shadow node are done relative to it),
  3. There is no root view / any react view above it -> native measurement (done by react-native) might also think that its positioned at (0, 0) (same as in shadow tree).

This is something to verify in the future. Opened ticked on the board for it.

Changes

The sheet now sends updates to the shadow tree at following moments:

  1. layout (including initial one),
  2. sheet detent change to any stable state (could consider here not sending the update when the sheet is hidden, added to project board),

I'm not sending updates on every sheet move (View.top change), to avoid clogging the JS thread (and later UI), whole advantage of our sheet is that it feels smooth (given sensible Android device).
However, it could be considered in case of any further issues.

Please note, that between the event syncs or in case the JS thread is blocked / clogged, this still will be a problem. However, any JS would lag, not only pressables and it's not down to us.

Commits:

  • Prevent modal content from disappearing
  • Add comment that createShadowNode is used only on old arch
  • Add comment on threading on NativeProxy mechanisms
  • Simplify code in RNSModalScreenShadowNode.cpp
  • Make modal screen component selection more clear in Screen.tsx
  • Add headerHeight to diff prop list when determining need for shadow state update
  • Add pressables to form sheet example!
  • Send updates to shadow tree on form sheet layout changes in appropriate moments

Recordings

before after
Screen.Recording.2025-03-18.at.14.22.17.mov
Screen.Recording.2025-03-18.at.14.18.08.mov

Test code and steps to reproduce

TestFormSheet, open the sheet, play with it & see that pressables work!

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

Copy link
Member Author

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Notes for future me

Comment on lines +46 to +48
if (abs(lastWidth - realWidth) < delta &&
abs(lastHeight - realHeight) < delta &&
abs(lastHeaderHeight - realHeaderHeight) < delta
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a catch - dunno why the realHeaderHeight has not been diffed before - it surely should be.

Comment on lines +179 to +181
// In case of form sheet we get layout notification a bit later, in `onBottomSheetBehaviorDidLayout`
// after the attached behaviour laid out this view.
if (changed && isNativeStackScreen && !usesFormSheetPresentation()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

In case of formSheet, when CoordinatorLayout layouts it calls to the BottomSheetBehavior to layout the child (Screen), which in turn calls CoordinatorLayout.layoutChild(Screen) leading to a call to the overridden onLayout method here - and this is where we would update the shadow state before.

This was not good, because at the moment of onLayout being called, the behaviour hadn't finished its layout process yet - it later would modify top property of the Screen, which we are interested in.

Therefore I've moved the "onlayout" callback to new method onBottomSheetBehaviorDidLayout which is called from our ScreensCoordinatorLayout after it has completely finished computing the layout.

I decided to not move all the code to the mentioned new callback, because the semantics are a bit different, especially the changed parameter - in case of Screen.onLayout it determines whether the bounds of the screen changed. In case of onBottomSheetBehaviorDidLayout the coordinatorLayoutDidChange informs whether coordinator layout bounds did change. I don't know whether this (moving all the logic to the new callback) would lead to some issues or not, therefore I've decided to leave regular screen behaviour intact for now.

Copy link
Contributor

@kligarski kligarski left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Member Author

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

CI passes locally, here it fails most likely due to slow runner.

@kkafar kkafar merged commit 0547b19 into main Mar 18, 2025
9 of 11 checks passed
@kkafar kkafar deleted the @kkafar/fix-pressables-in-formsheet-android branch March 18, 2025 18:13
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