Fix issue with StartBringIntoView scrolling for items already visible (…

…microsoft/microsoft-ui-xaml#3167)
Kinnara · Sep 15, 2020 · acf30b7 · acf30b7
1 parent 8a56ee5
commit acf30b7
Show file tree

Hide file tree

Showing 3 changed files with 87 additions and 2 deletions.
diff --git a/ModernWpf.Controls/Repeater/ItemsRepeater/ViewportManagerDownLevel.cs b/ModernWpf.Controls/Repeater/ItemsRepeater/ViewportManagerDownLevel.cs
@@ -101,12 +101,17 @@ public override Rect GetLayoutVisibleWindow()
         {
             var visibleWindow = m_visibleWindow;
 
-            if (m_makeAnchorElement != null)
+            if (m_makeAnchorElement != null && m_isAnchorOutsideRealizedRange)
             {
                 // The anchor is not necessarily laid out yet. Its position should default
                 // to zero and the layout origin is expected to change once layout is done.
                 // Until then, we need a window that's going to protect the anchor from
                 // getting recycled.
+
+                // Also, we only want to mess with the realization rect iff the anchor is not inside it.
+                // If we fiddle with an anchor that is already inside the realization rect,
+                // shifting the realization rect results in repeater, layout and scroller thinking that it needs to act upon StartBringIntoView.
+                // We do NOT want that!
                 visibleWindow.X = 0.0;
                 visibleWindow.Y = 0.0;
             }

diff --git a/test/ModernWpfTestApp/ApiTests/RepeaterTests/RepeaterTests.cs b/test/ModernWpfTestApp/ApiTests/RepeaterTests/RepeaterTests.cs
@@ -31,6 +31,7 @@
 using System.Windows.Markup;
 using ModernWpf.Controls;
 using System.Windows.Media;
+using System.Diagnostics;
 
 namespace ModernWpf.Tests.MUXControls.ApiTests.RepeaterTests
 {
@@ -669,5 +670,84 @@ public void VerifyRepeaterDoesNotLeakItemContainers()
             Verify.AreEqual(0, MUXControlsTestApp.Samples.DisposableUserControl.OpenItems, "Verify we cleaned up all the DisposableUserControl that were created");
         }
 
+        [TestMethod]
+        public void BringIntoViewOfExistingItemsDoesNotChangeScrollOffset()
+        {
+            System.Windows.Controls.ScrollViewerEx scrollViewer = null;
+            ItemsRepeater repeater = null;
+            AutoResetEvent scrollViewerScrolledEvent = new AutoResetEvent(false);
+
+            RunOnUIThread.Execute(() =>
+            {
+                repeater = new ItemsRepeater();
+                repeater.ItemsSource = Enumerable.Range(0, 100).Select(x => x.ToString()).ToList();
+
+                scrollViewer = new System.Windows.Controls.ScrollViewerEx() {
+                    Content = repeater,
+                    MaxHeight = 400,
+                    MaxWidth = 200
+                };
+
+
+                Content = scrollViewer;
+                Content.UpdateLayout();
+            });
+
+            IdleSynchronizer.Wait();
+
+            RunOnUIThread.Execute(() =>
+            {
+                Log.Comment("Scroll to end");
+                scrollViewer.ViewChanged += (object sender, ScrollViewerViewChangedEventArgs e) =>
+                {
+                    if(!e.IsIntermediate)
+                    {
+                        Log.Comment("ScrollViewer scrolling finished");
+                        scrollViewerScrolledEvent.Set();
+                    }
+                };
+                scrollViewer.ChangeView(null, repeater.ActualHeight, null);
+                scrollViewer.UpdateLayout();
+            });
+
+            Log.Comment("Wait for scrolling");
+            if (Debugger.IsAttached)
+            {
+                scrollViewerScrolledEvent.WaitOne();
+            }
+            else
+            {
+                if (!scrollViewerScrolledEvent.WaitOne(TimeSpan.FromMilliseconds(5000)))
+                {
+                    throw new Exception("Timeout expiration in WaitForEvent.");
+                }
+            }
+
+            IdleSynchronizer.Wait();
+
+            double endOfScrollOffset = 0;
+            RunOnUIThread.Execute(() =>
+            {
+                Log.Comment("Determine scrolled offset");
+                endOfScrollOffset = scrollViewer.VerticalOffset;
+                // Idea: we might not have scrolled to the end, however we should at least have moved so much that the end is not too far away
+                Verify.IsTrue(Math.Abs(endOfScrollOffset - repeater.ActualHeight) < 500, $"We should at least have scrolled some amount. " +
+                    $"ScrollOffset:{endOfScrollOffset} Repeater height: {repeater.ActualHeight}");
+
+                var lastItem = (FrameworkElement)repeater.GetOrCreateElement(99);
+                lastItem.UpdateLayout();
+                Log.Comment("Bring last element into view");
+                lastItem.BringIntoView();
+            });
+
+            IdleSynchronizer.Wait();
+
+            RunOnUIThread.Execute(() =>
+            {
+                Log.Comment("Verify position did not change");
+                Verify.IsTrue(Math.Abs(endOfScrollOffset - scrollViewer.VerticalOffset) < 1);
+            });
+        }
+
     }
 }
diff --git a/test/TestAppUtils/Controls/ScrollViewerEx.cs b/test/TestAppUtils/Controls/ScrollViewerEx.cs
@@ -46,7 +46,7 @@ public class ScrollViewerEx : ScrollViewer
                     changed = true;
                 }
 
-                ScrollToHorizontalOffset(verticalOffset.Value);
+                ScrollToVerticalOffset(verticalOffset.Value);
             }
 
             return changed;