-
-
Notifications
You must be signed in to change notification settings - Fork 507
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
fix(Android, Fabric): jumping content with native header #2169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Could not get syntax highlighting working with implementation being split to separate .cpp file... Decided navigating through code is more important than this separation for now.
... do not unnecessarily pass context to screen view managers When WEAK_THIS has been initialized in `init` block it did not work (jni function calls were failing) -- too early.
kkafar
commented
Jun 8, 2024
1 task
WoLewicki
approved these changes
Jun 18, 2024
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! Just answer all the comments and apply changes if needed before merging.
1 task
kkafar
added a commit
that referenced
this pull request
Jun 21, 2024
…text (#2199) ## Description My recent PR: * #2169 introduced creation of dummy layout, which requires react context to be attached to activity, as we need access to the activity. Unfortunately when running on Paper the context is not attached to the activity yet, resulting in exception being thrown. <details> <summary>Exception</summary> ``` Failed to create NativeModule 'UIManager' java.lang.IllegalArgumentException: [RNScreens] Attempt to use context detached from activity at com.swmansion.rnscreens.utils.ScreenDummyLayoutHelper.ensureDummyLayoutWithHeader(ScreenDummyLayoutHelper.kt:68) at com.swmansion.rnscreens.utils.ScreenDummyLayoutHelper.<init>(ScreenDummyLayoutHelper.kt:53) at com.swmansion.rnscreens.RNScreensPackage.createViewManagers(RNScreensPackage.kt:28) at com.facebook.react.ReactInstanceManager.getOrCreateViewManagers(ReactInstanceManager.java:933) at com.swmansion.reanimated.ReanimatedPackage.createUIManager(ReanimatedPackage.java:78) at com.swmansion.reanimated.ReanimatedPackage.getModule(ReanimatedPackage.java:38) at com.facebook.react.BaseReactPackage$ModuleHolderProvider.get(BaseReactPackage.java:156) at com.facebook.react.BaseReactPackage$ModuleHolderProvider.get(BaseReactPackage.java:144) at com.facebook.react.bridge.ModuleHolder.create(ModuleHolder.java:186) at com.facebook.react.bridge.ModuleHolder.getModule(ModuleHolder.java:151) at com.facebook.react.bridge.NativeModuleRegistry.getModule(NativeModuleRegistry.java:148) at com.facebook.react.bridge.CatalystInstanceImpl.getNativeModule(CatalystInstanceImpl.java:469) at com.facebook.react.bridge.CatalystInstanceImpl.getNativeModule(CatalystInstanceImpl.java:445) at com.facebook.react.uimanager.UIManagerHelper.getUIManager(UIManagerHelper.java:88) at com.facebook.react.uimanager.UIManagerHelper.getUIManager(UIManagerHelper.java:46) at com.facebook.react.ReactInstanceManager.attachRootViewToInstance(ReactInstanceManager.java:1231) at com.facebook.react.ReactInstanceManager.setupReactContext(ReactInstanceManager.java:1180) at com.facebook.react.ReactInstanceManager.lambda$runCreateReactContextOnNewThread$1(ReactInstanceManager.java:1143) at com.facebook.react.ReactInstanceManager.$r8$lambda$FD-H2RG7CdgXPtYJUBikxLbd8MA(Unknown Source:0) at com.facebook.react.ReactInstanceManager$$ExternalSyntheticLambda4.run(Unknown Source:4) at android.os.Handler.handleCallback(Handler.java:958) at android.os.Handler.dispatchMessage(Handler.java:99) at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:27) at android.os.Looper.loopOnce(Looper.java:205) at android.os.Looper.loop(Looper.java:294) at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:233) ``` </details> I'll need to sort that out when working on fix for jumping content on Android + Paper combination, however right now it is more important for examples to work correctly. ## Changes Creating `ScreenDummyLayoutHelper` now only when running on new architecture. ## Test code and steps to reproduce Run `TestsExample` w/o this change, you will see the exception being thrown -> resulting in freeze on whitescreen. With this change the example runs normally. ## Checklist - [x] Ensured that CI passes
alduzy
pushed a commit
that referenced
this pull request
Jun 28, 2024
## Description Just noticed while working on #2169 that we got a warning in CustomToolbar. Edit: followed review suggestions and suppressed lints for all our view, where this was requried. ## Changes Suppressed lint on missing constructors. We're safe to miss these there, as this view is constructed only programatically (we do not inflate any of our views). ## Test code and steps to reproduce N/A ## Checklist - [ ] Ensured that CI passes
alduzy
pushed a commit
that referenced
this pull request
Jun 28, 2024
## Description This PR intents to solve the *eternal* issue with "jumping content" on Android & Fabric. The issue itself is present on every platform / RN architecture combination, however this PR scope is to solve situation only on Android + Fabric. Android + Paper, and iOS will be solved in separate PRs. > [!note] > These videos are recorded with `topInsetEnabled: false`, as this prop implementation causes another series of issues that will be handled separately. Here is before & after comparison (best way to see is to go frame-by-frame) | Before | After | |--------|--------| | <video width="320" height="240" controls src="https://github.com/software-mansion/react-native-screens/assets/50801299/e1e995b5-885b-4bd4-941a-57cdc6b321b2"></video> | <video width="320" height="240" controls src="https://github.com/software-mansion/react-native-screens/assets/50801299/6ca87888-0f05-4dfc-b6db-bfd08e3735b3"></video> | This will even work with irregular font sizes! ### Short issue genesis > [!note] > The flow described here below is a simplification, but should give you a good grasp on the issue. Basically during the very first Yoga layout, that happens on secondary thread, React layout mechanism has no knowledge of the header size (there isn't even a node representing the header at appropriate tree-level present in shadow tree), thus the `Screen` content is layouted with more available space that it has in reality. These dimensions are then send to UI thread, and propagated bottom-up (children before parents) and `Screen` contents do receive too high frame. Then, when `ScreenContainer` / `ScreenStack` does receive its frame from RN, it triggers a fragmentary pass of native layout using `CoordinatorLayout` (the layout stops at `Screen`), offsetting the `Screen` by just-computed-header-height on Y axis, and in consequence pushing some of the `Screen`'s contents off the screen (exactly a strip of header height). The situation is then salvaged by sending state update from `Screen` to React Native with new frame size. ### Implemented solution During the first Yoga layout pass (when there is not state from UI thread received yet) we utilise the fact that RN clones our ShadowNode & calls `adapt` on it. In the adapt method we call into JVM where we have set up a dummy view hierarchy with coordinator layout & header, we layout it & return result to C++ layer where we set bottom padding on the Screen so that its contents do have right amount of space. > [!important] > Downside of this solution is the fact, that the Yoga state / Shadow Tree still indicates that the `Screen`'s origin is at `(x, y) = (0, 0)` and it still will have wrong dimensions. Setting dummy dimension on `HeaderConfig` shadow node will improve situation only slightly, as the `Screen` will still have wrong origin, but it will have appropriate size immediately, **hence `Screen`'s state update might not trigger follow-up transaction**. Thus I'm thinking now that I will update the solution. ### Yet ~un~tested approaches * ~I want to try making custom descriptor for `ScreenStack`, and try to customise shadownode's layout method.~ <- tested this & I believe this will not work due to the fact, that `ShadowNode.layout` does not use `layoutContext.viewportOffset` at all (so we can not use this to offset our descendants). At the same time the `layout` method does not propagate layout metrics - they are extracted for each shadow node directly from it's yoga node and this process does not take into consideration parent's layout metrics. **However, if the `x, y` view origin coordinates determined by yoga are in parent node coordinate system** we can use `HeaderConfig` to ensure appropriate `Screens` size and at the same time set `frame.y` manually! ## Test code and steps to reproduce I've added `TestHeader` test case. It's best to run it with `FabricTestExample`, record it, and then see frame-by-frame that the content no longer jumps. ## Checklist - [x] Included code example that can be used to test this change - [ ] Ensured that CI passes
alduzy
pushed a commit
that referenced
this pull request
Jun 28, 2024
…text (#2199) ## Description My recent PR: * #2169 introduced creation of dummy layout, which requires react context to be attached to activity, as we need access to the activity. Unfortunately when running on Paper the context is not attached to the activity yet, resulting in exception being thrown. <details> <summary>Exception</summary> ``` Failed to create NativeModule 'UIManager' java.lang.IllegalArgumentException: [RNScreens] Attempt to use context detached from activity at com.swmansion.rnscreens.utils.ScreenDummyLayoutHelper.ensureDummyLayoutWithHeader(ScreenDummyLayoutHelper.kt:68) at com.swmansion.rnscreens.utils.ScreenDummyLayoutHelper.<init>(ScreenDummyLayoutHelper.kt:53) at com.swmansion.rnscreens.RNScreensPackage.createViewManagers(RNScreensPackage.kt:28) at com.facebook.react.ReactInstanceManager.getOrCreateViewManagers(ReactInstanceManager.java:933) at com.swmansion.reanimated.ReanimatedPackage.createUIManager(ReanimatedPackage.java:78) at com.swmansion.reanimated.ReanimatedPackage.getModule(ReanimatedPackage.java:38) at com.facebook.react.BaseReactPackage$ModuleHolderProvider.get(BaseReactPackage.java:156) at com.facebook.react.BaseReactPackage$ModuleHolderProvider.get(BaseReactPackage.java:144) at com.facebook.react.bridge.ModuleHolder.create(ModuleHolder.java:186) at com.facebook.react.bridge.ModuleHolder.getModule(ModuleHolder.java:151) at com.facebook.react.bridge.NativeModuleRegistry.getModule(NativeModuleRegistry.java:148) at com.facebook.react.bridge.CatalystInstanceImpl.getNativeModule(CatalystInstanceImpl.java:469) at com.facebook.react.bridge.CatalystInstanceImpl.getNativeModule(CatalystInstanceImpl.java:445) at com.facebook.react.uimanager.UIManagerHelper.getUIManager(UIManagerHelper.java:88) at com.facebook.react.uimanager.UIManagerHelper.getUIManager(UIManagerHelper.java:46) at com.facebook.react.ReactInstanceManager.attachRootViewToInstance(ReactInstanceManager.java:1231) at com.facebook.react.ReactInstanceManager.setupReactContext(ReactInstanceManager.java:1180) at com.facebook.react.ReactInstanceManager.lambda$runCreateReactContextOnNewThread$1(ReactInstanceManager.java:1143) at com.facebook.react.ReactInstanceManager.$r8$lambda$FD-H2RG7CdgXPtYJUBikxLbd8MA(Unknown Source:0) at com.facebook.react.ReactInstanceManager$$ExternalSyntheticLambda4.run(Unknown Source:4) at android.os.Handler.handleCallback(Handler.java:958) at android.os.Handler.dispatchMessage(Handler.java:99) at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:27) at android.os.Looper.loopOnce(Looper.java:205) at android.os.Looper.loop(Looper.java:294) at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:233) ``` </details> I'll need to sort that out when working on fix for jumping content on Android + Paper combination, however right now it is more important for examples to work correctly. ## Changes Creating `ScreenDummyLayoutHelper` now only when running on new architecture. ## Test code and steps to reproduce Run `TestsExample` w/o this change, you will see the exception being thrown -> resulting in freeze on whitescreen. With this change the example runs normally. ## Checklist - [x] Ensured that CI passes
2 tasks
8 tasks
This was referenced Aug 23, 2024
kkafar
added a commit
that referenced
this pull request
Aug 27, 2024
## Description This PR applies modifications to a previous fix: #2169 for fabric only, which has stopped working since RN `0.75`. In RN `0.74` the `adopt` in `RNSScreenComponentDescriptior.h` was once called without `stateData` but with children and we could then check if the `ScreenStackHeaderConfig` is present among them and make adjustments based on it. When working on #2292 it became clear that the fix does not work anymore. Now the `adopt` is called either with no children and no `stateData` or with both. The solution is to move the code to `appendChild` in `RNSScreenShadowNode.cpp` so we can perform the adjustments as soon as the children append. ## Changes - moved code from `adopt` in `RNSScreenComponentDescriptior.h` to newly added `appendChild` override in `RNSScreenShadowNode.cpp` ## Screenshots / GIFs ### Before https://github.com/user-attachments/assets/6b76864b-58bb-4c6e-9f5b-a01bb0c88d2a ### After https://github.com/user-attachments/assets/98931e77-3877-4f67-8b28-f49d2e0f42ff ## Test code and steps to reproduce - Use `TestHeader` to test this change ## Checklist - [ ] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
kkafar
added a commit
that referenced
this pull request
Aug 27, 2024
…ement (#2292) ## Description > [!important] This PR aims to fix only pressables on screen components. This PR does not fix similar pressable issue with pressables in native header. That interaction will be fixed separately. Pressable elements work just fine until there's a gesture involved. On sensitive physical devices even a little movement during the press is treated as a gesture. When the `Pressable` element detects a gesture it calls [onResponderMove](https://github.com/facebook/react-native/blob/82795715aefba07ae9d79278ce3fd4d2e9a928f2/packages/react-native/Libraries/Pressability/Pressability.js#L484) which then checks wether the gesture happened within the element or went outside by comparing the touch coordinates with coordinates of the element using `_isTouchWithinResponderRegion`. The `responderRegion` is obtained from `_responderID` and happens to have unexpected values when the native header is present. It tuns out that the Y origin is slightly off. After some further investigation and comparison of coordinates it turned out that the height of the android status bar is not well calculated in various scenarios: <table> <td> `statusBarHidden: true` </td> <td> `statusBarTranslucent: true` </td> <td> `statusBarTranslucent: false` </td> </tr> <tr> <td> ![Screenshot_1723212300](https://github.com/user-attachments/assets/57e2f4a3-b002-4ca3-9519-45cfece860c4) </td> <td> ![Screenshot_1723212331](https://github.com/user-attachments/assets/bd46c8d1-8813-4fae-a8a9-0326193371d2) </td> <td> ![Screenshot_1723212382](https://github.com/user-attachments/assets/c7373437-524d-4a0f-951e-ce2689a4fe5c) </td> </tr> </table> The `calculateHeaderHeight` used for calculating the header and statusBar height seems to be the problem. Luckily, we don't have to calculate it by ourselves anymore, because the correct `t` value is provided in the `onLayout` function of the `Screen`. Thus we can get rid of the custom function. Another issue found: after navigating to another screen the offset is off again (exactly by 2x). It's caused by changes introduced in [this PR](#2169), which was supposed to prevent content jumps, but doesn't work since RN `0.75` sadly. ![Screenshot_1723220034](https://github.com/user-attachments/assets/b0908c23-4667-4ccf-8e5e-5e7e11bca316) I found out that `FrameOriginCorrection` is not being unset when dimensions from JVM are received, while the `FrameHeightCorrection` is. After adding the missing unset for `FrameOriginCorrection` I rolled back to the commit with the mentioned PR merged and RN `0.74` and I can confirm it works. Fixes #1975 ## Changes - removed `calculateHeaderHeight` function - added unset for `FrameOriginCorrection` when dimensions from JVM are received - added `Test1975.tsx` repro - moved code responsible for determining header height during the very first render from component descriptor's `adopt` method to shadow node `appendChild`. ## Test code and steps to reproduce `TestHeader`, `Test1975` ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes --------- Co-authored-by: alduzy <alduzy@gmail.com> Co-authored-by: Alex Duży <91994767+alduzy@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR intents to solve the eternal issue with "jumping content" on Android & Fabric. The issue itself is present on every platform / RN architecture combination, however this PR scope is to solve situation only on Android + Fabric. Android + Paper, and iOS will be solved in separate PRs.
Note
These videos are recorded with
topInsetEnabled: false
, as this prop implementation causes another series of issues that will be handled separately.Here is before & after comparison (best way to see is to go frame-by-frame)
before-topinsetenabled-false.mov
after-topinsetenabled-false.mov
This will even work with irregular font sizes!
Short issue genesis
Note
The flow described here below is a simplification, but should give you a good grasp on the issue.
Basically during the very first Yoga layout, that happens on secondary thread, React layout mechanism has no knowledge of the header size (there isn't even a node representing the header at appropriate tree-level present in shadow tree), thus the
Screen
content is layouted with more available space that it has in reality. These dimensions are then send to UI thread, and propagated bottom-up (children before parents) andScreen
contents do receive too high frame. Then, whenScreenContainer
/ScreenStack
does receive its frame from RN, it triggers a fragmentary pass of native layout usingCoordinatorLayout
(the layout stops atScreen
), offsetting theScreen
by just-computed-header-height on Y axis, and in consequence pushing some of theScreen
's contents off the screen (exactly a strip of header height). The situation is then salvaged by sending state update fromScreen
to React Native with new frame size.Implemented solution
During the first Yoga layout pass (when there is not state from UI thread received yet) we utilise the fact that RN clones our ShadowNode & calls
adapt
on it. In the adapt method we call into JVM where we have set up a dummy view hierarchy with coordinator layout & header, we layout it & return result to C++ layer where we set bottom padding on the Screen so that its contents do have right amount of space.Important
Downside of this solution is the fact, that the Yoga state / Shadow Tree still indicates that the
Screen
's origin is at(x, y) = (0, 0)
and it still will have wrong dimensions.Setting dummy dimension on
HeaderConfig
shadow node will improve situation only slightly, as theScreen
will still have wrong origin, but it will have appropriate size immediately, henceScreen
's state update might not trigger follow-up transaction. Thus I'm thinking now that I will update the solution.Yet
untested approachesI want to try making custom descriptor for<- tested this & I believe this will not work due to the fact, thatScreenStack
, and try to customise shadownode's layout method.ShadowNode.layout
does not uselayoutContext.viewportOffset
at all (so we can not use this to offset our descendants). At the same time thelayout
method does not propagate layout metrics - they are extracted for each shadow node directly from it's yoga node and this process does not take into consideration parent's layout metrics. However, if thex, y
view origin coordinates determined by yoga are in parent node coordinate system we can useHeaderConfig
to ensure appropriateScreens
size and at the same time setframe.y
manually!Test code and steps to reproduce
I've added
TestHeader
test case. It's best to run it withFabricTestExample
, record it, and then see frame-by-frame that the content no longer jumps.Checklist