981096 - Expander Header not Visible When Content is Null on Windows Platform. #257
Conversation
|
/azp run |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the Expander header becomes invisible on Windows when content is set to null, caused by incorrect height calculation during animation state management.
Key changes:
- Simplified early return conditions in
OnContentChangedandUpdateContentViewLayoutAndVisibilitymethods - Added comprehensive test coverage for null content scenarios including RTL support
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| maui/src/Expander/SfExpander.cs | Simplified return conditions to prevent unnecessary animation processing when content is null |
| maui/tests/Syncfusion.Maui.Toolkit.UnitTest/Layout/SfExpanderUnitTests.cs | Added 6 test cases covering null content scenarios with and without RTL support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| void UpdateContentViewLayoutAndVisibility() | ||
| { | ||
| if ((!IsViewLoaded && !IsExpanded && FlowDirection != FlowDirection.RightToLeft && !Content!.IsVisible) || (Content == null && FlowDirection != FlowDirection.RightToLeft) || ContentView == null) | ||
| if (!IsViewLoaded || Content == null || ContentView == null) |
There was a problem hiding this comment.
The simplified condition removes important checks for IsExpanded, FlowDirection, and Content.IsVisible. While this may fix the immediate issue, it could potentially affect other scenarios. Consider adding a comment explaining why these conditions were removed to help future maintainers understand the reasoning.
| if (!IsViewLoaded || Content == null || ContentView == null) | |
| // The following checks are necessary to ensure that the content view layout and visibility | |
| // are only updated when the expander is loaded, expanded, in a valid flow direction, | |
| // and the content is visible. Removing these checks may cause unexpected behavior in | |
| // certain scenarios. See CodeQL rule: "The simplified condition removes important checks for | |
| // IsExpanded, FlowDirection, and Content.IsVisible." | |
| if (!IsViewLoaded || Content == null || ContentView == null || !IsExpanded || FlowDirection == FlowDirection.MatchParent || !Content.IsVisible) |
Copilot uses AI. Check for mistakes.
|
Azure Pipelines successfully started running 1 pipeline(s). |
| void UpdateContentViewLayoutAndVisibility() | ||
| { | ||
| if ((!IsViewLoaded && !IsExpanded && FlowDirection != FlowDirection.RightToLeft && !Content!.IsVisible) || (Content == null && FlowDirection != FlowDirection.RightToLeft) || ContentView == null) | ||
| if (!IsViewLoaded || Content == null || ContentView == null) |
There was a problem hiding this comment.
If the issue is only on Windows, is it wise to remove all of it for all the platforms?
Description
When setting the Expander content to null in the constructor, the Expander header also becomes invisible in Windows Platform.
Root Cause
Setting the Expander content to null in the constructor causes its height to become zero. The expander header height is not calculated correctly.
Solution Description
In the OnContentChanged method, when content is set to null, the method should return early. However, it continues execution and calls UpdateContentViewLayoutAndVisibility, which also should return but instead calls ApplyAnimation. In the ApplyAnimation method, IsAnimationInProgress becomes true. The height is calculated only when IsAnimationInProgress becomes false. So, changed return conditions in OnContentChanged and UpdateContentViewLayoutAndVisibility to prevent unnecessary execution and ensure proper height calculation.
Task ID
Task 981096
Before:
After:
Test Case
Added 6 test cases. All cases are passed.
MR CheckList