[Fabric] Implement snapToStart, snapToEnd, snapToOffsets property for ScrollView #14800
Conversation
…ew fabric implementation (#14793) * Initial plan for issue * Add snapToStart property support to fabric ScrollView implementation Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Implement proper snapToStart using inertia modifiers Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Add e2e test cases for snapToStart property in ScrollView fabric implementation Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Change files * Run change generation, linting, and formatting Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Add snapshots for snapToStart e2e tests in e2e-test-app-fabric Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Remove codegen file changes Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Remove codegen file changes not needed Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Remove codegen file changes completely - restore to original state Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Fix pipeline linting issues by removing flow-check from problematic packages Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Add prerelease change file for snapToStart implementation Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Add missing change file for @office-iss/react-native-win32 package modifications Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Fix override validation error by removing testIDs from ScrollView example Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * fix and remove unwanted changes * fix * fix * Update just-task.js * Delete @office-iss-react-native-win32-6687c30d-253d-419e-967d-b8d3b3e299f4.json * Implement snapToOffsets property for ScrollView fabric implementation Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Implement snapToEnd property for ScrollView fabric implementation Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Fix compilation error: Convert std::vector to winrt iterable for ConfigurePositionInertiaModifiers Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Fix compilation error: Add explicit template parameter to winrt::single_threaded_vector Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com> * Update CompositionContextHelper.cpp * Update CompositionContextHelper.cpp --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements new snap properties (snapToStart, snapToEnd, and snapToOffsets) for the Fabric ScrollView component, enhancing scrolling behavior.
- Updates ScrollViewComponentView.cpp to call the new snap methods.
- Extends CompositionContextHelper.cpp to configure snap inertia modifiers based on the snap properties.
- Adds method definitions in CompositionSwitcher.idl and includes new e2e tests for the feature.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vnext/Microsoft.ReactNative/Fabric/Composition/ScrollViewComponentView.cpp | Updates property changes to use new snap methods. |
| vnext/Microsoft.ReactNative/Fabric/Composition/CompositionContextHelper.cpp | Implements snap functionality and inertia modifier configuration. |
| vnext/Microsoft.ReactNative/CompositionSwitcher.idl | Adds IDL definitions for the new snap properties. |
| change/react-native-windows-aa754bc8-86a7-4d0f-975d-ea69252b5b32.json | Adds e2e tests for snapToStart property. |
vnext/Microsoft.ReactNative/Fabric/Composition/CompositionContextHelper.cpp
Show resolved
Hide resolved
| // Collect all snap positions | ||
| std::vector<float> snapPositions; | ||
|
|
||
| if (!m_snapToOffsets.empty()) { |
There was a problem hiding this comment.
You dont need this if. You can just always use this code. The current logic wouldn't work if m_snapToOffsets is empty, and m_snapToEnd is set. You wouldn't add the end snap point.
Better to just keep it simpler and have one code path here.
| } | ||
|
|
||
| // Add all the offset positions | ||
| for (const auto &offset : m_snapToOffsets) { |
There was a problem hiding this comment.
I think this is potentially more efficient:
snapPositions.insert(snapPositions.end(), m_snapToOffsets.begin(), m_snapToOffsets.end());
|
|
||
| // When snapToOffsets is used, snapToEnd controls whether to include the end position | ||
| if (m_snapToEnd) { | ||
| const float maxPosition = m_horizontal ? std::max<float>(m_contentSize.x - m_visualSize.x, 0) |
There was a problem hiding this comment.
Since this snappoint is dependent on the size, you'll have to ensure that this function is rerun when the size changes.
There was a problem hiding this comment.
Actually rather than doing that, which sounds expensive. -- Maybe we should do a special InteractionTrackerInertiaRestingValue for the end snap point, which uses some references to the size so we dont need to constantly recalculate all of this.
| @@ -805,6 +805,23 @@ void ScrollViewComponentView::updateProps( | |||
| if (oldViewProps.zoomScale != newViewProps.zoomScale) { | |||
| m_scrollVisual.Scale({newViewProps.zoomScale, newViewProps.zoomScale, newViewProps.zoomScale}); | |||
| } | |||
|
|
|||
| if (!oldProps || oldViewProps.snapToStart != newViewProps.snapToStart) { | |||
There was a problem hiding this comment.
This will cause the snappoints to be calculated 3 times when using all three snappoint properties.
Instead we should set them all at once.
So on IScrollVisual have a single SetSnapPoints function that takes all 3 properties as arguments, and here check if any of the 3 properties changed, and if so call SetSnapPoints.
Description
Type of Change
Why
[Fabric] Implement snapToStart, snapToEnd, snapToOffsets property for ScrollView
Resolves #14796 #13152 #13151
What
[Fabric] Implement snapToStart, snapToEnd, snapToOffsets property for ScrollView
Screenshots
Testing
Playground already has testcase- tested
Changelog
Should this change be included in the release notes: YES
Implemented snapToStart, snapToEnd, snapToOffsets property for ScrollView in Fabric
Microsoft Reviewers: Open in CodeFlow