Skip to content
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

Fix issue with StartBringIntoView scrolling for items already visible #3167

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions dev/Repeater/APITests/RepeaterTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -662,5 +662,57 @@ 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 scroll = null;
marcelwgn marked this conversation as resolved.
Show resolved Hide resolved
ItemsRepeater repeater = null;

RunOnUIThread.Execute(() =>
{
repeater = new ItemsRepeater();
repeater.ItemsSource = Enumerable.Range(0, 100).Select(x => x.ToString()).ToList();

scroll = new ScrollViewer() {
Content = repeater,
MaxHeight = 400,
MaxWidth = 200
};

Content = scroll;
Content.UpdateLayout();
});

IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
scroll.ChangeView(null, repeater.ActualHeight, null);
scroll.UpdateLayout();
});
IdleSynchronizer.Wait();

double endOfScrollOffset = 0;

RunOnUIThread.Execute(() =>
{
endOfScrollOffset = scroll.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();
lastItem.StartBringIntoView();
});

IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
Verify.IsTrue(Math.Abs(endOfScrollOffset - scroll.VerticalOffset) < 1);
});
}

}
}
7 changes: 6 additions & 1 deletion dev/Repeater/ViewportManagerDownlevel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only [](start = 20, length = 4)

nit: Only and Iff is a bit redundant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't bother fixing this unless you need to make another change

// 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;
}
Expand Down
8 changes: 7 additions & 1 deletion dev/Repeater/ViewportManagerWithPlatformFeatures.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down