facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
117.77k stars 24.15k forks source link

[iOS/iPadOS] - `FlatList` refresh handling broken/dysfunctional #36173

Open coolersham opened 1 year ago

coolersham commented 1 year ago

Description

Description

Since upgrading to the new architecture it seems like the refresh/pull-to-refresh feature of the FlatList is broken/not fully functional any longer. However, this is only the case if you place another FlatList OR ScrollView in the same view. Placing just a list in each view does not lead to problems.

Pulling down and refreshing works twice before being dysfunctional. Eventually, after a certain pattern, the functionality works again twice and then breaks again.

The sample code provided below works as intended on the old architecture.

Version

0.71.3

Output of npx react-native info

Not relevant for this issue.

Steps to reproduce

  1. Create a new react-native project with the help of the CLI, install all dependencies and pods and build the application on a iOS/iPadOS device.
  2. Paste the provided code into the App.tsx file
  3. Pull down the list and notice how the loading spinner is displayed for a brief time
  4. Use the Switch button to unmount the current view and mount another one containing two FlatList components and a ScrollView (for demonstration purposes).
  5. Pull to refresh on the most left FlatList (orange one) and once again use the Switch button
  6. Pull down the initial list and notice how the loading spinner is NOT displayed and the onRefresh handler is not called

Snack, code example, screenshot, or link to a repository

https://github.com/coolersham/NRNA-flatlist-reload-bug

import React, { useState } from "react"

import {
  FlatList,
  ScrollView,
  Text,
  TouchableOpacity,
  View,
} from "react-native"

export default function App(): JSX.Element {
  return (
    <View
      style={{
        flex: 1,
        padding: 48,
        alignItems: "center",
        backgroundColor: "white",
      }}
    >
      <ListDemo />
    </View>
  )
}

function ListDemo() {
  const data = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
  const [show, setShow] = useState(true)

  const style = { backgroundColor: "orange" }

  const switchButton = (
    <TouchableOpacity
      style={{
        height: 48,
        width: 300,
        alignItems: "center",
        backgroundColor: "red",
        justifyContent: "center",
      }}
      onPress={() => setShow((show) => !show)}
    >
      <Text style={{ color: "white" }}>Switch</Text>
    </TouchableOpacity>
  )

  if (show)
    return (
      <>
        {switchButton}
        <FlatList
          data={data}
          style={style}
          refreshing={false}
          renderItem={GenericListItem}
          onRefresh={() => console.log("List #1 refresh is called")}
        />
      </>
    )

  return (
    <>
      {switchButton}
      <View
        style={{
          flex: 1,
          flexDirection: "row",
        }}
      >
        <FlatList
          data={data}
          style={style}
          refreshing={false}
          renderItem={GenericListItem}
          onRefresh={() => console.log("List #2 refresh is called")}
        />

        {/* Breaks refresh handling of both lists occasionally */}
        <FlatList
          data={data}
          renderItem={GenericListItem}
          style={{ backgroundColor: "purple" }}
        />

        {/* The same applies to the normal ScrollView component */}
        <ScrollView style={{ backgroundColor: "blue" }}>
          {data.map((item) => (
            <GenericListItem key={item} />
          ))}
        </ScrollView>
      </View>
    </>
  )
}

function GenericListItem() {
  return (
    <View style={{ width: 300, height: 48 }}>
      <Text>Generic test item</Text>
    </View>
  )
}
redpanda-bit commented 1 year ago

I was unable to reproduce despite following the steps. Tested on iPhone 13, iOS 15.2. onRefresh gets called every single time.

coolersham commented 1 year ago

@carlosalmonte04 Did you enable the new architecture, Fabric and Hermes and build the application afterwards?

redpanda-bit commented 1 year ago

I did the steps below

  1. npx react-native init AwesomeApp --version=0.71.3
  2. cd in folder
  3. run npm install
  4. cd into ios
  5. run pod install
  6. copied and pasted provided code into App.tsx
  7. manual testing as described; - pulled the orange list, switched, pulled the new orange list, switched and pulled the original orange list

onRefresh got called every single time. Let me know if you'd like me to try a different way.

https://user-images.githubusercontent.com/25206487/219493096-aab9a704-96d7-4d3a-a27c-0f38567d8ca2.gif

coolersham commented 1 year ago

@carlosalmonte04 As stated in the main description of the issue, the problem is related to the new architecture. The template that the CLI retrieves in step 1. still uses the old architecture as a default setting.

You have to modify your fifth step to enable the new architecture and its corresponding parts. Instead of pod install do NO_FLIPPER=1 USE_FABRIC=1 USE_HERMES=1 RCT_NEW_ARCH_ENABLED=1 pod install.

Now you should be able to reproduce and perceive the bug. Please let me know if that has worked for you.

redpanda-bit commented 1 year ago

Yes, that worked for me after using NO_FLIPPER=1 USE_FABRIC=1 USE_HERMES=1 RCT_NEW_ARCH_ENABLED=1 pod install. I am able to reproduce the issue.

redpanda-bit commented 1 year ago

I'm adding a few breakpoints on the native side to see if I find something.

Also, adding a custom refreshControl component completely disables the onRefresh callback. Pulled down to refresh multiple times after adding the simple custom refreshControl component and onRefresh was not triggered.

redpanda-bit commented 1 year ago

Might need to update designs temporarily to have a refresh button instead of pull-to-refresh. Or have a SectionList for the second list. Both approaches are disappointing but I can't remember the solution last time I experience this issue, I remember it had to do with styling issues.

<SectionList
        horizontal
        sections={[
          {
            data: [1, 2, 3],
            renderItem: item => (
              <FlatList
                contentContainerStyle={{
                  backgroundColor: bgColor[item.item],
                }}
                data={data}
                renderItem={() => (
                  <View style={{width: 100}}>
                    <GenericListItem />
                  </View>
                )}
              />
            ),
          },
        ]}
        refreshing={false}
        onRefresh={() => console.log('List #2 refresh is called')}
        renderItem={GenericListItem}
      />
    </>

πŸ‘Ž

ezgif-2-a2900da66b

coolersham commented 1 year ago

Might need to update designs temporarily to have a refresh button instead of pull-to-refresh. Or have a SectionList for the second list. Both approaches are disappointing but I can't remember the solution last time I experience this issue, I remember it had to do with styling issues.

@carlosalmonte04 Thanks for looking into this and providing a possible workaround. However, as you mentioned, those approaches are disappointing and unsatisfactory.

I am not quite aware of how the responsibilities are divided in react-native matters, but could someone from the core or rather new architecture team look into this issue, please? Maybe I am doing something wrong or miss a piece of important information, but I think this functionality should not act like that. Even more, because it worked on the old architecture just fine.

Maybe @cortinico or would you mind delegating? Thanks!

coolersham commented 1 year ago

Again I kindly ask that someone from the core or new architecture team of react-native looks into this, please. Please just leave a comment, whether this issue will never be fixed or you do not have time for this/your priorities are spread differently, so that all people who are struggling with that issue know what the next steps will be.

redpanda-bit commented 1 year ago

@coolersham did you end up finding a way to solve this?

coolersham commented 1 year ago

@carlosalmonte04 Sadly, I have not found a way to solve this particular broken feature. However, at this point, I am pretty confused and a little bit disappointed by the way the Facebook/React Native team is handling issues. Looking at the sheer number of open issues my hopes are low that we will receive an answer on this one soon.

I am well aware that there are probably more important things to focus on, but as mentioned before, even a short answer would be sufficient to let us know the next steps.

cortinico commented 1 year ago

However, at this point, I am pretty confused and a little bit disappointed by the way the Facebook/React Native team is handling issues

@coolersham we receive hundreds of issues every day and sadly our team is really small. We're trying to do our best to look at all of them and provide answers.

I've looked at your issue and sadly I don't have relevant context/suggestions to share at this stage.

coolersham commented 1 year ago

Thank you @cortinico, I totally understand your situation!

Alright. So will this ever be analysed in detail or does the community have to wait, whether this issue may be fixed over time or figure it out by themselves and create a PR for it?

cortinico commented 1 year ago

@coolersham having a reproducer project would be a first step to make investigation easier for us and also for folks in the community.

You can use this template: https://github.com/react-native-community/reproducer-react-native

If someone from the community is willing to investigate and fix this, we'll be more than happy to review & merge the PR

coolersham commented 1 year ago

@cortinico I already provided the code needed to experience this bug in my initial issue description and provided all other necessary information. As you can perceive in the timeline of this issue, the bug got already confirmed by @carlosalmonte04. If it would remove the little overhead needed to experience that bug, I could also create a reproduction repo and share it here.

cortinico commented 1 year ago

If it would remove the little overhead needed to experience that bug, I could also create a reproduction repo and share it here.

Please do πŸ™ That would also help others which wish to investigate this

coolersham commented 1 year ago

Please do πŸ™ That would also help others which wish to investigate this

@cortinico Here is the repository that reproduces this bug in the latest version of react-native (0.71.6).

Alecattie commented 1 year ago

Any update on this?

coolersham commented 1 year ago

@Alecattie As no one commented here or linked a PR since I provided the reproduction repo - the answer is no.

coolersham commented 11 months ago

Almost seven months have passed since I opened this issue. Five months ago I provided a simple repro so this issue can be further investigated and fixed. Since then, nothing has happened from the maintainer or community side, except a quick fix in another issue, targeting the same problem #37308, which sadly brings along negative side effects and does not work in various scenarios either.

Has anyone found a viable solution that can be merged, or do we need to live without this expected standard from now on?

coolersham commented 8 months ago

Just upgraded the provided reproducer to the latest version of react-native (0.73.1) to check whether this issue has been fixed now. Turns out, this is sadly not the case. I have already moved on to an alternative implementation (functionality- and UI-wise). It still baffles me, that this bug is still around after more than 10 months and no one found a solution.

marcos-vinicius-mafei commented 7 months ago

Just upgraded the provided reproducer to the latest version of react-native (0.73.1) to check whether this issue has been fixed now. Turns out, this is sadly not the case. I have already moved on to an alternative implementation (functionality- and UI-wise). It still baffles me, that this bug is still around after more than 10 months and no one found a solution.

I'm also having this problem with the latest react-native version 😞 Any updates on this?

alex-strae commented 4 months ago

This is still a problem on 0.73.6 on New Architecture. As is italic fonts are no longer italic and modals flashing when modal dismiss (but not if pressing Cancel button).

cipolleschi commented 4 months ago

Hi there, Sorry for the long silence, but as @cortinico mentioned above, we receive tons of issues, the team is small and we need to prioritize ruthlessly.

I'll start looking in to the issue today and for the next few days. Thanks @coolersham for the reproducer, I'll try to keep you all updated on the progress.


Update 1: I can repro the problem. I also noticed another difference between the Old and the New Architecture: * in the Old Architecture, the UIActivityIndicator is below the content view: when the content view is released, and it comes back to the top, it hides the UIActivityIndicator. * in the New Architecture, the UIActivityIndicator is in front of the content view: when the content view is released, and it comes back to the top, UIActivityIndicator hides part of the ListView.

☝️ this is caused by the reproducers that sets the refreshing to false and does not handle the state properly, so it is not a bug in Core.

cipolleschi commented 4 months ago

We found the cause of the issue, and it was because the RCTPullToRefreshViewComponentView was not recycled correctly. What happened is that the UIRefreshControl was not deallocated and remained assigned to a disposed ScrollView. By properly recreating it in prepare for recycle, we fixed the bug.