[iOS] Account for contentInset in Fabric maintainVisibleContentPosition autoscroll#56672
Open
enahum wants to merge 1 commit intofacebook:mainfrom
Open
Conversation
…rollView The Fabric implementation of `_adjustForMaintainVisibleContentPosition` did not account for `contentInset` when computing the autoscroll threshold comparison or the scroll target position, unlike the Paper implementation (`RCTScrollView.m`) which already handles this correctly. When a non-zero `contentInset` is set on a scroll view, two bugs occurred: 1. The threshold comparison used raw `contentOffset` instead of the inset-adjusted value, causing autoscroll to fire incorrectly. 2. The autoscroll target was hardcoded to `0` instead of `-inset`, causing the scroll view to jump to the wrong position. Fix: compute `bottomInset`/`leftInset` (respecting `isInverted`) and use them to adjust both the threshold comparison and the `scrollToOffset` target, matching the existing Paper logic in `RCTScrollView.m`. No behavior change when `contentInset` is zero.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Fabric implementation of
_adjustForMaintainVisibleContentPositioninRCTScrollViewComponentView.mmdoes not account forcontentInsetwhen computing the autoscroll threshold comparison or the scroll target position. The Paper implementation (RCTScrollView.m) already handles this correctly viabottomInset/leftInsetvariables. The Fabric port simply missed carrying this logic over.Root cause
Vertical (before):
Vertical (after, matching Paper):
Same pattern for the horizontal branch with
leftInset.Visible symptoms
On a scroll view with a non-zero
contentInset:contentOffsetinstead of the inset-adjusted value, causing autoscroll to fire when it should not (or fail to fire when it should).scrollToOffsetis called with0instead of-inset, causing the scroll view to jump to the wrong position.The same scroll view behaves correctly on Paper (Old Architecture) since
RCTScrollView.malready accounts forcontentInset.No behavior change when
contentInsetis zero.Changelog:
[iOS] [Fixed] - Account for contentInset in Fabric ScrollView maintainVisibleContentPosition autoscroll threshold and target
Test Plan
contentInset={{ bottom: 100 }},autoscrollToTopThreshold: 10: insert item at index 0 while at the bottom → stays at correct position, does not jump toy=0contentInset={{ top: 100 }},autoscrollToTopThreshold: 10: insert item at index 0 while at the bottom → stays atcontentOffset.y = -100, does not jump toy=0contentInset={{ left: 100 }}: equivalent checkscontentInset={{ right: 100 }}: equivalent checkscontentInsetall zeros: existing behavior unchangedyarn jest packages/react-native/Libraries/Components/ScrollView