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

Images not showing up reliably in List on iOS 16 #1988

Closed
3 tasks done
windom opened this issue Sep 21, 2022 · 28 comments · Fixed by #1990
Closed
3 tasks done

Images not showing up reliably in List on iOS 16 #1988

windom opened this issue Sep 21, 2022 · 28 comments · Fixed by #1990

Comments

@windom
Copy link

windom commented Sep 21, 2022

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

Issue Description

What

On iOS 16 (Xcode 14.0.1), KFImages frequently fail to show up when used inside List/ForEach. We're using them as part of a more complex list item view, but the bug is reproducible using the very simple code below.

Reproduce

import SwiftUI
import Kingfisher

struct ContentView: View {
    private static var imgUrl = URL(string: "https://cdn.iconscout.com/icon/premium/png-48-thumb/kingfisher-3765747-3140298.png")!
    
    var body: some View {
        List {
            ForEach(1...100, id: \.self) { idx in
                KFImage(Self.imgUrl)
                    .frame(width: 48, height: 48)
            }
        }
    }
}

Scroll around a bit and images will disappear and/or fail to appear.

@onevcat
Copy link
Owner

onevcat commented Sep 21, 2022

Not sure what's going wrong, but it seems that when assigning a frame would just break the view identifier when it is in the list, and even when a new cell shows up, SwiftUI does not raise the onAppear event, so Kingfisher does not have a signal to start the loading.

A very quick workaround, is just appending an empty onAppear to the frame and everything would just start to work again:

List {
  ForEach(1...100, id: \.self) { idx in
    KFImage(Self.imgUrl)
      .frame(width: 48, height: 48)
      .onAppear()
  }
}

I will investigate how this can happen and if there is a way to make SwiftUI on iOS 16 happy...

@windom
Copy link
Author

windom commented Sep 21, 2022

onAppear indeed helps in that case, but found a more complex one where nothing seems to help:

import SwiftUI
import Kingfisher

struct ContentView: View {
    private static var imgUrl = URL(string: "https://cdn.iconscout.com/icon/premium/png-48-thumb/kingfisher-3765747-3140298.png")!
    
    var body: some View {
        List {
            ForEach(1..<10) { _ in
                Section {
                    ForEach(1..<10) { idx in
                        KFImage(Self.imgUrl)
                            .frame(width: 48, height: 48)
                    }
                }
            }
        }
    }
}

@onevcat onevcat reopened this Sep 21, 2022
@onevcat
Copy link
Owner

onevcat commented Sep 21, 2022

Apple's AsyncImage has the same problem, so it seems the cell life cycle in iOS 16 is now totally broken. Not sure if there is a good way to fix it in the third party side, unless we ignore the onAppear but start the loading in view's init. However, it obviously brings other problems such as performance issues.

截屏2022-09-21 23 31 59

@onevcat
Copy link
Owner

onevcat commented Sep 21, 2022

Removing the .frame(width: 48, height: 48) can stably fix it. But again, it is not an acceptable workaround.

@windom
Copy link
Author

windom commented Sep 21, 2022

Great finds. It's sad how broken it is. To add more examples, this fails too with the onAppear workaround:

var body: some View {
    List {
        ForEach(1...100, id: \.self) { idx in
            Button(action: {  }) {
                KFImage(Self.imgUrl)
                    .frame(width: 48, height: 48)
            }
        }
    }
}

but works this way:

var body: some View {
    List {
        ForEach(1...100, id: \.self) { idx in
            HStack {
                Button(action: {  }) {
                    KFImage(Self.imgUrl)
                        .frame(width: 48, height: 48)
                }
            }
            .onAppear()
        }
    }
}

(it does not help with the nested ForEach-es)

I'm installing 16.1 Beta to check if they fixed it there.

@onevcat
Copy link
Owner

onevcat commented Sep 21, 2022

If you don't mind or if the layout can keep correctly, you can also try to change width and height to its max variances to make it load:

KFImage(Self.imgUrl)
-  .frame(width: 48, height: 48)
+  .frame(maxWidth: 48, maxHeight: 48)

And just for fun, if you create a random "offset" to the size, it seems to be going back to normal:

var delta: CGFloat {
  CGFloat.random(in: -10..<10)
}

KFImage(Self.imgUrl)
  .frame(width: 48 + delta, height: 48 + delta)

Again, it is not a recommended way or a useable workaround for most cases. But we can almost be sure it has something about the assigned frame. Maybe the frame size value is used as some kind of factor to determine whether the view should "onAppear".

@onevcat
Copy link
Owner

onevcat commented Sep 21, 2022

Another find. Add a @State in the view, and having a way to trigger a state change could also fix it (even when you never use that state in your view's body):

@State private var toggle = false

var body: some View {
  List {
    Button("Tap me") { toggle.toggle() }
    ForEach(1..<10) { sectionIndex in
      Section {
        ForEach(1..<10) { rowIndex in
          KFImage(Self.imgUrl)
            .frame(width: 48, height: 48)
        }
      }
    }
  }
}

With this fact, you can setup a timer and toggle the state frequently to keep the view updated, until Apple can fix it.

struct ContentView: View {
  private static var imgUrl = URL(string: "https://cdn.iconscout.com/icon/premium/png-48-thumb/kingfisher-3765747-3140298.png")!
  let timer = Timer.publish(every: 0.1, on: .main, in: .common).autoconnect()
  @State private var toggle = false
  
  var body: some View {
    List {
      ForEach(1..<10) { sectionIndex in
        Section {
          ForEach(1..<10) { rowIndex in
            KFImage(Self.imgUrl)
              .frame(width: 48, height: 48)
          }
        }
      }
    }
    .onReceive(timer) { time in
      toggle.toggle()
    }
  }
}

It seems to be a workable way now, but please use it at your own risk. I cannot yet fully understand what is going on and the performance impact of this timer also requires checking.

@windom I have to go and take some sleep now. Please let me know if you have any luck with iOS 16.1 beta. If no, are you going to send a feedback to Apple? Since the issue is now easy enough to reproduce even with AsyncImage, I guess a sample app with AsyncImage could be enough to submit a feedback to them. If you are not going to do that, I will create one tomorrow.

@windom
Copy link
Author

windom commented Sep 21, 2022

Thank you for the many creative workarounds, we will evaluate what to go with.

Sadly 16.1 beta 2 did not fix that much. It fixed this case:

List {
    ForEach(1...100, id: \.self) { idx in
        KFImage(Self.imgUrl)
            .frame(width: 48, height: 48)
    }
}

but not this:

List {
    ForEach(1...100, id: \.self) { idx in
        Button(action: {}) {
            KFImage(Self.imgUrl)
                .frame(width: 48, height: 48)
        }
    }
}

and nor this:

List {
    ForEach(1..<10) { _ in
        Section {
            ForEach(1..<10) { idx in
                KFImage(Self.imgUrl)
                    .frame(width: 48, height: 48)
            }
        }
    }
}

I've submitted feedback using AsyncImage. (FB11560572)

@onevcat
Copy link
Owner

onevcat commented Sep 22, 2022

I also submitted feedback for it: FB11564208

I will keep an eye on it, but now I have to say there is no good way to fix it on our end.

@Pigpocket
Copy link

Not sure what's going wrong, but it seems that when assigning a frame would just break the view identifier when it is in the list, and even when a new cell shows up, SwiftUI does not raise the onAppear event, so Kingfisher does not have a signal to start the loading.

A very quick workaround, is just appending an empty onAppear to the frame and everything would just start to work again:

List {
  ForEach(1...100, id: \.self) { idx in
    KFImage(Self.imgUrl)
      .frame(width: 48, height: 48)
      .onAppear()
  }
}

I will investigate how this can happen and if there is a way to make SwiftUI on iOS 16 happy...

This workaround worked perfectly for me. Thanks.

@onevcat
Copy link
Owner

onevcat commented Oct 19, 2022

I also submitted feedback for it: FB11564208

I will keep an eye on it, but now I have to say there is no good way to fix it on our end.

The same problem is still happening under Xcode 14.1 RC and iOS 16.1 simulators. There is no response from Apple's Feedback yet.

And the "workaround" (with performance cost) here is also still working.

@axiel7
Copy link

axiel7 commented Oct 24, 2022

I installed the new iOS 16.1 stable version and it seems is working fine now, at least with my use case.

@onevcat
Copy link
Owner

onevcat commented Oct 25, 2022

Seems the example in this comment is still causing issue and the images are not loaded even in Xcode 14.1 RC2 and its iOS 16.1 simulator.

Didn't have a chance to try on iOS 16.1 devices yet but I am not optimistic.

@windom
Copy link
Author

windom commented Oct 25, 2022

Just tried using Xcode 14.1 RC2 and real iOS 16.1 device: the original issue with KFImage directly in List/ForEach seems to be fixed, however the case with nested ForEach-es and Sections is still broken.

@onevcat
Copy link
Owner

onevcat commented Oct 25, 2022

@windom May I know which version of Kingfisher are you trying? The original issue got a workaround in #1990 and it was already a part of version 7.4.0 and on the master branch. So if you are using these latest versions, I guess it is that workaround is doing its work. But the root cause is not yet addressed.

@windom
Copy link
Author

windom commented Oct 25, 2022

@windom May I know which version of Kingfisher are you trying? The original issue got a workaround in #1990 and it was already a part of version 7.4.0 and on the master branch. So if you are using these latest versions, I guess it is that workaround is doing its work. But the root cause is not yet addressed.

I used 7.3.2 to avoid the workaround, so I think they fixed it for the simplest case.

@noefroidevaux
Copy link

It's amazing this iOS 16/16.1 bug... I spent some time trying to find a workaround, but it seems very complicated. Only the workaround with the timer worked for me. But as it has an impact on the performance, I preferred to switch from List to ScrollView + LazyVStack and it seems working well...

@GF106
Copy link

GF106 commented Nov 17, 2022

Work for me

List{
            ForEach(items, id: \.id){ item in
                ZStack{
                    NavigationLink(destination: { EmptyView() }, label: { EmptyView() })
                        .opacity(0.01)
                    //Yout code
                }
            }
        }

@eseay
Copy link

eseay commented Dec 1, 2022

I was unfortunately unable to get the onAppear workaround to work, and while I was able to get the "less preferable" workaround via Timer to deliver the desired image loading result, I determined that the performance impact and CPU overhead was just not worth it.

Similar to @noefroidevaux, I opted to go the ScrollView + LazyVStack since my lists aren't that long. It seems to be working well, but does require a little bit of manual implementation of things like separators that List gives for free.

@rslifka
Copy link

rslifka commented Dec 28, 2022

I'm also in the List + Section scenario - the only implementation that works is removing frame modifier. In that case, it works beautifully however the images are of course woefully oversized 🥲

"Oh OK! So what if you set the max height of the list item and not the image itself? That way the image can still resize and look good!" - nope 😿

"Shoot, OH... what if you use a custom LabelStyle and set the frame size there?" - nope 😿

Any frame modification along the stack breaks the lifecycle.

The largest/oldest thread (4.1k upvotes) I can find on the Apple Developer Forums also tracks with the comments here. I added a comment to that thread linking back to this discussion as I suspect it would be helpful for a subset of those (at least) 4100+ developers 😄

I suspect this has to do with the Section container, because folding and unfolding a section causes everything within that section to work properly; no workarounds needed.

@onevcat you're a hero for all the work you've done to help figure this out. Thank you!

I have a very small number (< 30) of small images to load (~1k). Is there a way to force Kingfisher to load them instead of its normal lifecycle?

@onevcat
Copy link
Owner

onevcat commented Jan 2, 2023

@rslifka Previously in the dark age of SwiftUI, there was once a loadImmediately which might be "fixing" this issue. However, it is now removed long ago. Maybe it is actually a good chance to revive it for this issue 😂. I will see if that can work or no.

@onevcat
Copy link
Owner

onevcat commented Jan 2, 2023

I added a method in this fix/load-before-view-appear branch. It basically brings back the functionality of loadImmediately back and allows you to load the images as soon as the KFImage body is evaluated if the image is not yet loaded.

I guess it can somehow play as a better workaround before Apple can fix the issue on the SwiftUI side.

To use it:

  1. Change your integration of Kingfisher to fix/load-before-view-appear branch
  2. Add a startLoadingBeforeViewAppear() to the KFImage when you encountered this issue:
KFImage(someURL)
+ .startLoadingBeforeViewAppear()
  .resizable()
  .frame(width: 200, height: 200)

I need some help to try it in more complicate scenes. If it works fine, maybe we can consider including it in the master branch and tagging a release.

@ren6
Copy link

ren6 commented Jan 6, 2023

@onevcat
I can confirm your fix/load-before-view-appear branch has fixed this bug. I had this issue with complex UI: a ListView with Sections, and each section having several KFImages.
Now the issue is gone, images are loading correctly with your .startLoadingBeforeViewAppear() modifier.

List(objects) { obj in
                Section(content: {
                    ForEach(...) {
                         KFImage(URL(string: string))
                             .placeholder {
                                ProgressView().progressViewStyle(CircularProgressViewStyle(tint: Color.gray))
                            }
                            .fade(duration: 0.2)
                            .startLoadingBeforeViewAppear()
                            .resizable()
                            .aspectRatio(contentMode: .fit)
               }
         })

@MartinStrambach
Copy link

MartinStrambach commented Jan 6, 2023

I can confirm the issue is fixed with changes in fix/load-before-view-appear for us as well. Though I can't share any minimal working example because we have many nested UI elements.

@htb
Copy link

htb commented Jan 6, 2023

I can also confirm that this is working. Used in a list view, where only the aggressive timed update workaround worked before.

@onevcat
Copy link
Owner

onevcat commented Jan 7, 2023

Thank you, all folks!

Then let me prepare a release for this as a better workaround, before Apple can fix it someday later.

@onevcat
Copy link
Owner

onevcat commented Jan 8, 2023

A new version 7.5.0 was released in which this new modifier is contained. I am closing this for now and if Apple can fix it one day I will append some more comments here.

Thank you all for reporting and helping verify it!

@onevcat
Copy link
Owner

onevcat commented Mar 2, 2023

I can confirm that this issue has been fixed by Apple in Xcode 14.3 (now still in beta) and iOS 16.4. Since that, I will try to add some version checks in the startLoadingBeforeViewAppear method to skip the workaround in newer iOS versions and mark this method as soft-deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.