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

fix(Android,Fabric): prevent another infinite state update loop for header subviews with zero size #2696

Merged
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());
Copy link
Member Author

Choose a reason for hiding this comment

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

The bug I've reported in earlier comment was caused by accessing mounted state and not the most recent one.

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
Loading