Skip to content

Commit

Permalink
rename and extend new maintain visible content position feature
Browse files Browse the repository at this point in the history
Summary:
Builds off of cae7179

- Make the prop a dictionary for more configuration options
- Rename `maintainPositionAtOrBeyondIndex` -> `maintainVisibleContentPosition` + `minIndexForVisible`
- Add autoscroll threshold feature

Given the async native of RN JS and background layout, there is no way to trigger the scrollTo from JS without risking a delay, so we add the feature in native code.

== Test Plan ==
ScrollViewExample:
https://youtu.be/pmY8pxC9PRs

Reviewed By: shergin

Differential Revision: D6729160

fbshipit-source-id: 70f9bae460ce84567857a4f696da78ce9b3b834c
  • Loading branch information
sahrens authored and facebook-github-bot committed Jan 18, 2018
1 parent 7e7d00a commit 65184ec
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 22 deletions.
31 changes: 23 additions & 8 deletions Libraries/Components/ScrollView/ScrollView.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,18 +234,33 @@ const ScrollView = createReactClass({
*/
keyboardShouldPersistTaps: PropTypes.oneOf(['always', 'never', 'handled', false, true]),
/**
* When non-null, the scroll view will adjust the scroll position so that the content at or
* beyond the specified index that is currently visible will not change position. This is useful
* for lists that are loading content in both directions, e.g. a chat thread, where new messages
* coming in might otherwise cause the scroll position to jump. A value of 1 can be used to skip
* a spinner that does not need to maintain position. The default value is null.
* When set, the scroll view will adjust the scroll position so that the first child that is
* currently visible and at or beyond `minIndexForVisible` will not change position. This is
* useful for lists that are loading content in both directions, e.g. a chat thread, where new
* messages coming in might otherwise cause the scroll position to jump. A value of 0 is common,
* but other values such as 1 can be used to skip loading spinners or other content that should
* not maintain position.
*
* Caveat: reordering elements in the scrollview with this enabled will probably cause jumpiness
* and jank. It can be fixed, but there are currently no plans to do so.
* The optional `autoscrollToTopThreshold` can be used to make the content automatically scroll
* to the top after making the adjustment if the user was within the threshold of the top before
* the adjustment was made. This is also useful for chat-like applications where you want to see
* new messages scroll into place, but not if the user has scrolled up a ways and it would be
* disruptive to scroll a bunch.
*
* Caveat 1: Reordering elements in the scrollview with this enabled will probably cause
* jumpiness and jank. It can be fixed, but there are currently no plans to do so. For now,
* don't re-order the content of any ScrollViews or Lists that use this feature.
*
* Caveat 2: This simply uses `contentOffset` and `frame.origin` in native code to compute
* visibility. Occlusion, transforms, and other complexity won't be taken into account as to
* whether content is "visible" or not.
*
* @platform ios
*/
maintainPositionAtOrBeyondIndex: PropTypes.number,
maintainVisibleContentPosition: PropTypes.shape({
minIndexForVisible: PropTypes.number.isRequired,
autoscrollToTopThreshold: PropTypes.number,
}),
/**
* The maximum allowed zoom scale. The default value is 1.0.
* @platform ios
Expand Down
14 changes: 10 additions & 4 deletions RNTester/js/ScrollViewExample.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ if (Platform.OS === 'ios') {
exports.examples.push({
title: '<ScrollView> smooth bi-directional content loading\n',
description:
'The `maintainPositionAtOrBeyondIndex` prop allows insertions to either end of the content ' +
'The `maintainVisibleContentPosition` prop allows insertions to either end of the content ' +
'without causing the visible content to jump. Re-ordering is not supported.',
render: function() {
let itemCount = 6;
Expand All @@ -146,7 +146,10 @@ if (Platform.OS === 'ios') {
<View>
<ScrollView
automaticallyAdjustContentInsets={false}
maintainPositionAtOrBeyondIndex={1}
maintainVisibleContentPosition={{
minIndexForVisible: 1,
autoscrollToTopThreshold: 10,
}}
style={styles.scrollView}>
<ActivityIndicator style={{height: 40}} />
{this.state.items.map(item =>
Expand All @@ -156,9 +159,12 @@ if (Platform.OS === 'ios') {
<ScrollView
horizontal={true}
automaticallyAdjustContentInsets={false}
maintainPositionAtOrBeyondIndex={1}
maintainVisibleContentPosition={{
minIndexForVisible: 1,
autoscrollToTopThreshold: 10,
}}
style={[styles.scrollView, styles.horizontalScrollView]}>
<ActivityIndicator style={{height: 40}} />
<ActivityIndicator style={{width: 40}} />
{this.state.items.map(item =>
React.cloneElement(item, {key: item.props.msg, style: null}),
)}
Expand Down
2 changes: 1 addition & 1 deletion React/Views/ScrollView/RCTScrollView.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
@property (nonatomic, assign) BOOL DEPRECATED_sendUpdatedChildFrames;
@property (nonatomic, assign) NSTimeInterval scrollEventThrottle;
@property (nonatomic, assign) BOOL centerContent;
@property (nonatomic, copy) NSNumber *maintainPositionAtOrBeyondIndex;
@property (nonatomic, copy) NSDictionary *maintainVisibleContentPosition;
@property (nonatomic, assign) int snapToInterval;
@property (nonatomic, copy) NSString *snapToAlignment;

Expand Down
29 changes: 21 additions & 8 deletions React/Views/ScrollView/RCTScrollView.m
Original file line number Diff line number Diff line change
Expand Up @@ -911,16 +911,16 @@ - (void)updateContentOffsetIfNeeded
}
}

// maintainPositionAtOrBeyondIndex is used to allow seamless loading of content from both ends of
// maintainVisibleContentPosition is used to allow seamless loading of content from both ends of
// the scrollview without the visible content jumping in position.
- (void)setMaintainPositionAtOrBeyondIndex:(NSNumber *)maintainPositionAtOrBeyondIndex
- (void)setMaintainVisibleContentPosition:(NSDictionary *)maintainVisibleContentPosition
{
if (maintainPositionAtOrBeyondIndex != nil) {
if (maintainVisibleContentPosition != nil && _maintainVisibleContentPosition == nil) {
[_eventDispatcher.bridge.uiManager.observerCoordinator addObserver:self];
} else {
} else if (maintainVisibleContentPosition == nil && _maintainVisibleContentPosition != nil) {
[_eventDispatcher.bridge.uiManager.observerCoordinator removeObserver:self];
}
_maintainPositionAtOrBeyondIndex = maintainPositionAtOrBeyondIndex;
_maintainVisibleContentPosition = maintainVisibleContentPosition;
}

#pragma mark - RCTUIManagerObserver
Expand All @@ -930,7 +930,7 @@ - (void)uiManagerWillPerformMounting:(RCTUIManager *)manager
RCTAssertUIManagerQueue();
[manager prependUIBlock:^(RCTUIManager *uiManager, NSDictionary<NSNumber *, UIView *> *viewRegistry) {
BOOL horz = [self isHorizontal:self->_scrollView];
NSUInteger minIdx = [self->_maintainPositionAtOrBeyondIndex integerValue];
NSUInteger minIdx = [self->_maintainVisibleContentPosition[@"minIndexForVisible"] integerValue];
for (NSUInteger ii = minIdx; ii < self->_contentView.subviews.count; ++ii) {
// Find the first entirely visible view. This must be done after we update the content offset
// or it will tend to grab rows that were made visible by the shift in position
Expand All @@ -946,9 +946,10 @@ - (void)uiManagerWillPerformMounting:(RCTUIManager *)manager
}
}];
[manager addUIBlock:^(RCTUIManager *uiManager, NSDictionary<NSNumber *, UIView *> *viewRegistry) {
if (self->_maintainPositionAtOrBeyondIndex == nil) {
if (self->_maintainVisibleContentPosition == nil) {
return; // The prop might have changed in the previous UIBlocks, so need to abort here.
}
NSNumber *autoscrollThreshold = self->_maintainVisibleContentPosition[@"autoscrollToTopThreshold"];
// TODO: detect and handle/ignore re-ordering
if ([self isHorizontal:self->_scrollView]) {
CGFloat deltaX = self->_firstVisibleView.frame.origin.x - self->_prevFirstVisibleFrame.origin.x;
Expand All @@ -957,15 +958,27 @@ - (void)uiManagerWillPerformMounting:(RCTUIManager *)manager
self->_scrollView.contentOffset.x + deltaX,
self->_scrollView.contentOffset.y
);
if (autoscrollThreshold != nil) {
// If the offset WAS within the threshold of the start, animate to the start.
if (self->_scrollView.contentOffset.x - deltaX <= [autoscrollThreshold integerValue]) {
[self scrollToOffset:CGPointMake(0, self->_scrollView.contentOffset.y) animated:YES];
}
}
}
} else {
CGRect newFrame = self->_firstVisibleView.frame;
CGFloat deltaY = newFrame.origin.y - self->_prevFirstVisibleFrame.origin.y;
if (ABS(deltaY) > 0.1 || deltaY != 0.0) {
if (ABS(deltaY) > 0.1) {
self->_scrollView.contentOffset = CGPointMake(
self->_scrollView.contentOffset.x,
self->_scrollView.contentOffset.y + deltaY
);
if (autoscrollThreshold != nil) {
// If the offset WAS within the threshold of the start, animate to the start.
if (self->_scrollView.contentOffset.y - deltaY <= [autoscrollThreshold integerValue]) {
[self scrollToOffset:CGPointMake(self->_scrollView.contentOffset.x, 0) animated:YES];
}
}
}
}
}];
Expand Down
2 changes: 1 addition & 1 deletion React/Views/ScrollView/RCTScrollViewManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ - (UIView *)view
RCT_EXPORT_VIEW_PROPERTY(bouncesZoom, BOOL)
RCT_EXPORT_VIEW_PROPERTY(canCancelContentTouches, BOOL)
RCT_EXPORT_VIEW_PROPERTY(centerContent, BOOL)
RCT_EXPORT_VIEW_PROPERTY(maintainPositionAtOrBeyondIndex, NSNumber)
RCT_EXPORT_VIEW_PROPERTY(maintainVisibleContentPosition, NSDictionary)
RCT_EXPORT_VIEW_PROPERTY(automaticallyAdjustContentInsets, BOOL)
RCT_EXPORT_VIEW_PROPERTY(decelerationRate, CGFloat)
RCT_EXPORT_VIEW_PROPERTY(directionalLockEnabled, BOOL)
Expand Down

24 comments on commit 65184ec

@sahrens
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @janicduplessis, @erictraut, @browniefed: y'all might be interested in this. Working on Android now.

@sahrens
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you have any feedback on the API or anything.

@janicduplessis
Copy link
Contributor

Choose a reason for hiding this comment

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

@sahrens Nice, I'll have to implement proper pagination on comment threads to make use of this now 😀

@sahrens
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that depending on the application, you might want to use LayoutAnimation instead - e.g. tapping "show more" explicitly vs. automatically loading on scroll.

@artemdanylenko
Copy link

Choose a reason for hiding this comment

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

@sahrens any plans to integrate this into VirtualizedList somehow?

@sahrens
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already works - FlatList and friends use ScrollView under the hood so almost all ScrollView features work out of the box.

@artemdanylenko
Copy link

Choose a reason for hiding this comment

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

Thanks! Will have to try it.

@guysegal
Copy link

Choose a reason for hiding this comment

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

Is there a plan to implement it on Android soon?

@guysegal
Copy link

Choose a reason for hiding this comment

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

@sahrens this is amazing, we tried to implement something similar but with no success..
Are you actively working on the Android solution? Is there a way to assist?

@giantss
Copy link

@giantss giantss commented on 65184ec Jun 8, 2018

Choose a reason for hiding this comment

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

@sahrens Thank you very much for your solution, A better experience would be for the user to pull down to the top to load the history chat and the message won't beat, like onEndReached the effect of overlapping the bottom of the data is the same.

@marsonmao
Copy link

Choose a reason for hiding this comment

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

Looking forward to the android version!!

@bishalshr
Copy link

@bishalshr bishalshr commented on 65184ec Oct 4, 2018

Choose a reason for hiding this comment

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

@sahrens
maintainVisibleContentPosition={{ minIndexForVisible: 0, }} isn't working for prepending the first set of data. It works well after that.

@chinalwb
Copy link

@chinalwb chinalwb commented on 65184ec Nov 5, 2018

Choose a reason for hiding this comment

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

@sahrens I am running into the same issue with @bishalshr , I had reported the issue [FlatList] The maintainVisibleContentPosition doesn't work for the 1st time change. #19621 but nobody cares my complain.

@jbolter
Copy link

Choose a reason for hiding this comment

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

@sahrens what can we do to help get this functionality into Android? This is a life-saver for the scrolling issue that we've been facing but we need it on Android as well!

@enahum
Copy link
Contributor

@enahum enahum commented on 65184ec Nov 23, 2018

Choose a reason for hiding this comment

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

yeah actually this feature for android will be amazing, now our iOS app is using it and is really missed in android

@nsantacruz
Copy link

Choose a reason for hiding this comment

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

@sahrens if you could just point us to the relevant code in Android then I'd be happy to take a look at it and see if I can make a PR. I'm just not sure where to start.

@jbolter
Copy link

@jbolter jbolter commented on 65184ec Jan 6, 2019

Choose a reason for hiding this comment

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

Happy new year @sahrens ! We're digging in on figuring this out for Android but if you could give any of us any pointers on what you have already found that would be amazing. Thanks!

@sahrens
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry guys, there is no work planned here and I haven't looked into an Android solution. @janicduplessis or @kmagiera might be able to help out on Android though? I think they might have the most context on Android scrolling stuff.

@Strate
Copy link

@Strate Strate commented on 65184ec Jun 7, 2019

Choose a reason for hiding this comment

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

Hello all, do anyone resolved this for android somehow?

@DaveLomber
Copy link

Choose a reason for hiding this comment

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

+++ Need this for Android

@socceroos
Copy link

Choose a reason for hiding this comment

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

Would love to see this functionality come to Android as well. Is anyone working on this currently?

@nsantacruz
Copy link

Choose a reason for hiding this comment

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

They clearly stated that this is not a priority for Facebook. I think if anyone wants to see this done, they'll need to work on a pull request.

@friedolinfoerder
Copy link

Choose a reason for hiding this comment

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

Is there any workaround for prepending the first set of data (#19621) ?
Did anybody start to implement a solution for android?

@lsdimagine
Copy link

Choose a reason for hiding this comment

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

@friedolinfoerder @chinalwb I got the #19621 on RN 0.60 too... Did you find workaround? @sahrens do you have plan to fix it?

Please sign in to comment.