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

Implement unstable_getBoundingClientRect in RN Fabric refs #26137

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

rubennorte
Copy link
Contributor

We're fixing the timing of layout and passive effects in React Native, and adding support for some Web APIs so common use cases for those effects can be implemented with the same code on React and React Native.

Let's take this example:

function MyComponent(props) {
  const viewRef = useRef();

  useLayoutEffect(() => {
    const rect = viewRef.current?.getBoundingClientRect();
    console.log('My view is located at', rect?.toJSON());
  }, []);

  return <View ref={viewRef}>{props.children}</View>;
}

This could would work as expected on Web (ignoring the use of View and assuming something like div) but not on React Native because:

  1. Layout is done asynchronously in a background thread in parallel with the execution of layout and passive effects. This is incorrect and it's being fixed in React Native (see facebook/react-native@afec07a).
  2. We don't have an API to access layout information synchronously. The existing ref.current.measureInWindow uses callbacks to pass the result. That is asynchronous at the moment in Paper (the legacy renderer in React Native), but it's actually synchronous in Fabric (the new React Native renderer).

This fixes point 2) by adding a Web-compatible method to access layout information (on Fabric only).

This has 2 dependencies in React Native:

  1. Access to getBoundingClientRect in Fabric, which was added in https://github.com/facebook/react-native/blob/main/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp#L644-
    L676
  2. Access to DOMRect, which was added in facebook/react-native@673c761 .

As next step, I'll modify the implementation of this and other methods in Fabric to warn when they're accessed during render. We can't do this on Web because we can't (shouldn't) modify built-in DOM APIs, but we can do it in React Native because the refs objects are built by the framework.

@sammy-SC sammy-SC requested review from sammy-SC, yungsters and acdlite and removed request for sammy-SC February 9, 2023 18:40
@react-sizebot
Copy link

Comparing: 3ff1540...ee79b6c

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 154.82 kB 154.82 kB = 49.11 kB 49.11 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 156.82 kB 156.82 kB = 49.72 kB 49.72 kB
facebook-www/ReactDOM-prod.classic.js = 533.52 kB 533.52 kB = 94.98 kB 94.98 kB
facebook-www/ReactDOM-prod.modern.js = 518.79 kB 518.79 kB = 92.83 kB 92.83 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against ee79b6c

Comment on lines +212 to +217
expect(rect.toJSON()).toMatchObject({
x: 10,
y: 10,
width: 100,
height: 100,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that on web this is expected to have top/left/right/bottom values too: https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is only the mock I defined for the React tests because jsdom doesn't provide DOMRect. In React Native, the implementation is spec-compliant (see https://github.com/facebook/react-native/blob/main/Libraries/DOM/Geometry/DOMRect.js :) )

Copy link
Contributor

@sammy-SC sammy-SC left a comment

Choose a reason for hiding this comment

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

looks good! thanks for adding this.

@sammy-SC sammy-SC merged commit 53b1f69 into facebook:main Feb 9, 2023
@rubennorte rubennorte deleted the rn-get-bounding-client-rect branch February 9, 2023 18:52
github-actions bot pushed a commit that referenced this pull request Feb 9, 2023
We're fixing the timing of layout and passive effects in React Native,
and adding support for some Web APIs so common use cases for those
effects can be implemented with the same code on React and React Native.

Let's take this example:

```javascript
function MyComponent(props) {
  const viewRef = useRef();

  useLayoutEffect(() => {
    const rect = viewRef.current?.getBoundingClientRect();
    console.log('My view is located at', rect?.toJSON());
  }, []);

  return <View ref={viewRef}>{props.children}</View>;
}
```

This could would work as expected on Web (ignoring the use of `View` and
assuming something like `div`) but not on React Native because:
1. Layout is done asynchronously in a background thread in parallel with
the execution of layout and passive effects. This is incorrect and it's
being fixed in React Native (see
facebook/react-native@afec07a).
2. We don't have an API to access layout information synchronously. The
existing `ref.current.measureInWindow` uses callbacks to pass the
result. That is asynchronous at the moment in Paper (the legacy renderer
in React Native), but it's actually synchronous in Fabric (the new React
Native renderer).

This fixes point 2) by adding a Web-compatible method to access layout
information (on Fabric only).

This has 2 dependencies in React Native:
1. Access to `getBoundingClientRect` in Fabric, which was added in
https://github.com/facebook/react-native/blob/main/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp#L644-
L676
2. Access to `DOMRect`, which was added in
facebook/react-native@673c761
.

As next step, I'll modify the implementation of this and other methods
in Fabric to warn when they're accessed during render. We can't do this
on Web because we can't (shouldn't) modify built-in DOM APIs, but we can
do it in React Native because the refs objects are built by the
framework.

DiffTrain build for [53b1f69](53b1f69)
[View git log for this commit](https://github.com/facebook/react/commits/53b1f69ba6c08083e4934373d3f6f2e31b232874)
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 15, 2023
Summary:
This sync includes the following changes:
- **[86c8c8db7](facebook/react@86c8c8db7 )**: test: Don't retry flushActWork if flushUntilNextPaint threw ([#26121](facebook/react#26121)) //<Sebastian Silbermann>//
- **[64acd3918](facebook/react@64acd3918 )**: remove unguarded getRootNode call ([#26152](facebook/react#26152)) //<Josh Story>//
- **[71cace4d3](facebook/react@71cace4d3 )**: Migrate testRunner from jasmine2 to jest-circus ([#26144](facebook/react#26144)) //<Ming Ye>//
- **[c8510227c](facebook/react@c8510227c )**: Treat displayName as undefined ([#26148](facebook/react#26148)) //<Sebastian Markbåge>//
- **[55542bc73](facebook/react@55542bc73 )**: Update jest printBasicPrototype config ([#26142](facebook/react#26142)) //<Ming Ye>//
- **[6396b6641](facebook/react@6396b6641 )**: Model Float on Hoistables semantics ([#26106](facebook/react#26106)) //<Josh Story>//
- **[ef9f6e77b](facebook/react@ef9f6e77b )**: Enable passing Server References from Server to Client ([#26124](facebook/react#26124)) //<Sebastian Markbåge>//
- **[35698311d](facebook/react@35698311d )**: Update jest escapeString config ([#26140](facebook/react#26140)) //<Ming Ye>//
- **[6ddcbd4f9](facebook/react@6ddcbd4f9 )**: [flow] enable LTI inference mode ([#26104](facebook/react#26104)) //<Jan Kassens>//
- **[53b1f69ba](facebook/react@53b1f69ba )**: Implement unstable_getBoundingClientRect in RN Fabric refs ([#26137](facebook/react#26137)) //<Rubén Norte>//
- **[594093496](facebook/react@594093496 )**: Update to Jest 29 ([#26088](facebook/react#26088)) //<Ming Ye>//
- **[28fcae062](facebook/react@28fcae062 )**: Add support for SVG `transformOrigin` prop ([#26130](facebook/react#26130)) //<Aravind D>//
- **[3ff1540e9](facebook/react@3ff1540e9 )**: Prefer JSX in ReactNoop assertions (to combat out-of-memory test runs) ([#26127](facebook/react#26127)) //<Sebastian Silbermann>//
- **[01a0c4e12](facebook/react@01a0c4e12 )**: Add Edge Server Builds for workerd / edge-light ([#26116](facebook/react#26116)) //<Sebastian Markbåge>//
- **[f0cf832e1](facebook/react@f0cf832e1 )**: Update Flight Fixture to "use client" instead of .client.js ([#26118](facebook/react#26118)) //<Sebastian Markbåge>//
- **[03a216070](facebook/react@03a216070 )**: Rename "dom" fork to "dom-node" and "bun" fork to "dom-bun" ([#26117](facebook/react#26117)) //<Sebastian Markbåge>//
- **[4bf2113a1](facebook/react@4bf2113a1 )**: Revert "Move the Webpack manifest config to one level deeper ([#26083](facebook/react#26083))"  ([#26111](facebook/react#26111)) //<Sebastian Markbåge>//
- **[2ef24145e](facebook/react@2ef24145e )**: [flow] upgrade to 0.199.0 ([#26096](facebook/react#26096)) //<Jan Kassens>//
- **[922dd7ba5](facebook/react@922dd7ba5 )**: Revert the outer module object to an object ([#26093](facebook/react#26093)) //<Sebastian Markbåge>//
- **[9d111ffdf](facebook/react@9d111ffdf )**: Serialize Promises through Flight ([#26086](facebook/react#26086)) //<Sebastian Markbåge>//
- **[0ba4698c7](facebook/react@0ba4698c7 )**: Fix async test in React reconciler ([#26087](facebook/react#26087)) //<Ming Ye>//
- **[8c234c0de](facebook/react@8c234c0de )**: Move the Webpack manifest config to one level deeper ([#26083](facebook/react#26083)) //<Sebastian Markbåge>//
- **[977bccd24](facebook/react@977bccd24 )**: Refactor Flight Encoding ([#26082](facebook/react#26082)) //<Sebastian Markbåge>//
- **[d7bb524ad](facebook/react@d7bb524ad )**: [cleanup] Remove unused package jest-mock-scheduler ([#26084](facebook/react#26084)) //<Ming Ye>//
- **[6b3083266](facebook/react@6b3083266 )**: Upgrade prettier ([#26081](facebook/react#26081)) //<Jan Kassens>//
- **[1f5ce59dd](facebook/react@1f5ce59dd )**: [cleanup] fully roll out warnAboutSpreadingKeyToJSX ([#26080](facebook/react#26080)) //<Jan Kassens>//

Changelog:
[General][Changed] - React Native sync for revisions 48b687f...fccf3a9

jest_e2e[run_all_tests]

Reviewed By: rubennorte

Differential Revision: D43305607

fbshipit-source-id: 8da7567ca2a182f4be27788935c2da30a731f83b
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[86c8c8db7](facebook/react@86c8c8db7 )**: test: Don't retry flushActWork if flushUntilNextPaint threw ([facebook#26121](facebook/react#26121)) //<Sebastian Silbermann>//
- **[64acd3918](facebook/react@64acd3918 )**: remove unguarded getRootNode call ([facebook#26152](facebook/react#26152)) //<Josh Story>//
- **[71cace4d3](facebook/react@71cace4d3 )**: Migrate testRunner from jasmine2 to jest-circus ([facebook#26144](facebook/react#26144)) //<Ming Ye>//
- **[c8510227c](facebook/react@c8510227c )**: Treat displayName as undefined ([facebook#26148](facebook/react#26148)) //<Sebastian Markbåge>//
- **[55542bc73](facebook/react@55542bc73 )**: Update jest printBasicPrototype config ([facebook#26142](facebook/react#26142)) //<Ming Ye>//
- **[6396b6641](facebook/react@6396b6641 )**: Model Float on Hoistables semantics ([facebook#26106](facebook/react#26106)) //<Josh Story>//
- **[ef9f6e77b](facebook/react@ef9f6e77b )**: Enable passing Server References from Server to Client ([facebook#26124](facebook/react#26124)) //<Sebastian Markbåge>//
- **[35698311d](facebook/react@35698311d )**: Update jest escapeString config ([facebook#26140](facebook/react#26140)) //<Ming Ye>//
- **[6ddcbd4f9](facebook/react@6ddcbd4f9 )**: [flow] enable LTI inference mode ([facebook#26104](facebook/react#26104)) //<Jan Kassens>//
- **[53b1f69ba](facebook/react@53b1f69ba )**: Implement unstable_getBoundingClientRect in RN Fabric refs ([facebook#26137](facebook/react#26137)) //<Rubén Norte>//
- **[594093496](facebook/react@594093496 )**: Update to Jest 29 ([facebook#26088](facebook/react#26088)) //<Ming Ye>//
- **[28fcae062](facebook/react@28fcae062 )**: Add support for SVG `transformOrigin` prop ([facebook#26130](facebook/react#26130)) //<Aravind D>//
- **[3ff1540e9](facebook/react@3ff1540e9 )**: Prefer JSX in ReactNoop assertions (to combat out-of-memory test runs) ([facebook#26127](facebook/react#26127)) //<Sebastian Silbermann>//
- **[01a0c4e12](facebook/react@01a0c4e12 )**: Add Edge Server Builds for workerd / edge-light ([facebook#26116](facebook/react#26116)) //<Sebastian Markbåge>//
- **[f0cf832e1](facebook/react@f0cf832e1 )**: Update Flight Fixture to "use client" instead of .client.js ([facebook#26118](facebook/react#26118)) //<Sebastian Markbåge>//
- **[03a216070](facebook/react@03a216070 )**: Rename "dom" fork to "dom-node" and "bun" fork to "dom-bun" ([facebook#26117](facebook/react#26117)) //<Sebastian Markbåge>//
- **[4bf2113a1](facebook/react@4bf2113a1 )**: Revert "Move the Webpack manifest config to one level deeper ([facebook#26083](facebook/react#26083))"  ([facebook#26111](facebook/react#26111)) //<Sebastian Markbåge>//
- **[2ef24145e](facebook/react@2ef24145e )**: [flow] upgrade to 0.199.0 ([facebook#26096](facebook/react#26096)) //<Jan Kassens>//
- **[922dd7ba5](facebook/react@922dd7ba5 )**: Revert the outer module object to an object ([facebook#26093](facebook/react#26093)) //<Sebastian Markbåge>//
- **[9d111ffdf](facebook/react@9d111ffdf )**: Serialize Promises through Flight ([facebook#26086](facebook/react#26086)) //<Sebastian Markbåge>//
- **[0ba4698c7](facebook/react@0ba4698c7 )**: Fix async test in React reconciler ([facebook#26087](facebook/react#26087)) //<Ming Ye>//
- **[8c234c0de](facebook/react@8c234c0de )**: Move the Webpack manifest config to one level deeper ([facebook#26083](facebook/react#26083)) //<Sebastian Markbåge>//
- **[977bccd24](facebook/react@977bccd24 )**: Refactor Flight Encoding ([facebook#26082](facebook/react#26082)) //<Sebastian Markbåge>//
- **[d7bb524ad](facebook/react@d7bb524ad )**: [cleanup] Remove unused package jest-mock-scheduler ([facebook#26084](facebook/react#26084)) //<Ming Ye>//
- **[6b3083266](facebook/react@6b3083266 )**: Upgrade prettier ([facebook#26081](facebook/react#26081)) //<Jan Kassens>//
- **[1f5ce59dd](facebook/react@1f5ce59dd )**: [cleanup] fully roll out warnAboutSpreadingKeyToJSX ([facebook#26080](facebook/react#26080)) //<Jan Kassens>//

Changelog:
[General][Changed] - React Native sync for revisions 48b687f...fccf3a9

jest_e2e[run_all_tests]

Reviewed By: rubennorte

Differential Revision: D43305607

fbshipit-source-id: 8da7567ca2a182f4be27788935c2da30a731f83b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants