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

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

Open
coolersham opened this issue Feb 15, 2023 · 25 comments
Open

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

coolersham opened this issue Feb 15, 2023 · 25 comments
Labels
Component: FlatList Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Platform: iOS iOS applications. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@coolersham
Copy link

coolersham commented Feb 15, 2023

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>
  )
}
@coolersham coolersham added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Feb 15, 2023
@redpanda-bit
Copy link

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

@coolersham
Copy link
Author

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

@redpanda-bit
Copy link

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
Copy link
Author

@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
Copy link

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
Copy link

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
Copy link

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
Copy link
Author

coolersham commented Feb 26, 2023

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
Copy link
Author

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
Copy link

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

@coolersham
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

coolersham commented Apr 6, 2023

@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
Copy link
Contributor

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
Copy link
Author

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).

@cortinico cortinico added the Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. label Apr 11, 2023
@Alecattie
Copy link

Any update on this?

@coolersham
Copy link
Author

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

@coolersham
Copy link
Author

coolersham commented Sep 13, 2023

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
Copy link
Author

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
Copy link

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
Copy link

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
Copy link
Contributor

cipolleschi commented Apr 11, 2024

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 added a commit to cipolleschi/react-native that referenced this issue Apr 11, 2024
Summary:
This change properly recycles the RCTPullToRefreshViewComponentView so that it fixes some misbehaviors of the UIRefreshControl in OSS.

This should fix facebook#37308 and facebook#36173

## Changelog:
[iOS][Fixed] - Properly recycle the RCTPullToRefreshViewComponentView

Differential Revision: D56018924
cipolleschi added a commit to cipolleschi/react-native that referenced this issue Apr 11, 2024
Summary:

This change properly recycles the RCTPullToRefreshViewComponentView so that it fixes some misbehaviors of the UIRefreshControl in OSS.

This should fix facebook#37308 and facebook#36173

## Changelog:
[iOS][Fixed] - Properly recycle the RCTPullToRefreshViewComponentView

Differential Revision: D56018924
@cipolleschi
Copy link
Contributor

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.

facebook-github-bot pushed a commit that referenced this issue Apr 12, 2024
Summary:
Pull Request resolved: #44047

This change properly recycles the RCTPullToRefreshViewComponentView so that it fixes some misbehaviors of the UIRefreshControl in OSS.

This should fix #37308 and #36173

## Changelog:
[iOS][Fixed] - Properly recycle the RCTPullToRefreshViewComponentView

Reviewed By: sammy-SC

Differential Revision: D56018924

fbshipit-source-id: 3c71328aa0f6fb2a98a19593f0f06419e04e9cae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: FlatList Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Platform: iOS iOS applications. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

No branches or pull requests

8 participants