Fix issue with StartBringIntoView scrolling for items already visible (…

…#3167) * Add failing test * Fix bug * Update test * Update downlevel viewportmanager * Update test * Use correct event
microsoft · Sep 14, 2020 · 70adca8 · 70adca8
1 parent 57f443a
commit 70adca8
Show file tree

Hide file tree

Showing 3 changed files with 93 additions and 2 deletions.
diff --git a/dev/Repeater/APITests/RepeaterTests.cs b/dev/Repeater/APITests/RepeaterTests.cs
@@ -30,6 +30,7 @@
 using System.Threading;
 using System.Collections.Generic;
 using Windows.UI.Xaml.Tests.MUXControls.ApiTests.RepeaterTests.Common.Mocks;
+using System.Diagnostics;
 
 namespace Windows.UI.Xaml.Tests.MUXControls.ApiTests.RepeaterTests
 {
@@ -662,5 +663,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()
+        {
+            ScrollViewer 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 ScrollViewer() {
+                    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 = repeater.GetOrCreateElement(99);
+                lastItem.UpdateLayout();
+                Log.Comment("Bring last element into view");
+                lastItem.StartBringIntoView();
+            });
+
+            IdleSynchronizer.Wait();
+
+            RunOnUIThread.Execute(() =>
+            {
+                Log.Comment("Verify position did not change");
+                Verify.IsTrue(Math.Abs(endOfScrollOffset - scrollViewer.VerticalOffset) < 1);
+            });
+        }
+
     }
 }
diff --git a/dev/Repeater/ViewportManagerDownlevel.cpp b/dev/Repeater/ViewportManagerDownlevel.cpp
@@ -90,12 +90,17 @@ winrt::Rect ViewportManagerDownLevel::GetLayoutVisibleWindow() const
 {
     auto visibleWindow = m_visibleWindow;
 
-    if (m_makeAnchorElement)
+    if (m_makeAnchorElement && 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.0f;
         visibleWindow.Y = 0.0f;
     }

diff --git a/dev/Repeater/ViewportManagerWithPlatformFeatures.cpp b/dev/Repeater/ViewportManagerWithPlatformFeatures.cpp
@@ -97,12 +97,18 @@ winrt::Rect ViewportManagerWithPlatformFeatures::GetLayoutVisibleWindow() const
 {
     auto visibleWindow = m_visibleWindow;
 
-    if (m_makeAnchorElement)
+    if (m_makeAnchorElement && 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.0f;
         visibleWindow.Y = 0.0f;
     }