-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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
ReactRootView not wrapping to it's content (Android only) #38024
Comments
|
React Native Version 0.71.0 |
|
I switched using 0.71.11 but the same issue is shown.
and this the style:
As you can see the container it has a fixed size but on the native side is rendered completely full screen. |
I have captured also the screenshot of the view inspector to check the difference between the 2 versions of RN: This is the version with 0.70.6: https://drive.google.com/file/d/1lmMyN7kEKUi8rynhxqLFDJEY2Mw8LYXw/view?usp=sharing This is the version with 0.71.11: https://drive.google.com/file/d/1eL9zrsmEJ2xqpB5udeANdmAL_OeBz6Qb/view?usp=sharing |
@NickGerleman is this potentially related to: ? |
Very unlikely. That code is just adding some queries to an existing event handler, for firing an existing event, and doesn’t do any sort of mutation. |
Does anyhow have a clue what did break in 0.71.x? This is a big breaking change for us because we rely a lot on showing the RN JS views inside the android native view. |
#38473 is also related to this - we've added repro steps and images of the view trees there. The cherry pick 2b06a75 did simplify the view tree, but there's still an extra view group not flattened. This issue is breaking brownfield UX on Android from 0.71 onwards - please triage accordingly @cortinico @NickGerleman cc @rasaha91 |
I think this and the bottom sheet issue are unlikely to be related to a Yoga change. FYI @luluwu2032 in case this seems related to any changes during that time period (7/22-11/22) Meta may have made which impact Paper RootView. Much of this code is still shared between renderers, but I think a lot of Paper specific changes have been originating from OSS these days. @shivenmian we might be able to find potentially relevant commits by looking at commits to Android RootView code between the versions (commits between 7/22-11/22). This sort of change might happen adjacent to this layer, though I did not see a directly related change here. Might be worth searching through the change log. react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java Line 4 in 04ad34d
|
One other strategy if you have an easy local repro is to do a bisect of commits within that single release period. |
@NickGerleman potentially relevant: |
@NickGerleman could you provide some more instructions on what you mean? In our test app we can specify a version of react-native in package.json, but I don't think react-native produces a release for every commit? I might be misunderstanding what you are suggesting. |
Either:
|
So I ended up using the nightlies and determined the issue started showing up in the sept 2nd, 2022 nightly build. There is no sept 1st build, so I looked at all the commits on sept 1 and sept 2. I determined the issue must be a JS only issue since I never changed the native bits on my test app. Only two commits on those dates had JS changes: I suspect its the second commit that is problematic, as the only difference I can see in the react tree between a working case and a non working case is the addition of the devtools overlay. @tyao1 as FYI. Could you help look into this issue? |
As an update, I locally built 0.71-stable, reverting the commit for the devtools overlay and things start working. So I think it's fair to conclude that commit caused the regression. I'm not sure what the proper fix is, but I would like to propose reverting that commit in main and cherry-picking it to 0.71-stable and 0.72-stable for now. @NickGerleman @cortinico Does this seem like a reasonable approach? I'm not sure why this issue doesn't occur on iOS, and I can't really help investigate that since I don't have a mac setup. But the proper fix would probably involve understanding why android is behaving differently and fixing that. |
This line looks suspicious: c52df02#diff-2a40597b015e446bd9cb647f972aad51d3cdbf396e43b1f52fe35029f8152e32R151 New React Devtools overlay takes up space based on window dimensions, which is incorrect when the root view is smaller than the screen. Does this repro outside of DEV bundles? IIRC @hoxyq might have been looking at DevTools overlay more recently. |
Directly reverting the diff would introduce a fairly noticeable devx regression, and I think this change interacts with future changes. Forward fix is the right move imo, especially if there is a scoped enough fix to be backported. See reactwg/react-native-releases#78 for pick requests. |
Yes, two reasons for it:
If these are changes that introduce this regression, it should not reproduce on prod, because this overlay is gated by __DEV__ |
Re #1: This should be fairly easy to workaround. If you are getting a window-space coordinate from ‘measureInWindow’, you can do the same measurement on the overlay, and subtract the offsets, to get a local offset instead. Re #2: Does the same thing work in browser with the same structure and everything display: flex? Would love to log it if so. But otherwise, if a real height is needed, we could always fall back to onLayout or measurement of user provided content in the worse case. |
Is it possible there was a regression, and that hook does not return null when we aren't inspecting? Even if that is the case, it was working prior to that commit. I don't know whether the root cause of the issue was introduced with c52df02 but I am reasonably sure that the issue starts occurring with that commit. |
Yeah, you are right, this condition is not false even if React DevTools are closed, but this component still returns null. Oh, I think I got it, this is probably because Can you try validating it with removing this prop or setting it to |
I think the proper solution should ensure the UI doesn't resize even after enabling devtools or inspector. As such, collapsible shouldn't be a function of those. |
…e connected Summary: ## Changelog: [General][Fix] - Do not render React DevTools overlays unless they are connected Fixes facebook#38024. Reproducible example: https://github.com/rasaha91/rn-bottomsheet. - This is a temporary workaround to resolve described problem for DEV bundles without attached React DevTools. - Still, such problem will be present for DEV bundles with attached React DevTools, but this should be only for brownfield apps with a shrinked React Native window. Checking that DevTools hook is present is not enough to determine whether the DevTools are connected. These changes fix it. Short description of what's going on there: 1. When React Native root window is rendered inside some native view, which takes only portion of the screen (like the native bottom sheet in the reproducible example), DevtoolsOverlay / InspectorOverlay takes up space based on application window dimension, this results into resizing the hosting window on the native side. https://pxl.cl/357r3 2. Right way to fix this would be removing the usage of application window sizes, so that DevtoolsOverlay / InspectorOverlay will be allowed only to take React Native's window. 3. Unfortunately, just removing setting is not enough, we should also have at least 1 of 2 things: - `collapsable` prop should be set to `true` => View will be flattened - Remove [`flex: 1` style on both root and inner Views](https://github.com/facebook/react-native/blob/b28e3c16ed7cbc8b3ed3f26d91c58acb4bb28879/packages/react-native/Libraries/ReactNative/AppContainer.js#L145-L147), but this is breaking how LogBox works now. | {F1062478964} | {F1062492367} Differential Revision: D47954883 fbshipit-source-id: e63e56628605e1fc10f3fcc14bef3ca9094fe4e7
Hey, I've published a temporary workaround for this - #38727. Why such regression happened?Because this View is no longer flattened. React DevTools hook is present in DEV bundles and collapsable prop resolves to Proposed workaround should solve this problem. Specifically, when React DevTools are not attached (opened), so running dev bundles without React DevTools opened should work the same as production ones. I've tested it on https://github.com/rasaha91/rn-bottomsheet. @rasaha91 @shivenmian please verify that these changes solve your issue. What is not working wellBrief description of what's going on there:
Screen.Recording.2023-07-31.at.14.22.09.mov
For Q3, I am planning some work on improving this whole debugging overlay and this is one of the problems that I will try to solve, but this will take much more time. |
…e connected (facebook#38727) Summary: Pull Request resolved: facebook#38727 ## Changelog: [General][Fix] - Do not render React DevTools overlays unless they are connected Fixes facebook#38024. Reproducible example: https://github.com/rasaha91/rn-bottomsheet. - This is a temporary workaround to resolve described problem for DEV bundles without attached React DevTools. - Still, such problem will be present for DEV bundles with attached React DevTools, but this should be only for brownfield apps with a shrinked React Native window. Checking that DevTools hook is present is not enough to determine whether the DevTools are connected. These changes fix it. Short description of what's going on there: 1. When React Native root window is rendered inside some native view, which takes only portion of the screen (like the native bottom sheet in the reproducible example), DevtoolsOverlay / InspectorOverlay takes up space based on application window dimension, this results into resizing the hosting window on the native side. https://pxl.cl/357r3 2. Right way to fix this would be removing the usage of application window sizes, so that DevtoolsOverlay / InspectorOverlay will be allowed only to take React Native's window. 3. Unfortunately, just removing setting is not enough, we should also have at least 1 of 2 things: - `collapsable` prop should be set to `true` => View will be flattened - Remove [`flex: 1` style on both root and inner Views](https://github.com/facebook/react-native/blob/b28e3c16ed7cbc8b3ed3f26d91c58acb4bb28879/packages/react-native/Libraries/ReactNative/AppContainer.js#L145-L147), but this is breaking how LogBox works now. | {F1062478964} | {F1062492367} Reviewed By: NickGerleman Differential Revision: D47954883 fbshipit-source-id: 924ae5dff701e2011536c819f17982d2e6ebe3a1
@hoxyq apologies for the delay in testing, was sidetracked with a few critical tasks. I have tested this patch and it does fix the issue on RN72, and throws up the TraceUpdateOverlay error for RN71 as you mentioned. Please go ahead and merge this PR, and do patch this to RN71 as well :) Thanks so much again for this work, much appreciated! cc @kelset @fortmarek @cortinico @rasaha91 |
…e connected (facebook#38727) Summary: Pull Request resolved: facebook#38727 ## Changelog: [General][Fix] - Do not render React DevTools overlays unless they are connected Fixes facebook#38024. Reproducible example: https://github.com/rasaha91/rn-bottomsheet. - This is a temporary workaround to resolve described problem for DEV bundles without attached React DevTools. - Still, such problem will be present for DEV bundles with attached React DevTools, but this should be only for brownfield apps with a shrinked React Native window. Checking that DevTools hook is present is not enough to determine whether the DevTools are connected. These changes fix it. Short description of what's going on there: 1. When React Native root window is rendered inside some native view, which takes only portion of the screen (like the native bottom sheet in the reproducible example), DevtoolsOverlay / InspectorOverlay takes up space based on application window dimension, this results into resizing the hosting window on the native side. https://pxl.cl/357r3 2. Right way to fix this would be removing the usage of application window sizes, so that DevtoolsOverlay / InspectorOverlay will be allowed only to take React Native's window. 3. Unfortunately, just removing setting is not enough, we should also have at least 1 of 2 things: - `collapsable` prop should be set to `true` => View will be flattened - Remove [`flex: 1` style on both root and inner Views](https://github.com/facebook/react-native/blob/b28e3c16ed7cbc8b3ed3f26d91c58acb4bb28879/packages/react-native/Libraries/ReactNative/AppContainer.js#L145-L147), but this is breaking how LogBox works now. | {F1062478964} | {F1062492367} Reviewed By: NickGerleman Differential Revision: D47954883 fbshipit-source-id: 9c39543d9efb6090cfa6a3910f3e87f69ca6d5ea
…e connected (facebook#38727) Summary: Pull Request resolved: facebook#38727 ## Changelog: [General][Fix] - Do not render React DevTools overlays unless they are connected Fixes facebook#38024. Reproducible example: https://github.com/rasaha91/rn-bottomsheet. - This is a temporary workaround to resolve described problem for DEV bundles without attached React DevTools. - Still, such problem will be present for DEV bundles with attached React DevTools, but this should be only for brownfield apps with a shrinked React Native window. Checking that DevTools hook is present is not enough to determine whether the DevTools are connected. These changes fix it. Short description of what's going on there: 1. When React Native root window is rendered inside some native view, which takes only portion of the screen (like the native bottom sheet in the reproducible example), DevtoolsOverlay / InspectorOverlay takes up space based on application window dimension, this results into resizing the hosting window on the native side. https://pxl.cl/357r3 2. Right way to fix this would be removing the usage of application window sizes, so that DevtoolsOverlay / InspectorOverlay will be allowed only to take React Native's window. 3. Unfortunately, just removing setting is not enough, we should also have at least 1 of 2 things: - `collapsable` prop should be set to `true` => View will be flattened - Remove [`flex: 1` style on both root and inner Views](https://github.com/facebook/react-native/blob/b28e3c16ed7cbc8b3ed3f26d91c58acb4bb28879/packages/react-native/Libraries/ReactNative/AppContainer.js#L145-L147), but this is breaking how LogBox works now. | {F1062478964} | {F1062492367} Reviewed By: NickGerleman Differential Revision: D47954883 fbshipit-source-id: d45d8cb82daa8dc9de58f54c137815b3a7abd5db
…e connected (facebook#38727) Summary: Pull Request resolved: facebook#38727 ## Changelog: [General][Fix] - Do not render React DevTools overlays unless they are connected Fixes facebook#38024. Reproducible example: https://github.com/rasaha91/rn-bottomsheet. - This is a temporary workaround to resolve described problem for DEV bundles without attached React DevTools. - Still, such problem will be present for DEV bundles with attached React DevTools, but this should be only for brownfield apps with a shrinked React Native window. Checking that DevTools hook is present is not enough to determine whether the DevTools are connected. These changes fix it. Short description of what's going on there: 1. When React Native root window is rendered inside some native view, which takes only portion of the screen (like the native bottom sheet in the reproducible example), DevtoolsOverlay / InspectorOverlay takes up space based on application window dimension, this results into resizing the hosting window on the native side. https://pxl.cl/357r3 2. Right way to fix this would be removing the usage of application window sizes, so that DevtoolsOverlay / InspectorOverlay will be allowed only to take React Native's window. 3. Unfortunately, just removing setting is not enough, we should also have at least 1 of 2 things: - `collapsable` prop should be set to `true` => View will be flattened - Remove [`flex: 1` style on both root and inner Views](https://github.com/facebook/react-native/blob/b28e3c16ed7cbc8b3ed3f26d91c58acb4bb28879/packages/react-native/Libraries/ReactNative/AppContainer.js#L145-L147), but this is breaking how LogBox works now. | {F1062478964} | {F1062492367} Reviewed By: NickGerleman Differential Revision: D47954883 fbshipit-source-id: d45d8cb82daa8dc9de58f54c137815b3a7abd5db
As an FYI, we have integrated the fix, and it works... sort of. If you have something like flipper running in the background, then devtools overlay will be mounted and it won't be obvious why the UI is behaving differently. @hoxyq it sounds like this issue will go away with the work you are doing in Q3? |
Yes, I am planning to work on fixing it and hopefully this will be shipped in 0.74. |
Summary: # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Differential Revision: D50644900 fbshipit-source-id: fdb5c43a2b5c7447f964e3f6bf470af492b4e81a
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: c32e4d356372a9b75fde2ae2e78440b23095c990
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: 8f6c52a1d7d17ef2e9c69269ecdee133168583d7
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: c822fbdfd31988748a1c51a1753a622b0636cafb
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: 364f176cc0c2c175c5f50c58e326508a981a259d
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: 056ceba48404383e6fc0e4484f0243d6b7f01cd1
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: dfe606bee80d40ca7a7d945020712d43690c3386
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: c618d30db1eade945097882ac1d21c691ff12635
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: dee787a809d5140c93ad343380127e9cc6d2533a
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: 467fb2a7cb77413324d4f968f2929f7d2d0097d7
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: 9ec8a15370858daeec70d4511a82b0c1a6cf78bf
Summary: Pull Request resolved: facebook#41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - facebook#38024 Here is an explanation: facebook#38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: 277a19f3cf55e1113ce3522db4eae69274b28f7f
Summary: Pull Request resolved: #41750 # Changelog: [ANDROID] [FIXED] - fixed unexpected resizing of ReactRootView for dev bundles in brownfield apps There was an android-only issue related to brownfield apps, when React Native root view is inside a modal and resizes it - #38024 Here is an explanation: #38024 (comment) Reviewed By: NickGerleman, motiz88 Differential Revision: D50644900 fbshipit-source-id: 660cb2e6208e05b4eeee4fe2cde3406a4afa6c76
Description
We have some code in our app that creates a new ReactRootView and add's the view to an existing Android Native View.
Once we upgraded to version 0.71.x we noticed that the size of the ReactRootView is not resizing to it's content as it was doing in the versions before. Now the view is always shown to Fit the parent container. Is that intentional?
Is there a way to go back to the previous behaviour?
React Native Version
0.71.0
Output of
npx react-native info
info Fetching system and libraries information...
System:
OS: macOS 13.4.1
CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Memory: 131.39 MB / 32.00 GB
Shell: 3.2.57 - /bin/bash
Binaries:
Node: 18.13.0 - ~/.nvm/versions/node/v18.13.0/bin/node
Yarn: 1.22.19 - ~/.yarn/bin/yarn
npm: 8.19.3 - ~/.nvm/versions/node/v18.13.0/bin/npm
Watchman: 2022.10.17.00 - /usr/local/bin/watchman
Managers:
CocoaPods: 1.11.3 - /Users/ilber/.rvm/gems/ruby-2.6.5/bin/pod
SDKs:
iOS SDK:
Platforms: DriverKit 22.4, iOS 16.4, macOS 13.3, tvOS 16.4, watchOS 9.4
Android SDK:
API Levels: 21, 22, 27, 28, 31, 33, 33, 34
Build Tools: 28.0.3, 29.0.2, 30.0.2, 30.0.3, 32.0.0, 33.0.1, 33.0.2
Android NDK: Not Found
IDEs:
Android Studio: 2022.2 AI-222.4459.24.2221.10121639
Xcode: 14.3.1/14E300c - /usr/bin/xcodebuild
Languages:
Java: 11.0.2 - /Users/ilber/.sdkman/candidates/java/current/bin/javac
npmPackages:
@react-native-community/cli: Not Found
react: 18.2.0 => 18.2.0
react-native: 0.71.11 => 0.71.11
react-native-macos: Not Found
npmGlobalPackages:
react-native: Not Found
Steps to reproduce
val view = ReactRootView(currentActivity).apply {
startReactApplication(
reactInstanceManager,
"ModuleName",
"InitialProps"
)
layoutParams = ViewGroup.LayoutParams(WRAP_CONTENT, WRAP_CONTENT)
}
// add the view to the native stack.
Snack, code example, screenshot, or link to a repository
No
The text was updated successfully, but these errors were encountered: