-
Notifications
You must be signed in to change notification settings - Fork 2k
[Android] - Fix KeepScrollOffset Behavior During Dynamic Item Additions in CollectionView #29255
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -370,10 +370,9 @@ public virtual void UpdateItemsSource() | |
| UpdateAdapter(); | ||
|
|
||
| // Set up any properties which require observing data changes in the adapter | ||
| UpdateItemsUpdatingScrollMode(); | ||
|
|
||
| UpdateEmptyView(); | ||
| AddOrUpdateScrollListener(); | ||
| UpdateItemsUpdatingScrollMode(); | ||
| UpdateSnapBehavior(); | ||
| } | ||
|
|
||
|
|
@@ -382,6 +381,15 @@ protected virtual void UpdateItemsUpdatingScrollMode() | |
| if (ItemsViewAdapter == null || ItemsView == null) | ||
| return; | ||
|
|
||
| if (ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset) | ||
| { | ||
| ScrollHelper.AddScrollListener(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Handler lifecycle — |
||
| } | ||
| else | ||
| { | ||
| ScrollHelper.RemoveScrollListener(); | ||
| } | ||
|
Comment on lines
+384
to
+391
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed the concern |
||
|
|
||
| if (ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepItemsInView) | ||
| { | ||
| // Keeping the current items in view is the default, so we don't need to watch for data changes | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ internal class ScrollHelper : RecyclerView.OnScrollListener | |
|
|
||
| bool _undoNextScrollAdjustment; | ||
| bool _maintainingScrollOffsets; | ||
|
|
||
| bool _isAtScrollOrigin = true; | ||
| int _lastScrollX; | ||
| int _lastScrollY; | ||
| int _lastDeltaX; | ||
|
|
@@ -26,13 +26,6 @@ public ScrollHelper(RecyclerView recyclerView) | |
| // Used by the renderer to maintain scroll offset when using ItemsUpdatingScrollMode KeepScrollOffset | ||
| public void UndoNextScrollAdjustment() | ||
| { | ||
| // Don't start tracking the scroll offsets until we really need to | ||
| if (!_maintainingScrollOffsets) | ||
| { | ||
| _maintainingScrollOffsets = true; | ||
| _recyclerView.AddOnScrollListener(this); | ||
| } | ||
|
|
||
| _undoNextScrollAdjustment = true; | ||
|
|
||
| _lastScrollX = _recyclerView.ComputeHorizontalScrollOffset(); | ||
|
|
@@ -212,18 +205,46 @@ void TrackOffsets() | |
| // offset to shift; since the ItemsUpdatingScrollMode is set to KeepScrollOffset; we need to undo | ||
| // that shift and stay where we were before the item was added | ||
|
|
||
| _undoNextScrollAdjustment = false; | ||
| _recyclerView.ScrollBy(-_lastDeltaX, -_lastDeltaY); | ||
| if (_isAtScrollOrigin) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Android CollectionView — KeepScrollOffset semantics — This gates the offset correction on
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] CollectionView Android — Gating the offset correction on |
||
| { | ||
| _recyclerView.ScrollBy(-_lastDeltaX, -_lastDeltaY); | ||
| } | ||
|
|
||
| _undoNextScrollAdjustment = false; | ||
| _lastDeltaX = 0; | ||
| _lastDeltaY = 0; | ||
| } | ||
| else | ||
| { | ||
| _isAtScrollOrigin = newXOffset == 0 | ||
| && newYOffset == 0; | ||
| } | ||
| } | ||
|
|
||
| public override void OnScrolled(RecyclerView recyclerView, int dx, int dy) | ||
| { | ||
| base.OnScrolled(recyclerView, dx, dy); | ||
| TrackOffsets(); | ||
| } | ||
|
|
||
| internal void AddScrollListener() | ||
| { | ||
| // Set up scroll listener to track the scroll offsets when we're using KeepScrollOffset. | ||
| if (!_maintainingScrollOffsets) | ||
| { | ||
| _maintainingScrollOffsets = true; | ||
| _recyclerView.AddOnScrollListener(this); | ||
| } | ||
| } | ||
|
|
||
| internal void RemoveScrollListener() | ||
| { | ||
| // Remove the scroll listener when we're done and no longer need to track the offsets. | ||
| if (_maintainingScrollOffsets) | ||
| { | ||
| _maintainingScrollOffsets = false; | ||
| _recyclerView.RemoveOnScrollListener(this); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| using System.Collections.ObjectModel; | ||
| using Maui.Controls.Sample.Issues; | ||
|
|
||
| namespace Controls.TestCases.HostApp.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 29131, "Android - KeepScrollOffset does not work as expected when new items are added in CollectionView", PlatformAffected.Android)] | ||
| public class Issue29131 : TestContentPage | ||
| { | ||
| ObservableCollection<string> items; | ||
| CollectionView collectionView; | ||
| int count = 1; | ||
|
|
||
| protected override void Init() | ||
| { | ||
| items = new ObservableCollection<string>(Enumerable.Range(1, 30).Select(i => $"Item {i}")); | ||
|
|
||
| Button keepScrollOffsetButton = CreateButton("KeepScrollOffset", "KeepScrollOffsetButton", OnKeepScrollOffsetClicked); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could include more Buttons to change the ItemsUpdatingScrollMode value https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/Items/ItemsUpdatingScrollMode.cs#L8
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, adding tests for the various On Android: The On iOS : Given these inconsistencies, only the Since the other two PRs handles the Looking for your insights. |
||
| Button addButton = CreateButton("Add Item to Top", "AddNewItem", OnAddItemClicked); | ||
| Button scrollButton = CreateButton("Scroll CollectionView", "ScrollButton", OnScrollButtonClicked); | ||
|
|
||
| collectionView = new CollectionView | ||
| { | ||
| AutomationId = "CollectionView", | ||
| ItemsSource = items, | ||
| ItemTemplate = new DataTemplate(() => | ||
| { | ||
| var label = new Label(); | ||
| label.SetBinding(Label.TextProperty, "."); | ||
| return new Border | ||
| { | ||
| Content = label, | ||
| Padding = 10, | ||
| Margin = new Thickness(5), | ||
| BackgroundColor = Colors.LightGray, | ||
| }; | ||
| }) | ||
| }; | ||
|
|
||
| Grid grid = new Grid | ||
| { | ||
| RowDefinitions = | ||
| { | ||
| new RowDefinition { Height = GridLength.Auto }, | ||
| new RowDefinition { Height = GridLength.Auto }, | ||
| new RowDefinition { Height = GridLength.Auto }, | ||
| new RowDefinition { Height = GridLength.Star } | ||
| }, | ||
| RowSpacing = 5 | ||
| }; | ||
| grid.Add(keepScrollOffsetButton, 0, 0); | ||
| grid.Add(addButton, 0, 1); | ||
| grid.Add(scrollButton, 0, 2); | ||
| grid.Add(collectionView, 0, 3); | ||
|
|
||
| Content = grid; | ||
| } | ||
|
|
||
| Button CreateButton(string text, string automationId, EventHandler onClick) | ||
| { | ||
| return new Button | ||
| { | ||
| Text = text, | ||
| AutomationId = automationId, | ||
| Command = new Command(_ => onClick(this, EventArgs.Empty)) | ||
| }; | ||
| } | ||
|
|
||
| void OnKeepScrollOffsetClicked(object sender, EventArgs e) | ||
| { | ||
| collectionView.ItemsUpdatingScrollMode = ItemsUpdatingScrollMode.KeepScrollOffset; | ||
| } | ||
|
|
||
| void OnScrollButtonClicked(object sender, EventArgs e) | ||
| { | ||
| int index = (count % 2 == 0) ? 0 : items.Count - 1; | ||
| var position = (count % 2 == 0) ? ScrollToPosition.Start : ScrollToPosition.End; | ||
| collectionView.ScrollTo(index, position: position, animate: true); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] UI test reliability — The page starts an animated |
||
| count++; | ||
| } | ||
|
|
||
| void OnAddItemClicked(object sender, EventArgs e) | ||
| { | ||
| items.Insert(0, $"Item {items.Count + 1}"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue29131 : _IssuesUITest | ||
| { | ||
| public Issue29131(TestDevice device) : base(device) { } | ||
|
|
||
| public override string Issue => "Android - KeepScrollOffset does not work as expected when new items are added in CollectionView"; | ||
| const string AddNewItem = "AddNewItem"; | ||
| const string ScrollButton = "ScrollButton"; | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.CollectionView)] | ||
| public void KeepScrollOffsetShouldWork() | ||
| { | ||
| App.WaitForElement("CollectionView"); | ||
| App.Click("KeepScrollOffsetButton"); | ||
| App.Click(ScrollButton); | ||
| App.Click(AddNewItem); | ||
| App.WaitForElement("Item 30"); | ||
| App.Click(ScrollButton); | ||
| App.Click(AddNewItem); | ||
| App.WaitForElement("Item 32"); | ||
| App.Click(ScrollButton); | ||
| App.Click(AddNewItem); | ||
| App.WaitForElement("Item 30"); | ||
| } | ||
| } |
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.
[major] Android CollectionView — scroll listener lifecycle —
ScrollHelper.AddScrollListener()can no-op after the helper has already been removed byAddOrUpdateScrollListener()/RemoveScrollListener(), because those methods callClearOnScrollListeners()but do not resetScrollHelper's_maintainingScrollOffsetsflag. Concrete scenario: withKeepScrollOffsetactive,UpdateItemsSource()callsAddOrUpdateScrollListener(), clearing all listeners includingScrollHelper; then this line callsAddScrollListener(), but_maintainingScrollOffsetsis still true so the helper is not re-registered and future collection updates are not tracked.