-
Notifications
You must be signed in to change notification settings - Fork 25.1k
fix: pressability issues new arch #51835
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
base: main
Are you sure you want to change the base?
fix: pressability issues new arch #51835
Conversation
Hardanish-Singh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 facebook/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 |

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] [FIXED] Pressability issues on new arch
Test Plan: