Skip to content

Addressing an AutoScrollAction issue when using VerticalLayoutGravity.bottom#572

Merged
johnnewman-square merged 15 commits into
mainfrom
johnnewman/bug/bottom-gravity
May 9, 2025
Merged

Addressing an AutoScrollAction issue when using VerticalLayoutGravity.bottom#572
johnnewman-square merged 15 commits into
mainfrom
johnnewman/bug/bottom-gravity

Conversation

@johnnewman-square

@johnnewman-square johnnewman-square commented May 2, 2025

Copy link
Copy Markdown
Collaborator
  • Adjusting the underlying CollectionView's contentSize before auto-scrolling. This will scroll after the contentSize logic within CollectionView (here) has adjusted the offset.
  • Ensuring the items provided to the didPerform closure are synced with the visible items after scrolling.

Checklist

Please do the following before merging:

  • Unit tests.
  • Ensure any public-facing changes are reflected in the changelog. Include them in the Main section.

@johnnewman-square johnnewman-square force-pushed the johnnewman/bug/bottom-gravity branch from 488488c to a4db6bd Compare May 5, 2025 15:22
@johnnewman-square

johnnewman-square commented May 5, 2025

Copy link
Copy Markdown
Collaborator Author

Note: there are similar existing issues with AutoScrollAction.Pin sending stale visible items to its didPerform closure. I'll address that in a separate PR. Edit: autoscrolling Pin and OnInsertedItem behaviors have been consolidated in this PR.

@johnnewman-square johnnewman-square changed the title Addressing an autoScrollAction issue with bottom VerticalLayoutGravity Addressing an AutoScrollAction.scrollToItem issue when using VerticalLayoutGravity.bottom May 5, 2025
@johnnewman-square johnnewman-square force-pushed the johnnewman/bug/bottom-gravity branch from 586477f to be0e624 Compare May 5, 2025 16:55
@johnnewman-square johnnewman-square marked this pull request as ready for review May 5, 2025 17:04
Comment thread ListableUI/Sources/ListView/ListView.swift Outdated
Comment thread ListableUI/Sources/ListView/ListView.swift
@johnnewman-square johnnewman-square changed the title Addressing an AutoScrollAction.scrollToItem issue when using VerticalLayoutGravity.bottom Addressing an AutoScrollAction issue when using VerticalLayoutGravity.bottom May 6, 2025
Comment on lines -1303 to -1306
if animated {
stateObserver.onDidEndScrollingAnimation { state in
pin.didPerform(self.scrollPositionInfo)
}

@johnnewman-square johnnewman-square May 6, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Using self within this onDidEndScrollingAnimation { ... } closure was causing a ListView memory leak. This is fixed - the Pin and OnInsertedItem behaviors funnel through one function that uses state.positionInfo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh good find

/// asynchronously update the underlying `contentSize` as part of the initial layout,
/// moments after this method is executed. The list's `contentSize` is overridden to
/// keep the offset anchored to the bottom when using `VerticalLayoutGravity.bottom`.
collectionView.performBatchUpdates(nil)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you may actually want to call this one? For iOS 16 which still has the bug that code fixes. You can pass an empty CollectionViewChanges IIRC

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the info. I've pushed up a change in f82f3d8 to add a private performEmptyBatchUpdates() function that uses an empty CollectionViewChanges.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, thanks!

@kyleve kyleve left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One comment on which method to call, otherwise LGTM! Nice work

@johnnewman-square johnnewman-square force-pushed the johnnewman/bug/bottom-gravity branch from ef30570 to f82f3d8 Compare May 6, 2025 22:39

/// This protocol allows `ListView` to treat the `OnInsertedItem` and `Pin` behaviors
/// in a similar fashion.
public protocol Behavior {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't have any suggestions for better alternatives but Behavior feels a little weird here. Like it's almost more of a config? IDK

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback and good point - this protocol exists mostly for configuring the AutoScrollAction. I may go with AutoScrollAction.Configuration instead of AutoScrollAction.Behavior. Does Configuration feel like it fits a bit better?

@johnnewman-square johnnewman-square merged commit ababfc1 into main May 9, 2025
4 checks passed
@johnnewman-square johnnewman-square deleted the johnnewman/bug/bottom-gravity branch May 9, 2025 20:34
kyleve pushed a commit that referenced this pull request Jul 24, 2025
…-headerfooters

* origin/main:
  build: set up tuist (#584)
  Bumping versions to 16.3.0 (#589)
  Reset scroll position if list identifier changes (#588)
  Bumping versions to 16.2.0 (#587)
  Adding a completion handler to the `scrollToSection` API. (#585)
  Adding a programmatic scroll completion handler (#582)
  Remove iOS 19 (26) cap of the collection view first responder workaround (#581)
  Bumping versions to 16.1.0 (#583)
  Update for Blueprint 6.0.0 (#580)
  Bumping versions to 16.0.4. (#579)
  Update the first responder resignation workaround to be enabled by default and cap at < iOS 19 [UI-8849] (#578)
  Bumping versions to 16.0.3 (#577)
  Bottom gravity and autoscroll improvements (#576)
  Bumping versions to 16.0.2 (#574)
  Addressing an AutoScrollAction issue when using VerticalLayoutGravity.bottom (#572)
  release: Prepare 16.0.1 release (#569)
  Fix reordering crash introduced in 16.0 (#568)
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 this pull request may close these issues.

4 participants