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 fixes and scroll anchoring #4091

Merged
merged 28 commits into from
Jun 20, 2020

Conversation

grokys
Copy link
Member

@grokys grokys commented Jun 8, 2020

What does the pull request do?

Trying to fix issues with ItemsRepeater:

Checklist

  • Added unit tests (if possible)?

Depends on #4080

grokys added 17 commits June 5, 2020 19:09
To diagnose problems with `ItemsRepeater`.
The default value needs to be the same as the default value in the associated `ItemsRepeater` properties.
Hack was added when porting `ItemsRepeater. Not sure it's needed anymore.
`ItemsRepeater` doesn't work properly with a 0 cache length.
`OnEffectiveViewportChanged` needs to be triggered first, otherwise `_makeAnchorElement` is cleared too early, breaking scrolling in a list with variable-height items.
So one can see their bounds easily.
Instead of simply reading old `TranslatedBounds` value when `Bounds` changes, use the clip and transform from the previous `TranslatedBounds` with the new `Bounds`.
When a layout of a control exceeds the maximum passes and we re-queue it, reset the attempt count to 0 because otherwise it can end up stuck in the layout queue forever even when the next layout can succeed with e.g. 2 passes. This can happen with `ItemsRepeater`.
When `Offset` is adjusted due to scroll ancoring during the arrange pass in `ScrollContentPresenter`, don't invalidate the arrange: we've got it handled.
- Change the hack we're using to find the effective viewport in `ViewportManager`
- Unregister anchor candidates when bringing an item into view
- Adjust `ScrollContentPresenter` extent when tracking an anchor, if the adjustment causes the offset to go out of bounds of the extent
- Simplify coercing `Offset` in `ScrollViewer`
@dep dep bot added the dependent label Jun 8, 2020
grokys added 8 commits June 8, 2020 15:21
And don't call `OnEffectiveViewportChanged` if viewport unchanged.
Instead of using `ScrollContentPresenter.Viewport` which hasn't been updated yet.
`ScrollContentPresenter` now coerces offset so it can only be set after a viewport has been calculated.
- Make sure an anchor is a descendent of the `ScrollContentPresenter` when added
- Ignore controls that are no longer descendents of the `ScrollContentPresenter` when evaluating
When we set the DataContext, we save that fact in the VirtualizationInformation for that element, when that element gets cleared, we look up in the VirtualizationInformation whether we should need to clear the DataContext too, and do that if necessary.

Ported from microsoft/microsoft-ui-xaml@e9bd88e
We need to return the max of the bounds and the constraint, otherwise horizontal/vertical alignment doesnt work correctly.
@grokys grokys requested a review from a team June 19, 2020 08:33
@grokys grokys marked this pull request as ready for review June 19, 2020 08:33
@grokys grokys changed the title WIP: Itemsrepeater fixes and scroll anchoring Itemsrepeater fixes and scroll anchoring Jun 19, 2020
@grokys
Copy link
Member Author

grokys commented Jun 19, 2020

This hasn't fixed all the problems with ItemsRepeater but it should be better at least.

Copy link
Member

@jmacato jmacato left a comment

Choose a reason for hiding this comment

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

Works :) No regressions, at least as far as i can tell :)

@jmacato jmacato merged commit 1bce382 into master Jun 20, 2020
@jmacato jmacato deleted the fixes/itemsrepeater-fixes-and-scrollanchoring branch June 20, 2020 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants