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

Fix hosting view cell sizing bug #150

Merged

Conversation

bryankeller
Copy link
Contributor

Change summary

This fixes a tricky sizing issue that occurs due to slightly incorrect timing when setting up an EpoxySwiftUIHostingView for use in a collection view cell. The issue occurs due to us not installing our hosting controller (view controller) as a child prior to the cell being asked to size itself. The end result is that the cell sizes itself without having the hosting controller completely installed, and therefore lays out with its estimated size.

In most "normal" cases, there's no timing or sizing issue. However, an edge case exists where we update our collection view while it's currently off screen (for example, when it's not in the selected tab / view controller of a tab controller). Here's what that bug looks like in the Airbnb app:

RPReplay_Final1692239266.MP4

I spent some time creating a small project to reproduce the issue, and eventually identified exactly what Epoxy was doing differently compared to my own hosting controller / cell solution. Here's the bug reproduced in the example project:

Magazine Layout Launch Magazine Layout Reload Off Screen MagazineLayout Layout Issue
Simulator Screenshot - iPhone 14 Pro - 2023-08-16 at 18 10 56 Simulator Screenshot - iPhone 14 Pro - 2023-08-16 at 18 11 16 Simulator Screenshot - iPhone 14 Pro - 2023-08-16 at 18 11 04
Flow Layout Launch Flow Layout Reload Off Screen Flow Layout Issue
Simulator Screenshot - iPhone 14 Pro - 2023-08-16 at 18 10 34 Simulator Screenshot - iPhone 14 Pro - 2023-08-16 at 18 11 16 Simulator Screenshot - iPhone 14 Pro - 2023-08-16 at 18 10 39

The bug

The root issue was that we were using didMoveToWindow / the presence of a window as an indicator of whether or not we'd be able to find a prospective parent view controller up our responder chain - we need this view controller to install our hosting controller as a child. The assumption that we need a non-nil window in order to find a parent view controller up our responder chain is wrong, and delayed us from adding our hosting controller as a child until after the cell had been asked to self-size. Eventually, we would add our child view controller to some ancestor view controller, but it was too late, and the collection view would not ask us to self-size again without interacting with the collection view (scrolling it, forcing a layout invalidation some other way).

Broken down into sequential steps, here are the things that happen to cause the bug:

  1. collectionView(_:cellForItemAt:) is called
  2. An EpoxySwiftUIHostingView is created via makeView()
  3. The hosting view is added as a subview of and constrained to the cell's contentView via a call to setViewIfNeeded(view:)
  4. The hosting view's didMoveToSuperview function is called, but prior to this change, we did nothing in this function
  5. We return from collectionView(_:cellForItemAt:)
  6. UICollectionView calls the cell's preferredLayoutAttributesFitting: function, which returns an estimated size
  7. The hosting view's didMoveToWindow function is called, and we finally add our viewController as a child
  8. No additional sizing attempt is made by UICollectionViewFlowLayout or MagazineLayout

UICollectionViewCompositionalLayout recovers more gracefully, due to it more aggressively invalidating specific index paths, leading to more self-sizing requests. Apple also uses an insane number of private APIs in that layout, so it's hard to know if MagazineLayout could also "get lucky" and avoid this issue or not. This issue is Epoxy's fault anyways, so probably not worth worrying about.

One last note: Using the iOS 16+ UIHostingConfiguration type also completely avoids these issues, likely because they get their view setup timing correct (as one would expect).

How was it tested?

How did you verify that this change accomplished what you expected? Add more detail as needed.

  • Wrote automated tests
  • Built and ran on the iOS simulator
  • Built and ran on a device

Pull request checklist

All items in this checklist must be completed before a pull request will be reviewed.

  • Risky changes have been put behind a feature flag, e.g. CollectionViewConfiguration
  • Added a CHANGELOG.md entry in the "Unreleased" section for any library changes

@bryankeller bryankeller added the bug Something isn't working label Aug 17, 2023
@bryankeller bryankeller force-pushed the bk/fix-hosting-view-self-sizing-cell branch from aad22a3 to cd47344 Compare August 17, 2023 02:52
addViewControllerIfNeeded()
}
// Having our window set is an indicator that we should try adding our `viewController` as a
// child. We try this from a few other places to cover all of our bases.
Copy link
Contributor Author

@bryankeller bryankeller Aug 17, 2023

Choose a reason for hiding this comment

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

I love a good sports metaphor

image

@bryankeller bryankeller force-pushed the bk/fix-hosting-view-self-sizing-cell branch 5 times, most recently from b4d0bd7 to 4720bfe Compare August 17, 2023 03:52
Copy link
Contributor

@brynbodayle brynbodayle left a comment

Choose a reason for hiding this comment

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

Ah great find, and appreciate the detailed explanation! Totally agree.

@bryankeller bryankeller enabled auto-merge (squash) August 17, 2023 16:56
@bryankeller bryankeller force-pushed the bk/fix-hosting-view-self-sizing-cell branch from 4720bfe to 3f0ada1 Compare August 17, 2023 17:20
@bryankeller bryankeller merged commit 9d17ae9 into airbnb:master Aug 17, 2023
@bryankeller bryankeller deleted the bk/fix-hosting-view-self-sizing-cell branch August 17, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants