Skip to content

fix(Android,Fabric): prevent Yoga from stretch-fitting height of header subview #2811

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 27, 2025
Merged

fix(Android,Fabric): prevent Yoga from stretch-fitting height of header subview #2811

merged 3 commits into from
Mar 27, 2025

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Mar 26, 2025 •
edited
Loading

Description

When setting header subviews from the rendered component via setOptions the native header enlarges (see the "before" video below 👇).

Bug mechanism

When HeaderSubview is set from setOptions its mounted after first render has happened.
It means that HeaderConfig has already been laid out and possibly its shadow state got
updated with its size. Now, when first layout for the HeaderSubview is computed Yoga
will stretch-fit HeaderSubview height to fill available space - the HeaderSubview will
receive height equal to the height of the HeaderConfig. Such frame will be then send
to HostTree, triggering native header layout, which will expand to make enough space for
such high HeaderSubview & additional padding. Thanks to #2696 the update cycle will be broken
& the issue described in #2675 won't happen.

Note that there is no such buggy behaviour in case the HeaderSubviews is passed directly as option
when defining a Screen. This is because Yoga resolves the childHeight (HeaderSubview height)
differently depending on whether containingNode's height (HeaderConfig's height) is defined upfront or not.
When the containingNode height is not known (case of initial render with HeaderSubview present) the Yoga will
use FitContent or MaxContent (not sure here) SizingMode.
In cases, it is known (HeaderConfig has received state from HT, case of HeaderSubview rendered via setOptions) - StretchFit will be used for some reason (taking into consideration all layout options, including flex
direction which is row for both HeaderConfig and HeaderSubview).

I believe this regression has been introduced in #2466, where we added state updates for HeaderConfig
and HeaderSubviews. We need these state updates though, however it seems that we do not need to inform Yoga
with HeaderConfig height & therefore avoid this layout problem.

Debugging trail

It seems that the SizingMode for laying out HeaderSubview is determined here.,
which is being called from computeFlexBasisForChild.
The resolveChildAlignment method returns there Align::Strech and this leads to SizingMode::StretchFit being used later on when
measuring/laying out HeaderSubview.

Recordings

before after
before.mov
after.mov

Changes

Now, we do set only width & horizontal padding of the HeaderConfig. YGUndefined is passed as height argument to setSize call.

Test code and steps to reproduce

Added Test2811 that allows to test these changes directly. We need to also check following test cases for regressions:

Warning

There is a issue when header elements set in setOptions disappear / become invisible. link to internal board. It seems that it happens also on 4.9.2 and therefore is not a regression. However it is bad & must be fixed before stable release.

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes (db55977)

@kkafar
Prevent Yoga from stretch-fitting height of subview
db55977 
Yoga does that (stretch fits) when header config has its size set.
When header config does not have size (height) defined upfront,
this does not happen. It uses different SizingMode then.
@kkafar kkafar force-pushed the @kkafar/flickering-header-subviews-2 branch from 5daefb1 to db55977 Compare March 26, 2025 10:27
kligarski
kligarski approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@kligarski kligarski left a comment

Choose a reason for hiding this comment

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

Looks good.

@kkafar kkafar mentioned this pull request Mar 27, 2025
Merged
3 tasks
@kkafar kkafar merged commit b1e9c71 into main Mar 27, 2025
8 of 9 checks passed
@kkafar kkafar deleted the @kkafar/flickering-header-subviews-2 branch March 27, 2025 12:40
kkafar added a commit that referenced this pull request Mar 27, 2025
@kkafar
fix(Android,Fabric): prevent header subview disappearance when using …
ab2a303 
…`setOptions` (#2812)

## Description

* [x] Should be merged after #2811 & rebased.

When setting subviews via `setOptions` from `useEffect` hook in a
component, the first frame received might be computed by native
layout & completely invalid (zero height). RN layout is the source of a
subview **size** (not origin). When we send such update with zero height
Yoga might (or might not, depending on exact update timing in relation
to other ongoing commits / layouts) set the subview height to 0!
This causes the subview to become invisible & we want to avoid that.

This had not been a problem before
#2696,
because we would filter out
this kind of frame in the `setSize` guard in the
`ComponentDescriptor.adopt` method of the `HeaderSubview`.
#2696
allowed
for zero-sized frames for other, unrelated reason & we must allow these
  as long as they come from React Native layout & not native one.



## Changes

We now filter these invalid frames on the side of HostTree, by detecting
whether React has measured the subview or not.

## Test code and steps to reproduce

I've tested the problem on slightly modified `Test2466`:

<details>

<summary>Code snippet</summary>

```tsx
        import { NavigationContainer } from '@react-navigation/native';
        import { createNativeStackNavigator, NativeStackNavigationProp } from '@react-navigation/native-stack';
        import React from 'react';
        import { findNodeHandle, Text, View } from 'react-native';
        import PressableWithFeedback from '../shared/PressableWithFeedback';

        type StackParamList = {
          Home: undefined,
        }

        type RouteProps = {
          navigation: NativeStackNavigationProp<StackParamList>;
        }

        const Stack = createNativeStackNavigator<StackParamList>();

        function HeaderTitle(): React.JSX.Element {
          return (
            <PressableWithFeedback
              onLayout={event => {
                const { x, y, width, height } = event.nativeEvent.layout;
                console.log('Title onLayout', { x, y, width, height });
              }}
              onPressIn={() => {
                console.log('Pressable onPressIn');
              }}
              onPress={() => console.log('Pressable onPress')}
              onPressOut={() => console.log('Pressable onPressOut')}
              onResponderMove={() => console.log('Pressable onResponderMove')}
              ref={node => {
                console.log(findNodeHandle(node));
                node?.measure((x, y, width, height, pageX, pageY) => {
                  console.log('header component measure', { x, y, width, height, pageX, pageY });
                });
              }}
            >
              <View style={{ height: 40, justifyContent: 'center', alignItems: 'center' }}>
                <Text style={{ alignItems: 'center' }}>Regular Pressable</Text>
              </View>
            </PressableWithFeedback>
          );
        }

        function HeaderLeft(): React.JSX.Element {
          return (
            <HeaderTitle />
          );
        }

        function Home({ navigation }: RouteProps): React.JSX.Element {
          React.useEffect(() => {
            console.log('calling setOptions in useEffect');
            navigation.setOptions({
              //headerTitle: HeaderTitle,
              headerLeft: HeaderLeft,
              //headerRight: HeaderLeft,
            });
          }, [navigation]);
          return (
            <View style={{ flex: 1, backgroundColor: 'rgba(0, 0, 0, .8)' }}
            >
              <View style={{ flex: 1, alignItems: 'center', marginTop: 48 }}>
                <PressableWithFeedback
                  onPressIn={() => console.log('Pressable onPressIn')}
                  onPress={() => console.log('Pressable onPress')}
                  onPressOut={() => console.log('Pressable onPressOut')}
                >
                  <View style={{ height: 40, width: 200, justifyContent: 'center', alignItems: 'center' }}>
                    <Text style={{ alignItems: 'center' }}>Regular Pressable</Text>
                  </View>
                </PressableWithFeedback>
              </View>
            </View>
          );
        }

        function App(): React.JSX.Element {
          return (
            <NavigationContainer>
              <Stack.Navigator>
                <Stack.Screen
                  name="Home"
                  component={Home}
                  options={{
                    //headerTitle: HeaderTitle,
                    //headerLeft: HeaderLeft,
                    //headerRight: HeaderLeft,
                  }}
                />
              </Stack.Navigator>
            </NavigationContainer>
          );
        }

        export default App;
```

</details>

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes
(7d3205e)
@kkafar kkafar mentioned this pull request Mar 28, 2025
Closed
8 tasks
@kkafar kkafar mentioned this pull request Apr 14, 2025
Closed
9 tasks
@kkafar kkafar mentioned this pull request May 7, 2025
Merged
9 tasks
kkafar added a commit that referenced this pull request May 7, 2025
@kkafar
fix(Fabric,iOS): header subviews do not support dynamic content chang…
ad24734 
…es (#2905)

## Description

Regression most likely introduced in 4.5.0 by #2466.
Fixes #2714
Fixes #2815
Supersedes #2845


This is a ugly hack to workaround issue with dynamic content change.
When the size of this shadow node contents (children) change due to JS
update, e.g. new icon is added, if the size is set for the yogaNode
corresponding to this shadow node, the enforced size will be used
and the size won't be updated by Yoga to reflect the contents size
change -> host tree won't get layout metrics update -> we won't trigger
native
layout -> the views in header will be positioned incorrectly.

> [!important]
> This PR handles iOS only, however there is **similar** issue on
Android. The issue can be reproduced on the same test example. Android
will be handled in separate PR.

## Changes

## Test code and steps to reproduce

In this approach I've settled with:

1. not calling set size on iOS for
`RNSScreenStackHeaderSubviewShadowNode`,
2. updating header config padding & sending it as state to shadow tree.

Added `Test2714`

Most of the fragile header interactions must be tested:

* [x] Header title truncation - `TestHeaderTitle` ~❌ This PR introduces
regression here on iOS (Android not tested yet)~ ✅ Works
* [x] Pressables in header - `Test2466` (iOS works, Android code is
unmodified here)
* [x] #2807
(this PR does not touch Android)
* [x] #2811
(this PR does not touch Android)
* [x] #2812
(this PR does not touch Android)
* [x] Header behaviour on orientation changes -
#2756 (this
PR does not touch Android)
* [x] New test `Test2714` handling header item resizing.
## Checklist

- [x] Included code example that can be used to test this change
- [ ] Ensured that CI passes
@kkafar kkafar mentioned this pull request May 8, 2025
Merged
9 tasks
kkafar added a commit that referenced this pull request May 8, 2025
@kkafar
fix(Fabric,Android): header subviews do not support dynamic content c…
0e2fd32 
…hanges (#2910)

## Description

Fixes
#2714 on
Android
Fixes
#2815 on
Android

See #2905 for detailed description.

## Changes

Removed call to `RNSScreenStaceaderSubviewShadowNode.setSize` in
corresponding component descriptor.

It seems that we do not need to enforce node size from HostTree. Setting
appropriate content offset is enough for pressables to function
correctly (assuming that native layout **does not change size of any
subview**). I currently can't come up with any scenario where this would
happen.

## Test code and steps to reproduce

I've tested:

* [x] `Test2714` introduced in PR with iOS fixes -
#2905
* [x] Pressables in header -
#2466,
* [x] Header title truncation -
#2325 (only
few cases, as the list is long there) & noticed a regression (not
related to this PR, described in comment below the PR description),
* [x] Insets with orientation change (Android) -
#2756
* [x] #2807
(on `TestHeaderTitle` with call to `setOptions` in `useLayoutEffect`)
* [x] `Test2811` -
#2811
* [x] #2812
(with snippet provided in that PR description)



## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kligarski kligarski kligarski approved these changes

@maciekstosio maciekstosio Awaiting requested review from maciekstosio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kkafar @kligarski