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

ItemsRepeater scrolls to wrong element #1425

Closed
grokys opened this issue Oct 10, 2019 · 4 comments · Fixed by #3167
Closed

ItemsRepeater scrolls to wrong element #1425

grokys opened this issue Oct 10, 2019 · 4 comments · Fixed by #3167
Labels
area-ItemsRepeater bug Something isn't working team-Controls Issue for the Controls team

Comments

@grokys
Copy link

grokys commented Oct 10, 2019

Describe the bug

ItemsRepeater scrolls to the wrong element when scrolled to end and trying to bring into view last item.

Steps to reproduce the bug

Edit BasicDemo to contain the following:

    <ScrollViewer HorizontalScrollBarVisibility="Disabled"
                      VerticalScrollBarVisibility="Auto"
                      Grid.Row="1" Width="250" Height="350">
        <Border Background="LightBlue">
            <controls:ItemsRepeater x:Name="repeater">
                <controls:ItemsRepeater.ItemTemplate>
                    <DataTemplate>
                        <Border BorderBrush="Red" BorderThickness="1">
                            <TextBlock HorizontalAlignment="Stretch" Text="{Binding}"/>
                        </Border>
                    </DataTemplate>
                </controls:ItemsRepeater.ItemTemplate>
            </controls:ItemsRepeater>
        </Border>
    </ScrollViewer>
    public sealed partial class BasicDemo : Page
    {
        public BasicDemo()
        {
            this.InitializeComponent();
            repeater.ItemsSource = Enumerable.Range(0, 10000).Select(x => x.ToString()).ToList();
        }

        protected override void OnPointerPressed(PointerRoutedEventArgs e)
        {
            base.OnPointerPressed(e);

            var element = repeater.GetOrCreateElement(9999);
            element.UpdateLayout();
            element.StartBringIntoView();
        }
    }

Run the program and manually scroll to the end (using scrollbar). Click an item to trigger OnPointerPressed. ItemsRepeater scrolls to item 19 despite requesting item 9999.

Expected behavior

Nothing happens because item 9999 is already visible.

Screenshots

xBZY6JZvaU

Version Info

Current master (a53a650)

Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2019 Update (18362) Yes
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Mobile
Xbox
Surface Hub
IoT
@jevansaks
Copy link
Member

Sounds very similar to #1423, maybe same root cause?

@jevansaks jevansaks added area-ItemsRepeater bug Something isn't working labels Oct 11, 2019
@grokys
Copy link
Author

grokys commented Oct 11, 2019

Quite possibly, I was unsure whether to put them both in the same issue, but I figured it would be easier to track in two issues if they turn out to be different bugs.

@jevansaks jevansaks added the team-Controls Issue for the Controls team label Nov 7, 2019
@grokys
Copy link
Author

grokys commented Dec 23, 2019

The problem seems to go something like this:

  • repeater.GetOrCreateElement(9999); is called
  • ItemsRepeater::GetOrCreateElementImpl() calls m_viewportManager->OnMakeAnchor() with the element, meaning m_makeAnchorElement is set
  • When the control is measured, at first anchorRealized == true
  • Then m_elementManager.OnBeginMeasure() is called
  • This gets the realization rect from m_context.RealizationRect()
  • Which ends up in ViewportManagerWithPlatformFeatures::GetLayoutVisibleWindow()
  • Which sees that m_makeAnchorElement is set and so returns a visible window starting at 0, 0
  • OnBeginMeasure() passes this rect to DiscardElementsOutsideWindow
  • Which discards the anchor element as it thinks it's outside the window!

So it seems like this comment, while maybe true for non-laid out anchors, does the precise opposite for laid-out anchors:

// 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.

grokys added a commit to AvaloniaUI/Avalonia that referenced this issue Dec 23, 2019
microsoft/microsoft-ui-xaml#1425 describes a bug which causes scrolling to break with `ItemsRepeater`. This is a temporary fix that seems to work around it - not sure if it's the correct fix so when the WinUI issue is fixed, undo this change and port the change from there if it's different.
@marcelwgn
Copy link
Collaborator

I would like to look into this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ItemsRepeater bug Something isn't working team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants