-
Notifications
You must be signed in to change notification settings - Fork 1.9k
CollectionView display is broken when setting IsVisible after items are added - Fix #28425
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
Conversation
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Test] | ||
[Category(UITestCategories.IsVisible)] | ||
[Category(UITestCategories.CollectionView)] | ||
public void CollectionViewItemsShouldBeVisible() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is failing on Windows:
The app was expected to be running still, investigate as possible crash
Seems is crashing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please /azp again?
@@ -591,18 +591,6 @@ private protected virtual void InvalidateMeasureLegacy(InvalidationTrigger trigg | |||
} | |||
} | |||
|
|||
internal override void OnIsVisibleChanged(bool oldValue, bool newValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have tests related to this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about it. I hoped that the current uI tests would be enough to catch a potential regression
Ther are passing locally so these seem to still be flaky #27905 |
It has been many moons, but is this PR still needed? the issue #28415 is closed now. |
Description of Change
I think that because of this PR #20154 this override of
OnIsVisibleChanged
can be removed, which would improve the performance ofCompatibility.Layout
Issues Fixed
Fixes #28415