Fix: layout shift on orientation change #3295
Open
Conversation
This file contains hidden or 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
kmichalikk
force-pushed
the
@kmichalikk/layout-shift-on-orientation-change
branch
from
3209320 to
44345e5
Compare
kmichalikk
force-pushed
the
@kmichalikk/layout-shift-on-orientation-change
branch
from
8eb4111 to
60ed155
Compare
kkafar
reviewed
Nov 19, 2025
Member
kkafar
left a comment
kkafar
left a comment
There was a problem hiding this comment.
Good job! I've left few remarks & questions. Please answer them.
common/cpp/react/renderer/components/rnscreens/RNSScreenComponentDescriptor.h
Outdated
Show resolved
Hide resolved
| void adopt(ShadowNode &shadowNode) const override { | ||
| react_native_assert(dynamic_cast<RNSScreenShadowNode *>(&shadowNode)); | ||
| auto &screenShadowNode = static_cast<RNSScreenShadowNode &>(shadowNode); | ||
| auto &screenShadowNode = dynamic_cast<RNSScreenShadowNode &>(shadowNode); |
Collaborator
Author
There was a problem hiding this comment.
My bad, changed it back to static_cast. After consulting, I also changed the cast style in RNSScreenProps
common/cpp/react/renderer/components/rnscreens/RNSScreenShadowNodeCommitHook.cpp
Outdated
Show resolved
Hide resolved
common/cpp/react/renderer/components/rnscreens/RNSScreenShadowNodeCommitHook.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
+30
to
+39
| if (contextContainer_) { | ||
| auto fabricUIManager = | ||
| contextContainer_ | ||
| ->at<jni::alias_ref<facebook::react::JFabricUIManager::javaobject>>( | ||
| "FabricUIManager"); | ||
| fabricUIManager->getBinding() | ||
| ->getScheduler() | ||
| ->getUIManager() | ||
| ->unregisterCommitHook(*this); | ||
| } |
Member
There was a problem hiding this comment.
We never cleanup reference to the contextContainer. Shall we do it here?
Collaborator
Author
There was a problem hiding this comment.
should we? doesn't the smart pointer handle this on its own?
common/cpp/react/renderer/components/rnscreens/RNSScreenShadowNodeCommitHook.cpp
Outdated
Show resolved
Hide resolved
common/cpp/react/renderer/components/rnscreens/RNSScreenShadowNodeCommitHook.cpp
Outdated
Show resolved
Hide resolved
| const ShadowTreeCommitOptions & /*commitOptions*/) noexcept override; | ||
|
|
||
| private: | ||
| std::shared_ptr<const ContextContainer> contextContainer_; |
Member
There was a problem hiding this comment.
Is the decision of retaining the context container here intentional?
Collaborator
Author
There was a problem hiding this comment.
Since the commit hook needs to unregister in the destructor, I need the ref to the container
common/cpp/react/renderer/components/rnscreens/RNSScreenShadowNodeCommitHook.h
Outdated
Show resolved
Hide resolved
common/cpp/react/renderer/components/rnscreens/RNSScreenShadowNodeCommitHook.h
Outdated
Show resolved
Hide resolved
Member
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.
Fixes #2933
Description
This PR fixes the bug on android that results in Screen orientation being calculated incorrectly for some time after the physical device orientation has changed. This bug seems to happen due to asynchronous nature of Screen state update emitted to react from native layer. The state contains a size that is set on the Screen frame.
The bug is more visible on slower devices, because with longer processing of consecutive commits, the delay is longer.
It can be noticed on a trivial application (some text + different background color for screen and content view) with some heavy computations artificially added to JS.
More detailed description:
In
RNSScreenComponentDescriptor, on Android specifically, we callsetSize()on the Screen view to hardcode its dimensions. This is needed to account for screen header size, of which React has no idea. It is however the core reason for the bug. The size which we set is outdated. Only after the asynchronous state update from native side is processed, it is corrected. This can take some time. If we comment the code for handling the header size, this problem disappears. But we cannot.Since the code is staying here, we need to work with it. We agreed on the special value of frameSize = { 0, 0 }, for which we set YogaNode size to Undefined. This forces Yoga to recalculate the layout. It can do so because the parent view is correctly sized at this point (no calls to
setSize()with outdated values). We cannot do this manually, because the descriptor doesn't have access to parent nodes. Using the commit hook, we detect the orientation change (it can compare old and new RootShadowNode, which have the correct dimensions) and set the frameSize for the ShadowNode which is passed to the descriptor afterwards.Another problem is that the hook detects the change only once, for one instance of the shadow node. There could be more instances processed before the state is corrected. Luckily, React uses the same state instance for each shadow node instance between the updates. We take advantage of that, once reset to 0, the frameSize stays that way until some other updates changes the instance. Since we only store the screen offset and frameSize in the state, this works good enough. But it's still pretty hacky.
Changes
Description above. A new feature flag is added. When set, we register a new commit hook that checks if screen orientation changed & sets frameSize to 0. It is detected in Screen descriptor and Yoga is forced to recalculate the layout.
Before / After
android-layout-bad.mov
android-layout-better.mov
Testing
Use Test2933. You can take a look at the "checkerboard" test from react-native-reanimated.