-
Notifications
You must be signed in to change notification settings - Fork 127
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
RFC: DOM traversal and layout APIs in React Native #607
RFC: DOM traversal and layout APIs in React Native #607
Conversation
Hi, just some generic questions about this for my own understanding:
|
@tido64 this also requires some new methods in the UIManager binding to expose the information to JavaScript. Something similar to these methods: We might also have to the a few methods in
We expect this to be low and can optimize as needed. The implementation wouldn't be as large as it looks. With some other APIs defined as globals we can optimize them by removing the global altogether if not used. With these methods it's going to be harder as they're exposed through refs. It's unlikely we'll be able to statically determine whether they're used or not. |
…derer (#26282) ## Summary I'm going to start implementing parts of this proposal react-native-community/discussions-and-proposals#607 As part of that implementation I'm going to refactor a few parts of the interface between React and React Native. One of the main problems we have right now is that we have private parts used by React and React Native in the public instance exported by refs. I want to properly separate that. I saw that a few methods to attach event handlers imperatively on refs were also exposing some things in the public instance (the `_eventListeners`). I checked and these methods are unused, so we can just clean them up instead of having to refactor them too. Adding support for imperative event listeners is in the roadmap after this proposal, and its implementation might differ after this refactor. This is essentially a manual revert of #23386. I'll submit more PRs after this for the rest of the refactor. ## How did you test this change? Existing jest tests. Will test a React sync internally at Meta.
…derer (#26282) ## Summary I'm going to start implementing parts of this proposal react-native-community/discussions-and-proposals#607 As part of that implementation I'm going to refactor a few parts of the interface between React and React Native. One of the main problems we have right now is that we have private parts used by React and React Native in the public instance exported by refs. I want to properly separate that. I saw that a few methods to attach event handlers imperatively on refs were also exposing some things in the public instance (the `_eventListeners`). I checked and these methods are unused, so we can just clean them up instead of having to refactor them too. Adding support for imperative event listeners is in the roadmap after this proposal, and its implementation might differ after this refactor. This is essentially a manual revert of #23386. I'll submit more PRs after this for the rest of the refactor. ## How did you test this change? Existing jest tests. Will test a React sync internally at Meta. DiffTrain build for [d49e0e0](d49e0e0)
38a7222
to
ae807a7
Compare
…ule in Fabric (#26321) ## Summary The current definition of `Instance` in Fabric has 2 fields: - `node`: reference to the native node in the shadow tree. - `canonical`: public instance provided to users via refs + some internal fields needed by Fabric. We're currently using `canonical` not only as the public instance, but also to store internal properties that Fabric needs to access in different parts of the codebase. Those properties are, in fact, available through refs as well, which breaks encapsulation. This PR splits that into 2 separate fields, leaving the definition of instance as: - `node`: reference to the native node in the shadow tree. - `publicInstance`: public instance provided to users via refs. - Rest of internal fields needed by Fabric at the instance level. This also migrates all the current usages of `canonical` to use the right property depending on the use case. To improve encapsulation (and in preparation for the implementation of this [proposal to bring some DOM APIs to public instances in React Native](react-native-community/discussions-and-proposals#607)), this also **moves the creation of and the access to the public instance to separate modules** (`ReactFabricPublicInstance` and `ReactFabricPublicInstanceUtils`). In a following diff, that module will be moved into the `react-native` repository and we'll access it through `ReactNativePrivateInterface`. ## How did you test this change? Existing unit tests. Manually synced the PR in Meta infra and tested in Catalyst + the integration with DevTools. Everything is working normally.
Summary: This adds a feature flag to enable all the new DOM traversal and layout APIs (in react-native-community/discussions-and-proposals#607). Changelog: [Internal] Reviewed By: rshest Differential Revision: D43981608 fbshipit-source-id: 77bb1ee4faaaf30cfc8bb2e493763b168702f498
Summary: This implements a basic version of HTMLCollection that's close to the spec but diverges in some things (e.g.: methods could be called with an instance created through `Object.create`, etc.). This will be used soon to implement `ReadOnlyElement.children` (behind a flag). See: react-native-community/discussions-and-proposals#607 Changelog: [internal] Reviewed By: yungsters Differential Revision: D44055912 fbshipit-source-id: 37bcd7c12916b95a258e6b2e5717a642f478abdf
Summary: This implements a basic version of NodeList that's close to the spec but diverges in some things (e.g.: methods could be called with an instance created through `Object.create`, etc.). This will be used soon to implement `ReadOnlyNode.childNodes` (behind a flag). See: react-native-community/discussions-and-proposals#607 Changelog: [internal] Reviewed By: yungsters Differential Revision: D44055911 fbshipit-source-id: 10b435b06ea6f75eaead85f01c2703e05bb3bd37
Summary: This implements a basic version of DOMRectList that's close to the spec but diverges in some things (e.g.: methods could be called with an instance created through `Object.create`, etc.). This will be used soon to implement `ReadOnlyelement.getClientRects()` (behind a flag). See: react-native-community/discussions-and-proposals#607 Changelog: [internal] Reviewed By: yungsters Differential Revision: D44060540 fbshipit-source-id: ad29b5c41f2778864e7dd7ece9223dcf73cd5d6c
Summary: This just creates the class structure for `ReadOnlyNode`, `ReadOnlyElement` and `ReactNativeElement`, with all methods throwing as unimplemented. These classes will be gated behind a feature flag, so merging incomplete work is ok. This doesn't add the future setters that will log warnings / throw errors when used. See: react-native-community/discussions-and-proposals#607 Changelog: [internal] Reviewed By: rickhanlonii, yungsters Differential Revision: D44060539 fbshipit-source-id: e489532fd365d9aa2bb8308847a35eb715d675e7
…26416) ## Summary We are going to move the definition of public instances from React to React Native to have them together with the native methods in Fabric that they invoke. This will allow us to have a better type safety between them and iterate faster on the implementation of this proposal: react-native-community/discussions-and-proposals#607 The interface between React and React Native would look like this after this change and a following PR (#26418): React → React Native: ```javascript ReactNativePrivateInterface.createPublicInstance // to provide via refs ReactNativePrivateInterface.getNodeFromPublicInstance // for DevTools, commands, etc. ReactNativePrivateInterface.getNativeTagFromPublicInstance // to implement `findNodeHandle` ``` React Native → React (ReactFabric): ```javascript ReactFabric.getNodeFromInternalInstanceHandle // to get most recent node to call into native ReactFabric.getPublicInstanceFromInternalInstanceHandle // to get public instances from results from native ``` ## How did you test this change? Flow Existing unit tests
Summary: Pull Request resolved: facebook#39308 This adds a new method in Fabric to get the border size for an element, and uses it to implement the following methods as defined in react-native-community/discussions-and-proposals#607 : * `clientLeft`: left border width of the element. * `clientTop`: top border width of the element. If the element isn't displayed or it has display: inline, it return 0 in both cases. These APIs provide rounded integers. Changelog: [internal] Reviewed By: mdvacca Differential Revision: D49009140 fbshipit-source-id: 7cc2fac1ec0526a5ad441bf71039333e10ff9696
Summary: Pull Request resolved: facebook#39305 This adds a new method in Fabric to get the inner size for an element (whole size excluding borders, which would be the scrollable size of the element), and uses it to implement the following methods as defined in react-native-community/discussions-and-proposals#607 : `clientWidth`: width of the element excluding the size of the left and right border. `clientHeight`: height of the element excluding the size of the top and bottom border. If the element isn't displayed or it has display: inline, it return `0` in both cases. These APIs provided rounded integers. Changelog: [internal] Reviewed By: NickGerleman Differential Revision: D49008698 fbshipit-source-id: e9fc54775f162809b026de23397df9cbe59d8d80
Summary: This adds a new method in Fabric to get the scroll size for an element, and uses it to implement `scrollWidth` and `scrollHeight` as defined in react-native-community/discussions-and-proposals#607 Scroll size determine how much of the content of a node would move if the node was scrollable. If the content does not overflow the padding box of the node, then this is the same as the `client{Width,Height}` (the size of the node without its borders). If the content would overflow the node, then it would be the size of the content that would be scrollable (in other words, what would "move" when you scrolled). If the element isn't displayed or it has display: inline, it return 0 in both cases. These APIs provide rounded integers. Changelog: [internal] Differential Revision: D49058368 fbshipit-source-id: 04831ce15229e732a7d57fc4554c13ec19b362af
Summary: Pull Request resolved: #39305 This adds a new method in Fabric to get the inner size for an element (whole size excluding borders, which would be the scrollable size of the element), and uses it to implement the following methods as defined in react-native-community/discussions-and-proposals#607 : `clientWidth`: width of the element excluding the size of the left and right border. `clientHeight`: height of the element excluding the size of the top and bottom border. If the element isn't displayed or it has display: inline, it return `0` in both cases. These APIs provided rounded integers. Changelog: [internal] Reviewed By: NickGerleman Differential Revision: D49008698 fbshipit-source-id: 7c25b8c5ddbba7877ea398398f7a0b755e25d746
Summary: Pull Request resolved: #39308 This adds a new method in Fabric to get the border size for an element, and uses it to implement the following methods as defined in react-native-community/discussions-and-proposals#607 : * `clientLeft`: left border width of the element. * `clientTop`: top border width of the element. If the element isn't displayed or it has display: inline, it return 0 in both cases. These APIs provide rounded integers. Changelog: [internal] Reviewed By: mdvacca Differential Revision: D49009140 fbshipit-source-id: e667059702ca22e2b8e8721209e9c5c2553aa7ac
Summary: Pull Request resolved: facebook#39328 This adds a new method in Fabric to get the scroll size for an element, and uses it to implement `scrollWidth` and `scrollHeight` as defined in react-native-community/discussions-and-proposals#607 Scroll size determine how much of the content of a node would move if the node was scrollable. If the content does not overflow the padding box of the node, then this is the same as the `client{Width,Height}` (the size of the node without its borders). If the content would overflow the node, then it would be the size of the content that would be scrollable (in other words, what would "move" when you scrolled). If the element isn't displayed or it has display: inline, it return 0 in both cases. These APIs provide rounded integers. NOTE: The current implementation of `ScrollView` has several known bugs and inconsistencies across platforms (Android vs. iOS) and architectures (Paper vs. Fabric) (e.g.: content showing on top of the border on Android, `overflow: visible` only working on Android but not on iOS, etc.). The data that this API reports is the one that aligns with the Web (with a few limitations), and we'll need to fix the implementation to align with this. NOTE: transforms are not considered correctly for the sake of this API, but also not applied correctly in any of the native platforms. On Web, the scrollable area is the overflow of all the children **with transforms applied** which isn't honored in RN. We''ll fix the data reported by this API when we also fix the user perceived behavior. Changelog: [internal] Differential Revision: D49058368 fbshipit-source-id: 9b8cbc0810820655f1dd510ca7bdb212516e3610
Summary: Pull Request resolved: facebook#39328 This adds a new method in Fabric to get the scroll size for an element, and uses it to implement `scrollWidth` and `scrollHeight` as defined in react-native-community/discussions-and-proposals#607 Scroll size determine how much of the content of a node would move if the node was scrollable. If the content does not overflow the padding box of the node, then this is the same as the `client{Width,Height}` (the size of the node without its borders). If the content would overflow the node, then it would be the size of the content that would be scrollable (in other words, what would "move" when you scrolled). If the element isn't displayed or it has display: inline, it return 0 in both cases. These APIs provide rounded integers. NOTE: The current implementation of `ScrollView` has several known bugs and inconsistencies across platforms (Android vs. iOS) and architectures (Paper vs. Fabric) (e.g.: content showing on top of the border on Android, `overflow: visible` only working on Android but not on iOS, etc.). The data that this API reports is the one that aligns with the Web (with a few limitations), and we'll need to fix the implementation to align with this. NOTE: transforms are not considered correctly for the sake of this API, but also not applied correctly in any of the native platforms. On Web, the scrollable area is the overflow of all the children **with transforms applied** which isn't honored in RN. We''ll fix the data reported by this API when we also fix the user perceived behavior. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D49058368 fbshipit-source-id: 1b3652f24c0aba4e9d47cd0e96fbad4a18729b2f
Summary: Pull Request resolved: facebook#39328 This adds a new method in Fabric to get the scroll size for an element, and uses it to implement `scrollWidth` and `scrollHeight` as defined in react-native-community/discussions-and-proposals#607 Scroll size determine how much of the content of a node would move if the node was scrollable. If the content does not overflow the padding box of the node, then this is the same as the `client{Width,Height}` (the size of the node without its borders). If the content would overflow the node, then it would be the size of the content that would be scrollable (in other words, what would "move" when you scrolled). If the element isn't displayed or it has display: inline, it return 0 in both cases. These APIs provide rounded integers. NOTE: The current implementation of `ScrollView` has several known bugs and inconsistencies across platforms (Android vs. iOS) and architectures (Paper vs. Fabric) (e.g.: content showing on top of the border on Android, `overflow: visible` only working on Android but not on iOS, etc.). The data that this API reports is the one that aligns with the Web (with a few limitations), and we'll need to fix the implementation to align with this. NOTE: transforms are not considered correctly for the sake of this API, but also not applied correctly in any of the native platforms. On Web, the scrollable area is the overflow of all the children **with transforms applied** which isn't honored in RN. We''ll fix the data reported by this API when we also fix the user perceived behavior. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D49058368 fbshipit-source-id: 03f1875e66dbdaafb51291888f5fdf551692543e
Summary: Pull Request resolved: facebook#39328 This adds a new method in Fabric to get the scroll size for an element, and uses it to implement `scrollWidth` and `scrollHeight` as defined in react-native-community/discussions-and-proposals#607 Scroll size determine how much of the content of a node would move if the node was scrollable. If the content does not overflow the padding box of the node, then this is the same as the `client{Width,Height}` (the size of the node without its borders). If the content would overflow the node, then it would be the size of the content that would be scrollable (in other words, what would "move" when you scrolled). If the element isn't displayed or it has display: inline, it return 0 in both cases. These APIs provide rounded integers. NOTE: The current implementation of `ScrollView` has several known bugs and inconsistencies across platforms (Android vs. iOS) and architectures (Paper vs. Fabric) (e.g.: content showing on top of the border on Android, `overflow: visible` only working on Android but not on iOS, etc.). The data that this API reports is the one that aligns with the Web (with a few limitations), and we'll need to fix the implementation to align with this. NOTE: transforms are not considered correctly for the sake of this API, but also not applied correctly in any of the native platforms. On Web, the scrollable area is the overflow of all the children **with transforms applied** which isn't honored in RN. We''ll fix the data reported by this API when we also fix the user perceived behavior. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D49058368 fbshipit-source-id: b97ad4e60e4385cdd75c3092d8eb7df685e16afc
Summary: Pull Request resolved: #39328 This adds a new method in Fabric to get the scroll size for an element, and uses it to implement `scrollWidth` and `scrollHeight` as defined in react-native-community/discussions-and-proposals#607 Scroll size determine how much of the content of a node would move if the node was scrollable. If the content does not overflow the padding box of the node, then this is the same as the `client{Width,Height}` (the size of the node without its borders). If the content would overflow the node, then it would be the size of the content that would be scrollable (in other words, what would "move" when you scrolled). If the element isn't displayed or it has display: inline, it return 0 in both cases. These APIs provide rounded integers. NOTE: The current implementation of `ScrollView` has several known bugs and inconsistencies across platforms (Android vs. iOS) and architectures (Paper vs. Fabric) (e.g.: content showing on top of the border on Android, `overflow: visible` only working on Android but not on iOS, etc.). The data that this API reports is the one that aligns with the Web (with a few limitations), and we'll need to fix the implementation to align with this. NOTE: transforms are not considered correctly for the sake of this API, but also not applied correctly in any of the native platforms. On Web, the scrollable area is the overflow of all the children **with transforms applied** which isn't honored in RN. We''ll fix the data reported by this API when we also fix the user perceived behavior. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D49058368 fbshipit-source-id: 39a10bf7bddec9afc54f46cc02284d601b6962f3
…derer (#26282) ## Summary I'm going to start implementing parts of this proposal react-native-community/discussions-and-proposals#607 As part of that implementation I'm going to refactor a few parts of the interface between React and React Native. One of the main problems we have right now is that we have private parts used by React and React Native in the public instance exported by refs. I want to properly separate that. I saw that a few methods to attach event handlers imperatively on refs were also exposing some things in the public instance (the `_eventListeners`). I checked and these methods are unused, so we can just clean them up instead of having to refactor them too. Adding support for imperative event listeners is in the roadmap after this proposal, and its implementation might differ after this refactor. This is essentially a manual revert of #23386. I'll submit more PRs after this for the rest of the refactor. ## How did you test this change? Existing jest tests. Will test a React sync internally at Meta. DiffTrain build for [d49e0e0be0941490fe709f80de137516ba4c0ee3](facebook/react@d49e0e0)
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Reviewed By: sammy-SC Differential Revision: D54903376
Summary: Changelog: [internal] This moves all the new methods that were added to implement the DOM traversal and layout APIs (as per this RFC: react-native-community/discussions-and-proposals#607) to a separate C++ native module to avoid bloating the UIManager interface, initialize lazily, provide automatic caching of methods, simplify the API for implementors, etc. Pull Request resolved: #43512 Reviewed By: sammy-SC Differential Revision: D54903376 fbshipit-source-id: 69daa84c886d1c65dbb0b223dbb7c9077502c6ad
## Summary This adds the ability to create public instances for text nodes in Fabric. The implementation for the public instances lives in React Native (as it does for host components after facebook#26437). The logic here just handles their lazy instantiation when requested via `getPublicInstanceFromInternalInstanceHandle`, which is called by Fabric with information coming from the shadow tree. It's important that the creation of public instances for text nodes is done lazily to avoid regressing memory usage when unused. Instances for text nodes are left intact if the public instance is never accessed. This is necessary to implement access to text nodes in React Native as explained in react-native-community/discussions-and-proposals#607 ## How did you test this change? Added unit tests (also fixed a test that was only testing the logic in a mock :S).
This is a proposal to allow users to access a representation of the native view hierarchy in React Native using DOM-compatible APIs.
A rendered version of the proposal can be read here