Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ @implementation RCTScrollViewComponentView {
__weak UIView *_contentView;

CGRect _prevFirstVisibleFrame;
CGPoint _prevContentOffset;
__weak UIView *_firstVisibleView;
NSInteger _firstVisibleViewTag;

Expand Down Expand Up @@ -708,6 +709,7 @@ - (void)prepareForRecycle
self.frame = oldFrame;
_contentView = nil;
_prevFirstVisibleFrame = CGRectZero;
_prevContentOffset = CGPointZero;
_firstVisibleView = nil;
_firstVisibleViewTag = 0;
_virtualViewContainerState = nil;
Expand Down Expand Up @@ -1077,6 +1079,8 @@ - (void)_prepareForMaintainVisibleScrollPosition
}
if (hasNewView || ii == _contentView.subviews.count - 1) {
_prevFirstVisibleFrame = subview.frame;
// A smaller content size can clamp the live offset before the adjustment.
_prevContentOffset = _scrollView.contentOffset;
_firstVisibleView = subview;
_firstVisibleViewTag = subview.tag;
break;
Expand Down Expand Up @@ -1120,9 +1124,9 @@ - (void)_adjustForMaintainVisibleContentPosition
if (horizontal) {
CGFloat deltaX = _firstVisibleView.frame.origin.x - _prevFirstVisibleFrame.origin.x;
if (ABS(deltaX) > 0.5) {
CGFloat x = _scrollView.contentOffset.x;
CGFloat x = _prevContentOffset.x;
[self _forceDispatchNextScrollEvent];
_scrollView.contentOffset = CGPointMake(_scrollView.contentOffset.x + deltaX, _scrollView.contentOffset.y);
_scrollView.contentOffset = CGPointMake(x + deltaX, _scrollView.contentOffset.y);

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.

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)

if (autoscrollThreshold) {
// If the offset WAS within the threshold of the start, animate to the start.
if (x <= autoscrollThreshold.value()) {
Expand All @@ -1134,9 +1138,9 @@ - (void)_adjustForMaintainVisibleContentPosition
CGRect newFrame = _firstVisibleView.frame;
CGFloat deltaY = newFrame.origin.y - _prevFirstVisibleFrame.origin.y;
if (ABS(deltaY) > 0.5) {
CGFloat y = _scrollView.contentOffset.y;
CGFloat y = _prevContentOffset.y;
[self _forceDispatchNextScrollEvent];
_scrollView.contentOffset = CGPointMake(_scrollView.contentOffset.x, _scrollView.contentOffset.y + deltaY);
_scrollView.contentOffset = CGPointMake(_scrollView.contentOffset.x, y + deltaY);
if (autoscrollThreshold) {
// If the offset WAS within the threshold of the start, animate to the start.
if (y <= autoscrollThreshold.value()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,65 @@

@interface RCTScrollViewComponentView (Tests)
- (void)_keyboardWillChangeFrame:(NSNotification *)notification;
- (void)_prepareForMaintainVisibleScrollPosition;
- (void)_adjustForMaintainVisibleContentPosition;
@end

@interface RCTScrollViewComponentViewTests : XCTestCase
@end

@implementation RCTScrollViewComponentViewTests

- (void)testMaintainVisibleContentPositionAfterVerticalShrink
{
RCTScrollViewComponentView *view = [[RCTScrollViewComponentView alloc] initWithFrame:CGRectMake(0, 0, 100, 100)];
auto props = std::make_shared<ScrollViewProps>();
props->maintainVisibleContentPosition = facebook::react::ScrollViewMaintainVisibleContentPosition{};
[view updateProps:props oldProps:ScrollViewShadowNode::defaultSharedProps()];

RCTViewComponentView *contentView = [[RCTViewComponentView alloc] initWithFrame:CGRectMake(0, 0, 100, 1000)];
[view mountChildComponentView:contentView index:0];
UIView *anchor = [[UIView alloc] initWithFrame:CGRectMake(0, 800, 100, 40)];
anchor.tag = 42;
[contentView addSubview:anchor];

view.scrollView.contentSize = CGSizeMake(100, 1000);
view.scrollView.contentOffset = CGPointMake(0, 800);
[view _prepareForMaintainVisibleScrollPosition];

anchor.frame = CGRectMake(0, 300, 100, 40);
view.scrollView.contentSize = CGSizeMake(100, 400);
view.scrollView.contentOffset = CGPointZero;
[view _adjustForMaintainVisibleContentPosition];

XCTAssertEqualWithAccuracy(view.scrollView.contentOffset.y, 300, 0.5);
}

- (void)testMaintainVisibleContentPositionAfterHorizontalShrink
{
RCTScrollViewComponentView *view = [[RCTScrollViewComponentView alloc] initWithFrame:CGRectMake(0, 0, 100, 100)];
auto props = std::make_shared<ScrollViewProps>();
props->maintainVisibleContentPosition = facebook::react::ScrollViewMaintainVisibleContentPosition{};
[view updateProps:props oldProps:ScrollViewShadowNode::defaultSharedProps()];

RCTViewComponentView *contentView = [[RCTViewComponentView alloc] initWithFrame:CGRectMake(0, 0, 1000, 100)];
[view mountChildComponentView:contentView index:0];
UIView *anchor = [[UIView alloc] initWithFrame:CGRectMake(800, 0, 40, 100)];
anchor.tag = 42;
[contentView addSubview:anchor];

view.scrollView.contentSize = CGSizeMake(1000, 100);
view.scrollView.contentOffset = CGPointMake(800, 0);
[view _prepareForMaintainVisibleScrollPosition];

anchor.frame = CGRectMake(300, 0, 40, 100);
view.scrollView.contentSize = CGSizeMake(400, 100);
view.scrollView.contentOffset = CGPointZero;
[view _adjustForMaintainVisibleContentPosition];

XCTAssertEqualWithAccuracy(view.scrollView.contentOffset.x, 300, 0.5);
}

- (void)testAutomaticallyAdjustKeyboardInsetsAcrossRecycling
{
RCTScrollViewComponentView *view = [[RCTScrollViewComponentView alloc] initWithFrame:CGRectMake(0, 0, 100, 100)];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ internal class MaintainVisibleScrollPositionHelper<ScrollViewT>(
var config: Config? = null
private var firstVisibleViewRef: WeakReference<View>? = null
private var prevFirstVisibleFrame: Rect? = null
private var prevScrollOffset: Int? = null

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.

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?

private var isListening = false

private val contentView: ReactViewGroup?
Expand Down Expand Up @@ -91,6 +92,7 @@ internal class MaintainVisibleScrollPositionHelper<ScrollViewT>(
val config = config ?: return
val firstVisibleViewRef = firstVisibleViewRef ?: return
val prevFirstVisibleFrame = prevFirstVisibleFrame ?: return
val prevScrollOffset = prevScrollOffset ?: return
val firstVisibleView = firstVisibleViewRef.get() ?: return
val scrollView = scrollView ?: return

Expand All @@ -100,7 +102,7 @@ internal class MaintainVisibleScrollPositionHelper<ScrollViewT>(
if (horizontal) {
val deltaX = newFrame.left - prevFirstVisibleFrame.left
if (deltaX != 0) {
val scrollX = scrollView.scrollX
val scrollX = prevScrollOffset
scrollView.scrollToPreservingMomentum(scrollX + deltaX, scrollView.scrollY)
this.prevFirstVisibleFrame = newFrame
if (config.autoScrollToTopThreshold != null && scrollX <= config.autoScrollToTopThreshold) {
Expand All @@ -110,7 +112,7 @@ internal class MaintainVisibleScrollPositionHelper<ScrollViewT>(
} else {
val deltaY = newFrame.top - prevFirstVisibleFrame.top
if (deltaY != 0) {
val scrollY = scrollView.scrollY
val scrollY = prevScrollOffset
scrollView.scrollToPreservingMomentum(scrollView.scrollX, scrollY + deltaY)
this.prevFirstVisibleFrame = newFrame
if (config.autoScrollToTopThreshold != null && scrollY <= config.autoScrollToTopThreshold) {
Expand Down Expand Up @@ -138,6 +140,8 @@ internal class MaintainVisibleScrollPositionHelper<ScrollViewT>(
val frame = Rect()
child.getHitRect(frame)
prevFirstVisibleFrame = frame
// A smaller content size can clamp the live offset before didMountItems.
prevScrollOffset = currentScroll
break
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

package com.facebook.react.views.scroll

import android.content.Context
import android.view.View
import android.widget.FrameLayout
import com.facebook.react.bridge.UIManager
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests
import com.facebook.react.views.scroll.ReactScrollViewHelper.HasScrollEventThrottle
import com.facebook.react.views.scroll.ReactScrollViewHelper.HasSmoothScroll
import com.facebook.react.views.view.ReactViewGroup
import org.assertj.core.api.Assertions.assertThat
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.kotlin.mock
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment

@RunWith(RobolectricTestRunner::class)
class MaintainVisibleScrollPositionHelperTest {
private lateinit var context: Context
private val uiManager: UIManager = mock()

@Before
fun setUp() {
ReactNativeFeatureFlagsForTests.setUp()
context = RuntimeEnvironment.getApplication()
}

@Test
fun shrinkingContentAdjustsFromOffsetBeforeLayoutClamp() {
val scrollView = TestScrollView(context)
val content = ReactViewGroup(context)
val anchor = View(context)
content.addView(anchor)
scrollView.addView(content)
anchor.layout(0, 900, 100, 1000)
scrollView.scrollTo(0, 900)

val helper = MaintainVisibleScrollPositionHelper(scrollView, horizontal = false)
helper.config = MaintainVisibleScrollPositionHelper.Config(0, null)
helper.willMountItems(uiManager)

anchor.layout(0, 300, 100, 400)
scrollView.scrollTo(0, 100) // Layout clamped the old offset before didMountItems.
helper.didMountItems(uiManager)

assertThat(scrollView.requestedY).isEqualTo(300)
}

@Test
fun shrinkingHorizontalContentAdjustsFromOffsetBeforeLayoutClamp() {
val scrollView = TestScrollView(context)
val content = ReactViewGroup(context)
val anchor = View(context)
content.addView(anchor)
scrollView.addView(content)
anchor.layout(900, 0, 1000, 100)
scrollView.scrollTo(900, 0)

val helper = MaintainVisibleScrollPositionHelper(scrollView, horizontal = true)
helper.config = MaintainVisibleScrollPositionHelper.Config(0, null)
helper.willMountItems(uiManager)

anchor.layout(300, 0, 400, 100)
scrollView.scrollTo(100, 0)
helper.didMountItems(uiManager)

assertThat(scrollView.requestedX).isEqualTo(300)
}

@Test
fun growingContentStillAdjustsFromOffsetBeforeMount() {
val scrollView = TestScrollView(context)
val content = ReactViewGroup(context)
val anchor = View(context)
content.addView(anchor)
scrollView.addView(content)
anchor.layout(0, 300, 100, 400)
scrollView.scrollTo(0, 300)

val helper = MaintainVisibleScrollPositionHelper(scrollView, horizontal = false)
helper.config = MaintainVisibleScrollPositionHelper.Config(0, null)
helper.willMountItems(uiManager)

anchor.layout(0, 900, 100, 1000)
helper.didMountItems(uiManager)

assertThat(scrollView.requestedY).isEqualTo(900)
}

private class TestScrollView(context: Context) :
FrameLayout(context), HasScrollEventThrottle, HasSmoothScroll {
override var scrollEventThrottle: Int = 0
override var lastScrollDispatchTime: Long = 0
var requestedX: Int? = null
var requestedY: Int? = null

override fun reactSmoothScrollTo(x: Int, y: Int) {
scrollTo(x, y)
}

override fun scrollToPreservingMomentum(x: Int, y: Int) {
requestedX = x
requestedY = y
scrollTo(x, y)
}
}
}
12 changes: 12 additions & 0 deletions packages/rn-tester/RNTesterPods.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
79B29C2E2E607A99007612A5 /* SceneDelegate.mm in Sources */ = {isa = PBXBuildFile; fileRef = 79B29C2D2E607A99007612A5 /* SceneDelegate.mm */; };
8145AE06241172D900A3F8DA /* LaunchScreen.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = 8145AE05241172D900A3F8DA /* LaunchScreen.storyboard */; };
832F45BB2A8A6E1F0097B4E6 /* SwiftTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 832F45BA2A8A6E1F0097B4E6 /* SwiftTest.swift */; };
86178A1D4F95F48D0DC933E2 /* RCTScrollViewComponentViewTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 6250ED894CD7F3B5A761FE63 /* RCTScrollViewComponentViewTests.mm */; };
A975CA6C2C05EADF0043F72A /* RCTNetworkTaskTests.m in Sources */ = {isa = PBXBuildFile; fileRef = A975CA6B2C05EADE0043F72A /* RCTNetworkTaskTests.m */; };
C175B6D9ED9336FB66637943 /* libPods-RNTester.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 4C706D402EE4AF9BE838CBA9 /* libPods-RNTester.a */; };
CD10C7A5290BD4EB0033E1ED /* RCTEventEmitterTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CD10C7A4290BD4EB0033E1ED /* RCTEventEmitterTests.m */; };
Expand Down Expand Up @@ -93,6 +94,7 @@
4C706D402EE4AF9BE838CBA9 /* libPods-RNTester.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-RNTester.a"; sourceTree = BUILT_PRODUCTS_DIR; };
51BC9297B6C3163C14532020 /* Pods-RNTester.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-RNTester.release.xcconfig"; path = "Target Support Files/Pods-RNTester/Pods-RNTester.release.xcconfig"; sourceTree = "<group>"; };
5C60EB1B226440DB0018C04F /* AppDelegate.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = AppDelegate.mm; path = RNTester/AppDelegate.mm; sourceTree = "<group>"; };
6250ED894CD7F3B5A761FE63 /* RCTScrollViewComponentViewTests.mm */ = {isa = PBXFileReference; includeInIndex = 1; name = RCTScrollViewComponentViewTests.mm; path = "../react-native/React/Tests/Mounting/RCTScrollViewComponentViewTests.mm"; sourceTree = "<group>"; };
79B29C2C2E607A99007612A5 /* SceneDelegate.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SceneDelegate.h; path = RNTester/SceneDelegate.h; sourceTree = "<group>"; };
79B29C2D2E607A99007612A5 /* SceneDelegate.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; name = SceneDelegate.mm; path = RNTester/SceneDelegate.mm; sourceTree = "<group>"; };
8145AE05241172D900A3F8DA /* LaunchScreen.storyboard */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = file.storyboard; name = LaunchScreen.storyboard; path = RNTester/LaunchScreen.storyboard; sourceTree = "<group>"; };
Expand Down Expand Up @@ -267,6 +269,14 @@
name = Frameworks;
sourceTree = "<group>";
};
46F51AC598EF6515ADF3A7D9 /* LocalScrollViewTests */ = {
isa = PBXGroup;
children = (
6250ED894CD7F3B5A761FE63 /* RCTScrollViewComponentViewTests.mm */,
);
name = LocalScrollViewTests;
sourceTree = "<group>";
};
680759612239798500290469 /* Fabric */ = {
isa = PBXGroup;
children = (
Expand All @@ -284,6 +294,7 @@
83CBBA001A601CBA00E9B192 /* Products */,
2DE7E7D81FB2A4F3009E225D /* Frameworks */,
E23BD6487B06BD71F1A86914 /* Pods */,
46F51AC598EF6515ADF3A7D9 /* LocalScrollViewTests */,
);
indentWidth = 2;
sourceTree = "<group>";
Expand Down Expand Up @@ -774,6 +785,7 @@
E7DB20EB22B2BAA6005AC45F /* RCTConvert_YGValueTests.m in Sources */,
E7DB20E922B2BAA6005AC45F /* RCTComponentPropsTests.m in Sources */,
E7DB20D822B2BAA6005AC45F /* RCTJSONTests.m in Sources */,
86178A1D4F95F48D0DC933E2 /* RCTScrollViewComponentViewTests.mm in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
};
Expand Down
Loading