Skip to content

Commit

Permalink
fix(Android,Fabric): prevent another infinite state update loop for h…
Browse files Browse the repository at this point in the history
…eader subviews with zero size (#2696)

## Description

Closes #2675

### Error mechanism

This one was tricky to figure out.

Few concurrent facts first:

1. When `statusBarTranslucent` is `true` we apply `paddingTop` to native
`Toolbar` to heighten the header.
2. Since 4.6.0 we update frames of header subviews in ShadowTree to the
values from HostTree, everytime the frame changes in HostTree.
3. We had a check in header subview shadow node that would prevent node
size update if the frame was (0, 0).

Header subview that represents the search bar has no content. Therefore
native layout sets it frame to exactly (0, 0), but with origin `(x,
toolbar.paddingTop)`.
This frame - `((x, toolbar.paddingTop), (0, 0))` is send to shadow node,
**where it is ignored**, but the layout is triggered by subsequent
*header config* update.
Therefore Yoga resolves height of the subview to full height of the
parent (usually `154 px = 40 dip`). New layout metrics are sent to
HostTree, where next native toolbar layout
determines toolbar size to be its content height + padding == subview
height + paddingTop. This gets send to ShadowTree, then back to
HostTree, native layout is triggered and another
`paddingTop` value is added to the overall header height, and so on. 

This is a regression introduced in 4.6.0 and limited to Fabric. 

The fix is simple - accept the (0, 0) size for the subview in
ShadowTree. We just need to use more reasonable value to denote the
uninitialized frame - `{-1, -1}` seems like a better choice as
it is an invalid frame.

## Changes

- **Use different initial value in state so that we can unambiguously
distinguish between un- and initialized state**

## Test code and steps to reproduce

`Test2675`

Tested on all combinations:

* Android SDK 34 (w/o edge-to-edge) and 35 (w/ edge-to-edge enabled),
* `statusBarTranslucent: true` and `false`,
* search bar present, not present
* other header elements present / not present.

Also tested iOS on the same example, because the code changes affect
shared C++ code.


## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes
  • Loading branch information
kkafar authored Feb 17, 2025
1 parent 8de5d70 commit 31c7035
Show file tree
Hide file tree
Showing 7 changed files with 142 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ abstract class FabricEnabledHeaderSubviewViewGroup(

private var lastWidth = 0f
private var lastHeight = 0f
private var lastOffsetX = 0f
private var lastOffsetY = 0f

fun setStateWrapper(wrapper: StateWrapper?) {
mStateWrapper = wrapper
Expand Down Expand Up @@ -45,13 +47,17 @@ abstract class FabricEnabledHeaderSubviewViewGroup(
// Check incoming state values. If they're already the correct value, return early to prevent
// infinite UpdateState/SetState loop.
if (abs(lastWidth - realWidth) < DELTA &&
abs(lastHeight - realHeight) < DELTA
abs(lastHeight - realHeight) < DELTA &&
abs(lastOffsetX - offsetXDip) < DELTA &&
abs(lastOffsetY - offsetYDip) < DELTA
) {
return
}

lastWidth = realWidth
lastHeight = realHeight
lastOffsetX = offsetXDip
lastOffsetY = offsetYDip

val map: WritableMap =
WritableNativeMap().apply {
Expand Down
121 changes: 121 additions & 0 deletions apps/src/tests/Test2675.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import { NavigationContainer } from '@react-navigation/native';
import { NativeStackNavigationProp, createNativeStackNavigator } from '@react-navigation/native-stack';
import React from 'react';
import { Button, StyleSheet, Text, View } from 'react-native';

type RouteParams = {
Home: undefined;
DynamicHeader: undefined;
}

type NavigationProps = {
navigation: NativeStackNavigationProp<RouteParams>;
}

const Stack = createNativeStackNavigator();

function HomeScreen({ navigation }: NavigationProps) {
return (
<View style={styles.container}>
<Text>Home Screen</Text>
<Button title="Navigate DynamicHeaderScreen" onPress={() => navigation.navigate('DynamicHeader')} />
</View>
);
}

function DynamicHeaderScreen({ navigation }: NavigationProps) {
React.useLayoutEffect(() => {
navigation.setOptions({
headerRight: HeaderRight,
});
}, [navigation]);

React.useEffect(() => {
const timerId = setTimeout(() => {
navigation.setOptions({
headerLeft: HeaderLeft,
headerRight: HeaderLeft,
});
}, 1300);

return () => {
clearTimeout(timerId);
};
}, [navigation]);

return (
<View style={styles.container}>
<Text>DynamicHeaderScreen</Text>
<Button title="Go back" onPress={() => navigation.popTo('Home')} />
</View>
);
}

function HeaderLeft() {
return (
<View>
<Text>Left</Text>
</View>
);
}

function HeaderTitle() {
return (
<View>
<Text>Title</Text>
</View>
);
}

function HeaderRight() {
return (
<View>
<Text>Right</Text>
</View>
);
}

export default function App() {
return (
<NavigationContainer>
<Stack.Navigator>
<Stack.Screen
name="Home"
component={HomeScreen}
options={{
statusBarTranslucent: true,
statusBarStyle: 'dark',
//headerTitle: HeaderTitle,
//headerRight: HeaderRight,
headerSearchBarOptions: {
placeholder: 'Search...',
onChangeText: (event) => {
console.log('Search text:', event.nativeEvent.text);
},
},
}}
/>
<Stack.Screen name="DynamicHeader" component={DynamicHeaderScreen} options={{
statusBarTranslucent: true,
statusBarStyle: 'dark',
headerTitle: HeaderTitle,
headerSearchBarOptions: {
placeholder: 'Search...',
onChangeText: (event) => {
console.log('Search text:', event.nativeEvent.text);
},
},
}} />
</Stack.Navigator>
</NavigationContainer>
);
}

const styles = StyleSheet.create({
container: {
flex: 1,
justifyContent: 'center',
alignItems: 'center',
},
});

1 change: 1 addition & 0 deletions apps/src/tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ export { default as Test2466 } from './Test2466';
export { default as Test2552 } from './Test2552';
export { default as Test2631 } from './Test2631';
export { default as Test2668 } from './Test2668';
export { default as Test2675 } from './Test2675';
export { default as TestScreenAnimation } from './TestScreenAnimation';
export { default as TestScreenAnimationV5 } from './TestScreenAnimationV5';
export { default as TestHeader } from './TestHeader';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ class RNSScreenStackHeaderConfigComponentDescriptor final
auto stateData = state->getData();

if (stateData.frameSize.width != 0 && stateData.frameSize.height != 0) {
layoutableShadowNode.setSize(
{stateData.frameSize.width, stateData.frameSize.height});
layoutableShadowNode.setSize(stateData.frameSize);
#ifdef ANDROID
layoutableShadowNode.setPadding({
stateData.paddingStart,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <react/renderer/components/rnscreens/utils/RectUtil.h>
#include <react/renderer/core/ConcreteComponentDescriptor.h>
#include "RNSScreenStackHeaderSubviewShadowNode.h"
#include "utils/RectUtil.h"

namespace facebook::react {

Expand All @@ -32,12 +33,11 @@ class RNSScreenStackHeaderSubviewComponentDescriptor final

auto state = std::static_pointer_cast<
const RNSScreenStackHeaderSubviewShadowNode::ConcreteState>(
shadowNode.getState());
shadowNode.getMostRecentState());
auto stateData = state->getData();

if (stateData.frameSize.width != 0 && stateData.frameSize.height != 0) {
layoutableShadowNode.setSize(
Size{stateData.frameSize.width, stateData.frameSize.height});
if (!isSizeEmpty(stateData.frameSize)) {
layoutableShadowNode.setSize(stateData.frameSize);
}

ConcreteComponentDescriptor::adopt(shadowNode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class JSI_EXPORT RNSScreenStackHeaderSubviewState final {

#endif // ANDROID

const Size frameSize{};
const Size frameSize{-1.f, -1.f};
Point contentOffset{};

#pragma mark - Getters
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,11 @@ inline constexpr bool checkFrameSizesEqualWithEps(
equalWithRespectToEps(first.height, second.height, eps);
}

/**
* @return false if any component value is less than 0
*/
inline constexpr bool isSizeEmpty(const react::Size &size) {
return size.width < 0 || size.height < 0;
}

} // namespace rnscreens

0 comments on commit 31c7035

Please sign in to comment.