-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Revert Applying visibility change to child controls #28768
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
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.
Pull Request Overview
This PR reverts a previous change that affected the propagation of visibility to child controls. The key changes include:
- Reverting assertions and behavior related to visibility propagation in both test and production code.
- Removing tests that validate the propagation of the IsVisible property.
- Simplifying the VisualElement implementation by eliminating custom propagation logic.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Controls/tests/Xaml.UnitTests/Issues/Issue1549.cs | Added an assertion on InvertBoolenConverter.count to reflect the reverted behavior |
| src/Controls/tests/Core.UnitTests/VisualElementTests.cs | Removed tests validating visibility propagation to child controls |
| src/Controls/src/Core/VisualElement/VisualElement.cs | Removed property propagation calls and helper methods related to IsVisible |
Comments suppressed due to low confidence (3)
src/Controls/tests/Xaml.UnitTests/Issues/Issue1549.cs:192
- Please confirm that the assertion expecting InvertBoolenConverter.count to be 4 is stable given the revert; if this value is based on prior behavior, a brief code comment explaining the expected count would help avoid future confusion.
Assert.AreEqual(4, InvertBoolenConverter.count);
src/Controls/tests/Core.UnitTests/VisualElementTests.cs:317
- The tests for propagating visibility to child controls have been removed as part of the revert; please ensure that equivalent coverage is maintained in TestCases.HostApp and TestCases.Shared.Tests if this behavior is revisited in future releases.
[Fact] public void ShouldPropagateVisibilityToChildren() { ... }
src/Controls/src/Core/VisualElement/VisualElement.cs:1501
- The removal of the IsVisible property propagation call in OnIsVisibleChanged aligns with the revert; please double-check that this change does not disrupt any internal state updates expected by the existing test suite.
(this as IPropertyPropagationController)?.PropagatePropertyChanged(IsVisibleProperty.PropertyName);
|
/rebase |
3036baf to
c3643ae
Compare
|
/backport to release/9.0.1xx-sr5 |
|
Started backporting to release/9.0.1xx-sr5: https://github.com/dotnet/maui/actions/runs/14342767821 |
|
|
@jfversluis Since the original PR was reverted, should the bugs it was meant to fix be reopened? |
Description of Change
Reverts #20154 since it caused unexpected behavior for a lot of folks.
Reverting now and then improve on this and try again at a later stage.
Fixes #28677
Fixes #28415
cc: @kubaflo