-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement native inverted behaviors for ScrollView #8440
Conversation
Also pushed some of the platform agnostic changes to VirtualizedList in RN Core: facebook/react-native#32038 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO:
- Fix
scrollToIndex
for non-fixed-height items - Enable virtualization when
inverted={true}
and fix debug visual overlay
packages/@react-native-windows/virtualized-list/src/VirtualizedList.js
Outdated
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Microsoft.ReactNative.vcxproj.filters
Outdated
Show resolved
Hide resolved
cda08c1
to
e03a4d8
Compare
8adf563
to
927e209
Compare
// Windows-only: Invert scroll metrics when inverted prop is | ||
// set to retain monotonically increasing layout assumptions | ||
// in the direction of increasing scroll offsets. | ||
let scrollMetrics = this._scrollMetrics; | ||
if (this.props.inverted) { | ||
const { | ||
contentLength, | ||
dOffset, | ||
offset, | ||
velocity, | ||
visibleLength, | ||
} = scrollMetrics; | ||
scrollMetrics = { | ||
...scrollMetrics, | ||
dOffset: dOffset * -1, | ||
offset: contentLength - offset - visibleLength, | ||
velocity: velocity * -1, | ||
}; | ||
} | ||
return scrollMetrics; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we making these changes in parallel upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be upstreamed. This is a XAML specific approach for inverted lists. There are a handful of more general changes that I've upstreamed in facebook/react-native#32038.
return m_horizontal ? horizontalOffset > (scrollViewer.ScrollableWidth() - SCROLL_EPSILON) | ||
: verticalOffset > (scrollViewer.ScrollableHeight() - SCROLL_EPSILON); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to better understand this logic, and why we do not anchor when scrolling to the end. Is it to special case allowing new content to show up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we call the React ScrollView.scrollToEnd command, we presumably don't want anything to interrupt the scroll operation (except perhaps the user, but that's non-trivial to detect). This worked around a specific bug we saw where calling scrollToEnd
on the ScrollView would land at the second item in the list, rather than the first item in the list when the first item mounted while scrolling. Typically with VirtualizedList, when a new item is mounted at the head of the list data, one item will be unmounted from the tail of the list data, and I think that causes a view port shift when anchoring is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And actually, it wasn't just for the imperative ScrollView.scrollToEnd
command, I believe bottom edge anchoring was broken unless we disabled content anchoring as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the same bug happen for offset based scroll to index?
Seems like we kind of conceptually want a batch of "scrolling to target" to "target reached and list settled" where we do not want to anchor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my original implementation, I inverted the coordinate space entirely on the native side. So scrollTo
commands would be inverted and if the scroll operation was interrupted by layout, it would "adjust" for the new offset. This worked really well, but was completely incompatible with virtualization because while the ScrollViewer onScroll
event and scrollTo
command could be easily inverted, list virtualization relies too heavily on the onLayout
event, which we really cannot invert.
We could add a scrollToInverted
command that brought back this functionality so you could effectively scroll to any position in a layout dependent way and this scollToEnd
functionality would be replaced by scrollToInverted(0)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should fix the scrollToIndex
/ scrollToOffset
functionality in this PR though. As it is, scrollToIndex
is known to be unreliable by default (without using getItemLayout
)
@@ -1230,7 +1263,7 @@ class VirtualizedList extends React.PureComponent<Props, State> { | |||
this._fillRateHelper.computeBlankness( | |||
this.props, | |||
this.state, | |||
this._scrollMetrics, | |||
this._getScrollMetrics(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This smells a bit to me, to have cached scroll metrics be non-inverted, and have the accessor which inverts them. Easy to take in an upstream change which uses the raw variable, then have something acting differently.
Maybe we should update the cache in componentDidUpdate
if inverstion state changes, so that the only state we have is the correct one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed - I didn't spend a lot of time on this, but I was hoping to pull the scroll metrics and frame metrics out into a separate module that encapsulated these details so VirtualizedList could only get the scroll metrics via the accessor.
packages/@react-native-windows/virtualized-list/src/VirtualizedList.js
Outdated
Show resolved
Hide resolved
This is not super immediately clear, but What it basically means is that we can't assume its behavior only affects XAML. It is more of a staging package for upstream. We can do special behavior using |
// While this call fires too frequently resulting in unnecessary | ||
// calls to invalidate arrange, it is the only sure-fire way to call | ||
// InvalidateArrange any time any descendent layout changes. | ||
if (static_cast<ScrollViewShadowNode &>(nodeToUpdate).IsInverted()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider conditioning on whether we are anchoring instead of inverted, since there may be future cases with anchoring without inversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should implement this here. I think we should add a follow up PR that fixes the overflowAnchor
prop for arbitrary cases.
Bulk marking PRs without activity in the last 14 days as needing author feedback. This helps keep all active PRs visible on the first page of PRs. msftbot will close this PR in two weeks if the label is not removed. Please feel free to remove the label if you are still actively working on this. |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
Note, switching from |
This PR modifies ScrollViewManager and VirtualizedList to work around the limitations of the RN core approach to list inversion. Specifically, the RN core approach to list inversion uses scroll axis transforms to flip the list content. While this creates the visual appearance of an inverted list, scrolling the list via keyboard or mouse wheel results in inverted scroll direction. To accomplish native inverted scroll behaviors, we focused on four expected behaviors of inverted lists: 1. When content loads "above" the view port in an inverted list, the visual content must remain anchored (as if the content renders below it in a non-inverted list). 2. When scrolled to the "start" of an inverted list (in absolute terms, the bottom of the scroll view), content appended to the start of the list must scroll into view synchronously (i.e., no delay between content rendering and view port changing). 3. When content renders on screen, it must render from bottom to top, so render passes that take multiple frames do not appear to scroll to the start of the list. 4. When imperatively scrolling to the "start" of the content, we must always scroll to the latest content, even if the content rendered after the scroll-to-start animation already began. For 1., we leverage the XAML `CanBeScrollAnchor` property on each top-level item in the list view. While this is an imperfect solution (re-rendering of this content while in the view port can result in view port shifts as new content renders above), it is a good trade-off of performance and functionality. For 2., we leverage the XAML `HorizontalAnchorRatio` and `VerticalAnchorRatio` properties. XAML has a special case for inverted lists when setting these property values to `1.0`. It instructs XAML to synchronously scroll to and render new content when scrolled to the bottom edge of the ScrollViewer. For 3., we leverage Yoga's implementation of `flexDirection: column-reverse` and `flexDirection: row-reverse` to ensure content is rendered from bottom to top. For 4., we implemented `ScrollViewViewChanger` to continuously check if the target scroll offset has changed since starting an animated scroll-to-end operation. If the target scroll offset no longer matches the scrollable extent of the ScrollViewer, we update the target offset by calling `ChangeView` again. Fixes microsoft#4098
This PR modifies ScrollViewManager and VirtualizedList to work around the limitations of the RN core approach to list inversion. Specifically, the RN core approach to list inversion uses scroll axis transforms to flip the list content. While this creates the visual appearance of an inverted list, scrolling the list via keyboard or mouse wheel results in inverted scroll direction. To accomplish native inverted scroll behaviors, we focused on four expected behaviors of inverted lists: 1. When content loads "above" the view port in an inverted list, the visual content must remain anchored (as if the content renders below it in a non-inverted list). 2. When scrolled to the "start" of an inverted list (in absolute terms, the bottom of the scroll view), content appended to the start of the list must scroll into view synchronously (i.e., no delay between content rendering and view port changing). 3. When content renders on screen, it must render from bottom to top, so render passes that take multiple frames do not appear to scroll to the start of the list. 4. When imperatively scrolling to the "start" of the content, we must always scroll to the latest content, even if the content rendered after the scroll-to-start animation already began. For 1., we leverage the XAML `CanBeScrollAnchor` property on each top-level item in the list view. While this is an imperfect solution (re-rendering of this content while in the view port can result in view port shifts as new content renders above), it is a good trade-off of performance and functionality. For 2., we leverage the XAML `HorizontalAnchorRatio` and `VerticalAnchorRatio` properties. XAML has a special case for inverted lists when setting these property values to `1.0`. It instructs XAML to synchronously scroll to and render new content when scrolled to the bottom edge of the ScrollViewer. For 3., we leverage Yoga's implementation of `flexDirection: column-reverse` and `flexDirection: row-reverse` to ensure content is rendered from bottom to top. For 4., we implemented `ScrollViewViewChanger` to continuously check if the target scroll offset has changed since starting an animated scroll-to-end operation. If the target scroll offset no longer matches the scrollable extent of the ScrollViewer, we update the target offset by calling `ChangeView` again. Fixes microsoft#4098
We previously stopped development on the native inverted approach for Windows that would use anchoring and flexDirection to implement a truly reversed list (as opposed to a list flipped via transform) due to a bug where the anchoring would cause the view to "ride" to the top of the ScrollViewer when you scrolled into the virtualized tail spacer. This change introduces a new View prop called `overflowAnchor` and uses this prop to ensure that the tail spacer can never be anchored.
Summary: This PR modifies ScrollViewManager and VirtualizedList to work around the limitations of the RN core approach to list inversion. Specifically, the RN core approach to list inversion uses scroll axis transforms to flip the list content. While this creates the visual appearance of an inverted list, scrolling the list via keyboard or mouse wheel results in inverted scroll direction. To accomplish native inverted scroll behaviors, we focused on four expected behaviors of inverted lists: 1. When content loads "above" the view port in an inverted list, the visual content must remain anchored (as if the content renders below it in a non-inverted list). 2. When scrolled to the "start" of an inverted list (in absolute terms, the bottom of the scroll view), content appended to the start of the list must scroll into view synchronously (i.e., no delay between content rendering and view port changing). 3. When content renders on screen, it must render from bottom to top, so render passes that take multiple frames do not appear to scroll to the start of the list. 4. When imperatively scrolling to the "start" of the content, we must always scroll to the latest content, even if the content rendered after the scroll-to-start animation already began. For 1., we leverage the XAML `CanBeScrollAnchor` property on each top-level item in the list view. While this is an imperfect solution (re-rendering of this content while in the view port can result in view port shifts as new content renders above), it is a good trade-off of performance and functionality. For 2., we leverage the XAML `HorizontalAnchorRatio` and `VerticalAnchorRatio` properties. XAML has a special case for inverted lists when setting these property values to `1.0`. It instructs XAML to synchronously scroll to and render new content when scrolled to the bottom edge of the ScrollViewer. For 3., we leverage Yoga's implementation of `flexDirection: column-reverse` and `flexDirection: row-reverse` to ensure content is rendered from bottom to top. For 4., we implemented `ScrollViewViewChanger` to continuously check if the target scroll offset has changed since starting an animated scroll-to-end operation. If the target scroll offset no longer matches the scrollable extent of the ScrollViewer, we update the target offset by calling `ChangeView` again. Fixes microsoft#4098 Test Plan: See test plan in D42787560 Reviewers: shawndempsey, chpurrer, lefever, helenistic Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D42786593 Tasks: T140460683
I believe decision was that this PR will not be taken into RNW on Paper. Tagging with Needs Attention to confirm decision in triage and close. |
Discussed in PR triage. Decision to not take change for Paper architecture still stands. |
This PR modifies ScrollViewManager and VirtualizedList to work around the limitations of the RN core approach to list inversion. Specifically, the RN core approach to list inversion uses scroll axis transforms to flip the list content. While this creates the visual appearance of an inverted list, scrolling the list via keyboard or mouse wheel results in inverted scroll direction.
To accomplish native inverted scroll behaviors, we focused on four expected behaviors of inverted lists:
For 1., we leverage the XAML
CanBeScrollAnchor
property on each top-level item in the list view. While this is an imperfect solution (re-rendering of this content while in the view port can result in view port shifts as new content renders above), it is a good trade-off of performance and functionality.For 2., we leverage the XAML
HorizontalAnchorRatio
andVerticalAnchorRatio
properties. XAML has a special case for invertedlists when setting these property values to
1.0
. It instructs XAML to synchronously scroll to and render new content when scrolled to the bottom edge of the ScrollViewer.For 3., we leverage Yoga's implementation of
flexDirection: column-reverse
andflexDirection: row-reverse
to ensure content is rendered from bottom to top.For 4., we implemented
ScrollViewViewChanger
to continuously check if the target scroll offset has changed since starting an animated scroll-to-end operation. If the target scroll offset no longer matches the scrollable extent of the ScrollViewer, we update the target offset by callingChangeView
again.There are two known limitations to this change:
disableVirtualization={true}
when you setinverted={true}
debug={true}
, which is really only useful when virtualization is enabled.Fixes #4098
Testing
Note: in the video, I'm using the scroll wheel and the ScrollView is scrolling in the correct direction:
React.Native.Playground.Win32.2022-08-02.14-50-23.mp4
Microsoft Reviewers: Open in CodeFlow