fix: pressability issues new arch#51835
Conversation
|
Any updates on this? |
|
Hello guys, can this fix be cherry-picked into v0.80.x, for example ? |
|
Will also check if this resolve #53366 in next hours. EDIT: |
|
Any alternative solutions while we wait for this PR to be merged? |
|
👀 |
|
when is this going to be merged ? |
|
Any updates on this? |
|
Let me update this PR today or tmrw so there are no merge conflicts and it works with the latest main, then we can maybe move it forward! |
de24f92 to
272dcfd
Compare
|
|
Hey, I rewrote this PR to be mergable against Kindly requesting another round of review @Hardanish-Singh For folks needing this fix for older RN versions, I kept a copy of the old branch targeting RN 0.79 here: |
cipolleschi
left a comment
There was a problem hiding this comment.
This change is kind of big and touches multiple parts of React Native.
I strongly suggest to wrap it within a feature flag so we can easily switch back to the old behavior in case it misbehave in prod in our apps.
ideally, it would be better if you can split it in smaller pieces, but perhaps it is easier to review as a whole for an approval and then we can split it.
I don't have the full picture on this, so I'll forward to somebody in the team that can provide a good review.
|
Thanks for the first quick check! Let me add a feature flag first. The feature flag could gate this call in
if (typeof this._responderID === 'number') {
UIManager.measure(this._responderID, this._measureCallback);
} else {
- this._responderID.measure(this._measureCallback);
+ this._responderID.measureAsyncOnUI(this._measureCallback);
}
}Putting the native code behind a feature flag would be a bit difficult since we touch turbo module specs. Adding a new virtual method to the scheduler kind of trickles down into all those places touched here, so it would be a bit hard to split the PR up otherwise. |
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** Ported patch from react/react-native#51835 to version 0.76.9 I also had to enable build from source for Android in order to make patch work, that will result in increased build time. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Introduces a React Native patch adding `measureAsyncOnUI` across Fabric/UIManager and switches app components to plain `TouchableOpacity`, while enabling Android builds to compile RN from source. > > - **React Native (patched)**: > - Add `measureAsyncOnUI` API across Fabric/UIManager (JS, iOS, Android, JNI) and expose via `UIManagerBinding`. > - Update `Pressability` and `ReactFabricHostComponent` to use `measureAsyncOnUI` for view measurement. > - **Android Build**: > - Enable building `react-native` from source with Gradle dependency substitutions. > - **App Components**: > - Simplify touch handling in `ButtonBase`, `ListItemMultiSelect`, and `ListItemSelect` by removing `react-native-gesture-handler` tap wrappers and using `RNTouchableOpacity` directly. > - **Tooling**: > - Add Yarn patch/resolution for `react-native@0.76.9` and update `yarn.lock`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit da7a9a0. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
|
Hi @cipolleschi, It would be great if someone from Meta could take a look to all pressability issues in new arch or try to implement the requested feature flag. It's a bug present and reported multiples time from months now... |
|
The Pressable |
Add react-native patch based on upstream PR react/react-native#51835 to fix Android-specific onPress failures for Pressable components inside Tooltip on certain Samsung devices. The root cause is that Pressability.measure() reads stale layout info from the shadow tree while Reanimated has already updated the native view. This patch introduces measureAsyncOnUI which measures using the native view hierarchy on the UI thread. Also removes the createPressHandler workaround since this patch properly fixes the root cause. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
Add react-native patch based on upstream PR react/react-native#51835 to fix Android-specific issue where onPress events do not trigger for Pressable components when used inside a Tooltip on certain Samsung devices. Also removes the createPressHandler workaround since this patch properly fixes the root cause. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
Re-apply the changes from PR #86160 which was reverted. Adds a react-native patch based on upstream react/react-native#51835 to fix onPress events not triggering for Pressable components inside Tooltips on certain Samsung devices. Also removes the createPressHandler workaround since this patch properly fixes the root cause. Co-authored-by: linhvovan29546 <linhvovan29546@gmail.com> Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
Re-apply the changes from PR #86160 which was reverted. Adds a react-native patch based on upstream react/react-native#51835 to fix onPress events not triggering for Pressable components inside Tooltips on certain Samsung devices. Also removes the createPressHandler workaround since this patch properly fixes the root cause.
7f2df56 to
7aa9495
Compare
|
Warning JavaScript API change detected This PR commits an update to
This change was flagged as: |
Would appreciate it very much if we could revisit this PR, thanks in advance! 😊 |

Summary:
Addresses: #51621
Fixes press ability issues on new arch by adding a separate path for measuring, called:
measureAsyncOnUIthis method will call into the platform MountingManager implementations to measure using just the native view hierarchy.
This way the measurement should always be correct.
Note: the method for measuring is basically copied over from old arch code.
Changelog:
[GENERAL] [ADDED] Add
HostInstance.measureAsyncOnUI(callback)for measuring host views from the native view hierarchy on the UI thread.[GENERAL] [FIXED] Pressability issues on new arch
Test Plan: