Conversation
|
Hi @aryan1306! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| CGFloat x = _prevContentOffset.x; | ||
| [self _forceDispatchNextScrollEvent]; | ||
| _scrollView.contentOffset = CGPointMake(_scrollView.contentOffset.x + deltaX, _scrollView.contentOffset.y); | ||
| _scrollView.contentOffset = CGPointMake(x + deltaX, _scrollView.contentOffset.y); |
There was a problem hiding this comment.
With this, it's possible to have the content offset greater than the height of the scroll view and you end up overscrolling. The target offset should be clamped to the bounds of the scroll view. You can use a helper like this to precompute the target point:
static CGPoint RCTClampMaintainVisibleContentOffset(
UIScrollView *scrollView,
CGPoint offset)
{
UIEdgeInsets insets = scrollView.adjustedContentInset;
CGFloat minX = -insets.left;
CGFloat maxX = fmax(
minX,
scrollView.contentSize.width -
scrollView.bounds.size.width +
insets.right);
CGFloat minY = -insets.top;
CGFloat maxY = fmax(
minY,
scrollView.contentSize.height -
scrollView.bounds.size.height +
insets.bottom);
return CGPointMake(
fmin(fmax(offset.x, minX), maxX),
fmin(fmax(offset.y, minY), maxY));
}and then the correction becomes
CGPoint targetOffset = CGPointMake(
x + deltaX,
_scrollView.contentOffset.y);
[self _forceDispatchNextScrollEvent];
_scrollView.contentOffset =
RCTClampMaintainVisibleContentOffset(_scrollView, targetOffset);(same for vertical as well)
There was a problem hiding this comment.
I added RCTClampMaintainVisibleContentOffset as you suggested and use it for both the horizontal and vertical adjustments. I also added two tests where the target lands past the end of the content. Both fail without the clamp and pass with it.
| var config: Config? = null | ||
| private var firstVisibleViewRef: WeakReference<View>? = null | ||
| private var prevFirstVisibleFrame: Rect? = null | ||
| private var prevScrollOffset: Int? = null |
There was a problem hiding this comment.
To avoid multiple offset states, could you just compute this on the fly in onLayoutChange and only call scrollTo when the scroll position is greater than the allowable offset?
There was a problem hiding this comment.
Thanks for the suggestion! I dug into this, and ReactScrollView.onLayoutChange already does exactly that check. It's actually what causes this bug.
When the content shrinks, things happen in this order:
willMountItems: we record the anchor.- The content view gets its new layout, and
onLayoutChangeclampsscrollYto the new max (750 → 200 in the test). didMountItems: we add the anchor's delta to that already-clamped value (200 − 600), and the list jumps to the top.
So we need the offset from before step 2. I also tried it the other way: no stored offset, and skip the clamp while MVCP is active. That fixed this case, but when rows below the anchor shrink, nothing pulls the view back and it overscrolls.
To keep the extra state small, I've limited it to the vertical path. ReactHorizontalScrollView.onLayoutChange doesn't clamp, so horizontal never needed it. I also rewrote the tests to use a real ReactScrollView, so they go through the actual clamp.
iOS: clamp the maintainVisibleContentPosition target to the scrollable range so restoring the pre-mount offset after a shrink cannot overscroll. Android: only the vertical path needs the pre-mount offset, because ReactScrollView.onLayoutChange clamps scrollY before didMountItems. ReactHorizontalScrollView does not clamp, so horizontal reads scrollX live as before. Tests now drive a real ReactScrollView so they go through that clamp.
Summary:
Fixes #58578.
When ScrollView content shrinks, layout can clamp the current scroll offset before
maintainVisibleContentPositionadjusts the visible anchor. Applying the anchor delta to that clamped offset can move the list to the top.Capture the scroll offset before mounting and use it when applying the anchor adjustment in Android and iOS Fabric. Add regression tests for vertical and horizontal shrinking content.
Changelog:
[GENERAL] [FIXED] - Preserve the visible ScrollView anchor when content shrinks.
Test Plan:
./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests com.facebook.react.views.scroll.MaintainVisibleScrollPositionHelperTest --no-daemon: all three tests passed.RNTesterUnitTestsscheme on an iPhone 17 Pro simulator: all three tests inRCTScrollViewComponentViewTestspassed, including the vertical and horizontal shrink regressions.