Skip to content

Fix an issue where resizing scroll views could result in their content offset being changed erroneously#557

Merged
kyleve merged 4 commits intomainfrom
kve/fix-content-offset-shift
Jun 26, 2025
Merged

Fix an issue where resizing scroll views could result in their content offset being changed erroneously#557
kyleve merged 4 commits intomainfrom
kve/fix-content-offset-shift

Conversation

@kyleve
Copy link
Collaborator

@kyleve kyleve commented Jun 6, 2025

Bug

Simulator.Screen.Recording.-.iPad.10th.generation.-.2025-05-20.at.15.02.56.mp4

Notes

🐛 If you have a partial modal (or any other modal that resizes), and you resize the modal while the keyboard is focused and scrolled away from the top, to be taller (eg by adding content), the scroll view within would jump up towards the top.

Westin and I had dug in a while ago and thought it was just a UIKit bug, but it turns out it's an order of operations thing: You need to apply the new contentSize to the scroll view before you make its frame taller, otherwise UIKit's automatic contentOffset adjustment kicks you back towards the top.

To fix this, I've added a applyBeforeLayout addition to ViewDescription.Config, which will be applied before the LayoutAttributes (except on initial view allocation – we still apply the attributes first so the view's properties are sensical).

/// An array of update closures that are applied **after** the `LayoutAttributes` is applied to the view.
public var updates: [Update]

/// An array of update closures that are applied **before** the `LayoutAttributes` is applied to the view after
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Grammar is weird here

@kyleve
Copy link
Collaborator Author

kyleve commented Jun 8, 2025

POS integration passed, surprisingly! https://github.com/squareup/ios-register/pull/127207

@kyleve kyleve marked this pull request as ready for review June 8, 2025 22:47
@kyleve kyleve requested a review from a team as a code owner June 8, 2025 22:47
controller = NativeViewController(node: child)
child.layoutAttributes.apply(to: controller.view)
// So the view has a reasonable size during creation/allocation, do this afterwards.
child.viewDescription.applyBeforeLayout(to: controller.view)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@watt Thoughts on this? This feels a bit better – init is all in a performWithoutAnimation, and I'd rather the view have at least a sane frame/bounds before we call the apply method here since some views don't generally work well if their frame/bounds has zero size.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, it's totally possible to have a view end up with a zero frame, and that's normal enough I'd still call that "sane", so I would probably do applyBeforeLayout first just for consistency. but in practice I don't think it will matter much either way.

@kyleve kyleve changed the title [WIP] Fix an issue where resizing scroll views could result in their content offset being changed erroneously Fix an issue where resizing scroll views could result in their content offset being changed erroneously Jun 10, 2025
@maxg-square
Copy link
Contributor

You'll probably need to update against main and remove the legacy layout enum usage in the tests FYI.

controller = NativeViewController(node: child)
child.layoutAttributes.apply(to: controller.view)
// So the view has a reasonable size during creation/allocation, do this afterwards.
child.viewDescription.applyBeforeLayout(to: controller.view)
Copy link
Collaborator

Choose a reason for hiding this comment

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

well, it's totally possible to have a view end up with a zero frame, and that's normal enough I'd still call that "sane", so I would probably do applyBeforeLayout first just for consistency. but in practice I don't think it will matter much either way.

kyleve added 2 commits June 25, 2025 13:06
…t-shift

* origin/main:
  Fix multi-line link accessibility paths
  Added support for tabbing through links in `AttributedLabel`
  chore: release 6.0.0 (#560)
  feat!: Remove legacy layout (#556)
@kyleve kyleve enabled auto-merge (squash) June 26, 2025 18:07
@kyleve kyleve merged commit 83bfcef into main Jun 26, 2025
4 checks passed
@kyleve kyleve deleted the kve/fix-content-offset-shift branch June 26, 2025 18:13
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.

3 participants