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

Incorrect childCount in removeViewAt #2282

Closed
BenIrving opened this issue Aug 5, 2024 · 6 comments · Fixed by #2307
Closed

Incorrect childCount in removeViewAt #2282

BenIrving opened this issue Aug 5, 2024 · 6 comments · Fixed by #2307
Assignees
Labels
NewArch Issues related only to new architecture Platform: Android This issue is specific to Android Repro provided A reproduction with a snack or repo is provided

Comments

@BenIrving
Copy link
Contributor

Description

There is a bug in react-native-screens on Android when newArchEnabled=true and we have FlatLists in our screens

We attempt to remove an incorrect number of children when calling removeViewAt in SurfaceMountingManager

This seems related to the changes introduced by @WoLewicki in 2134 as we patched these changes into a previous version of rns and this bug became immediately apparent. This was tested on rn73 & rn74 as well

Screenshot_1722864873

Steps to reproduce

  1. Press listitem1
  2. Press listitem2
  3. Observe crash

Snack or a link to a repository

https://github.com/BenIrving/rnsrepro

Screens version

3.34.0

React Native version

0.75.0-rc.6

Platforms

Android

JavaScript runtime

None

Workflow

React Native (without Expo)

Architecture

Fabric (New Architecture)

Build type

Debug mode

Device

None

Device model

No response

Acknowledgements

Yes

@github-actions github-actions bot added Platform: Android This issue is specific to Android Repro provided A reproduction with a snack or repo is provided labels Aug 5, 2024
@kkafar kkafar added the NewArch Issues related only to new architecture label Aug 6, 2024
@alduzy alduzy self-assigned this Aug 7, 2024
@alduzy
Copy link
Member

alduzy commented Aug 7, 2024

@BenIrving Thanks for reporting the issue, I am working on a possible fix.
As a workaround you can try setting the removeClippedSubviews option in the FlatList to false.

@janicduplessis
Copy link
Contributor

I have also encountered this issue, for now I reverted #2134 since it would cause screens to go completely blank when navigating back to it.

@BenIrving
Copy link
Contributor Author

@alduzy I can confirm the workaround fixes the issue. Is there any progress on a proper fix here?

@alduzy
Copy link
Member

alduzy commented Aug 19, 2024

@BenIrving There is a fix, but not the prettiest one. I'll prepare a PR

alduzy added a commit that referenced this issue Aug 20, 2024
…t on fabric (#2307)

## Description

This PR intents to fix the crash when navigating back from a screen with
FlatList on the new architecture. The crash was caused by miscalculated
`childCount` of the list.
Earlier on I found out that setting the
[removeClippedSubviews](https://reactnative.dev/docs/flatlist#removeclippedsubviews)
option to false (defaults to true on Android) in the FlatList fixes the
problem.

This PR is rather a quick fix with an extra condition, that adds simple
views in place of the miscalculated ones in `startTransitionRecursive`
function if there's a FlatList with `removeClippedSubviews` option set.

Fixes #2282.

## Changes

- added `Test2282.tsx` repro
- added extra condition in `startTransitionRecursive` function

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

- added `Test2282.tsx` repro

## Checklist

- [x] Ensured that CI passes
@Akash95B
Copy link

I am still facing the issue when i am using sectionlist and flatlist. And I am getting the error while navigating back.
@WoLewicki
Screenshot 2024-09-19 at 2 19 51 PM

Below is the sectionlist component

`import React, { useState } from 'react';
import { View, SectionList, FlatList, Image, TouchableOpacity } from 'react-native';
import UpArrow from '../../../../../assets/svg/UpArrow.svg';
import DownArrow from '../../../../../assets/svg/DownArrow.svg';
import styles from '../components/SectionStyle';
import { useTheme } from '@react-navigation/native';
import GilroyText from '../../../../../common/components/Text';

const SectionListItem = ({ sections = [] }) => {
const { colors } = useTheme();
const themedStyles = styles(colors);
const [expandedSection, setExpandedSection] = useState(sections.length > 0 ? sections[0].title : null);

const toggleSection = (title: any) => {
setExpandedSection(expandedSection === title ? null : title);
};

const renderSlotCard = ({ item }) => (

{'AF2'}
{item.time}
{item.slots} slots
<Image source={{ uri: 'https://randomuser.me/api/portraits/men/32.jpg' }} style={themedStyles.coachImage} />
{item.coach}

);

const renderDayHeader = (day: string | number | boolean | React.ReactElement<any, string | React.JSXElementConstructor> | Iterable<React.ReactNode> | React.ReactPortal | null | undefined) => (
{day}
);

const renderHorizontalList = ({ data }) => (
<FlatList
data={data || []}
renderItem={renderSlotCard}
keyExtractor={(item) => item.id}
horizontal
showsHorizontalScrollIndicator={false}
contentContainerStyle={themedStyles.horizontalListContent}
removeClippedSubviews={false}
/>
);

const renderSectionHeader = ({ section: { title } }) => {
const isExpanded = expandedSection === title;

return (
  <TouchableOpacity
    style={[
      themedStyles.sectionHeader,
      isExpanded && themedStyles.expandedSectionHeader,
    ]}
    onPress={() => toggleSection(title)}
  >
    <GilroyText style={themedStyles.sectionTitle}>{title}</GilroyText>
    {isExpanded ? <UpArrow /> : <DownArrow />}
  </TouchableOpacity>
);

};

const renderSectionFooter = ({ section }) => {
const isExpanded = expandedSection === section.title;

if (!isExpanded) return null;

return (
  <View style={themedStyles.expandedSection}>
    {section.days.map((dayItem) => (
      <View key={dayItem.day}>
        {renderDayHeader(dayItem.day)}
        {renderHorizontalList({ data: dayItem.data })}
      </View>
    ))}
    <View style={themedStyles.expandedSectionBottomBorder} />
  </View>
);

};

return (
<View style={{ flex: 1, marginLeft: 20 }}>
<SectionList
sections={sections.map(section => ({
...section,
data: section.data || [],
}))}
renderItem={() => null}
renderSectionHeader={renderSectionHeader}
renderSectionFooter={renderSectionFooter}
keyExtractor={(item) => item.id}
contentContainerStyle={themedStyles.sectionListContent}
removeClippedSubviews={false}
/>

);
};

export default SectionListItem;
`

@WoLewicki
Copy link
Member

@Akash95B is #2330 included in your build?

ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this issue Oct 9, 2024
…t on fabric (software-mansion#2307)

## Description

This PR intents to fix the crash when navigating back from a screen with
FlatList on the new architecture. The crash was caused by miscalculated
`childCount` of the list.
Earlier on I found out that setting the
[removeClippedSubviews](https://reactnative.dev/docs/flatlist#removeclippedsubviews)
option to false (defaults to true on Android) in the FlatList fixes the
problem.

This PR is rather a quick fix with an extra condition, that adds simple
views in place of the miscalculated ones in `startTransitionRecursive`
function if there's a FlatList with `removeClippedSubviews` option set.

Fixes software-mansion#2282.

## Changes

- added `Test2282.tsx` repro
- added extra condition in `startTransitionRecursive` function

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

- added `Test2282.tsx` repro

## Checklist

- [x] Ensured that CI passes
kkafar pushed a commit that referenced this issue Oct 25, 2024
…t on fabric (#2307)

This PR intents to fix the crash when navigating back from a screen with
FlatList on the new architecture. The crash was caused by miscalculated
`childCount` of the list.
Earlier on I found out that setting the
[removeClippedSubviews](https://reactnative.dev/docs/flatlist#removeclippedsubviews)
option to false (defaults to true on Android) in the FlatList fixes the
problem.

This PR is rather a quick fix with an extra condition, that adds simple
views in place of the miscalculated ones in `startTransitionRecursive`
function if there's a FlatList with `removeClippedSubviews` option set.

Fixes #2282.

- added `Test2282.tsx` repro
- added extra condition in `startTransitionRecursive` function

<!--

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

-->

- added `Test2282.tsx` repro

- [x] Ensured that CI passes

(cherry picked from commit c47ad84)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NewArch Issues related only to new architecture Platform: Android This issue is specific to Android Repro provided A reproduction with a snack or repo is provided
Projects
None yet
6 participants