Skip to content
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

'[skip ci] [RN][C++] Fix UIManagerBinding'\''s findNodeAtPoint #42439

Closed
wants to merge 3 commits into from

Conversation

RSNara
Copy link
Contributor

@RSNara RSNara commented Jan 20, 2024

Summary:
UIManagerBinding'''s findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See LayoutableShadowNode::findNodeAtPoint:

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Jan 20, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
…ook#42439)

Summary:
Pull Request resolved: facebook#42439

UIManagerBinding'\''s findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512

fbshipit-source-id: f7fdede23af80e78ebb02aa27aaac6a116797f5a
@RSNara RSNara force-pushed the export-D52909512 branch 2 times, most recently from 9237bf6 to 1fae9eb Compare January 20, 2024 23:35
RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
…ook#42439)

Summary:
Pull Request resolved: facebook#42439

UIManagerBinding'\''s findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512

fbshipit-source-id: c8d53f8f417262a974ad1f36ea783bb3837108f2
@RSNara RSNara force-pushed the export-D52909512 branch 2 times, most recently from a520b35 to 402af14 Compare January 20, 2024 23:42
RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
RSNara added a commit to RSNara/react-native that referenced this pull request Jan 20, 2024
Summary:
Pull Request resolved: facebook#42439

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Differential Revision: D52909512

Reviewed By: cortinico, sammy-SC

fbshipit-source-id: 6a05c9215ac1ad6726d00bc6db7a9a2fdb68b30d
RSNara added a commit to RSNara/react-native that referenced this pull request Jan 23, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 23, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 23, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 24, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 24, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 24, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

RSNara added a commit to RSNara/react-native that referenced this pull request Jan 25, 2024
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

Summary:
This API was used by the old architecture to lazily register/load components:
1. Load a ViewManager's class from the disk
2. Register the ViewManager's class with React Native

See: [RCTUIManager lazilyLoadView](https://github.com/facebook/react-native/blob/822bf52c29729d25b2bfb31655cf773609a9283d/packages/react-native/React/Modules/RCTUIManager.m#L1546-L1591)

The new architecture **does not** support lazy loading of **legacy** modules/components.

Therefore, let's leave this API unimplemented until we decide to implement lazy loading of legacy stuff in new architecture.

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D52677515
Summary:
If Flow can be trusted, getConstantsForViewManager always gets called with a non-null string:
1. The only call-site to getConstantsForViewManager is getViewManagerConfig: [PaperUIManager.js](https://github.com/facebook/react-native/blob/822bf52c29729d25b2bfb31655cf773609a9283d/packages/react-native/Libraries/ReactNative/PaperUIManager.js#L36-L80)
2. And getViewManagerConfig always passes in a non-null string.

So, let's just make the native argument type a non-nullable string.

Thoughts?

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D52628937
Summary:

UIManagerBinding's findNodeAtPoint assumes that UIManager::findNodeAtPoint returns non-null pointers. But, this is false! See [LayoutableShadowNode::findNodeAtPoint](https://github.com/facebook/react-native/blob/fd0ca4dd6209d79ac8c93dbffac2e3dca1caeadc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L333-L354):

https://www.internalfb.com/code/fbsource/[7169da7945c813b35cd0a4fa15e66969c90c2481]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp?lines=333%2C354

Changelog: [General][Fixed] Fix UIManagerBinding's findNodeAtPoint

Reviewed By: cortinico, sammy-SC

Differential Revision: D52909512
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D52909512

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jan 26, 2024
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 62e5980.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants