diff --git a/dev/Repeater/APITests/RepeaterTests.cs b/dev/Repeater/APITests/RepeaterTests.cs index b8d246639c..4b3480d2f1 100644 --- 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 index 70efa6243e..37c07273df 100644 --- 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 index 9ff9fbc59d..f9a00e10dd 100644 --- 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; }