[Windows, Android] Fix ScrollView Content Not Removed When Set to Null #33069
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33069Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33069" |
dotnet-policy-service
bot
added
the
partner/syncfusion
sheiksyedm
added
area-controls-scrollview
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where setting ScrollView.Content to null on Android and Windows did not properly remove the existing content from the native view hierarchy, while iOS handled this correctly. The fix modifies the handler update methods to explicitly clear native children when PresentedContent is null, preventing memory leaks and visual inconsistencies.
Key Changes
- Android & Windows handlers: Restructured content update logic to handle null content by clearing native children before returning
- Added UI test: Created automated test to verify content removal and re-addition behavior across all platforms
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Core/src/Handlers/ScrollView/ScrollViewHandler.Windows.cs | Refactored UpdateContentPanel to clear CachedChildren when content is set to null, ensuring proper cleanup on Windows |
| src/Core/src/Handlers/ScrollView/ScrollViewHandler.Android.cs | Refactored UpdateInsetView to call RemoveAllViews() when content is set to null, ensuring proper cleanup on Android |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33067.cs | Added NUnit UI test to verify ScrollView content removal and re-addition behavior |
| src/Controls/tests/TestCases.HostApp/Issues/Issue33067.cs | Added test page with interactive buttons to demonstrate and test the bug fix |
| { | ||
| currentPaddingLayer.CachedChildren.Clear(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Trailing whitespace should be removed from this empty line. This can be caught by running dotnet format before committing.
| var nativeContent = scrollView.PresentedContent.ToPlatform(handler.MauiContext); | ||
|
|
||
| if (GetContentPanel(scrollViewer) is ContentPanel currentPaddingLayer) | ||
| if (currentPaddingLayer != null) |
There was a problem hiding this comment.
Inconsistent null check pattern. This should use is not null to match the pattern used elsewhere in this file (line 139) and maintain consistency with the rest of the codebase.
| if (currentPaddingLayer != null) | |
| if (currentPaddingLayer is not null) |
| @@ -0,0 +1,80 @@ | |||
| using System.Collections.ObjectModel; | |||
There was a problem hiding this comment.
The System.Collections.ObjectModel namespace is not used in this file and should be removed.
| using System.Collections.ObjectModel; |
| if (_originalContent != null) | ||
| { | ||
| _scrollView.Content = _originalContent; | ||
| } |
There was a problem hiding this comment.
The null check for _originalContent is unnecessary here because it's initialized as null! (null-forgiving operator) on line 10 and is always assigned a non-null value in CreateUI() before any button click can occur. This check can be simplified to just _scrollView.Content = _originalContent;
| if (_originalContent != null) | |
| { | |
| _scrollView.Content = _originalContent; | |
| } | |
| _scrollView.Content = _originalContent; |
|
@ -0,0 +1,485 @@ Review Feedback: PR #33069 - [Windows, Android] Fix ScrollView Content Not Removed When Set to NullRecommendation✅ Approve with Minor Suggestions Critical Fix: This PR correctly fixes a memory leak and visual bug where Android and Windows fail to remove ScrollView content when set to null. The fix aligns with iOS behavior and follows best practices. Recommended changes (non-blocking):
📋 Full PR Review DetailsSummaryThis PR fixes a memory leak and visual bug on Android and Windows where setting Impact: Prevents memory leaks in apps that dynamically change ScrollView content, especially in scenarios like tab navigation or conditional UI updates. Root Cause AnalysisThe ProblemThe original Android and Windows handlers had a critical flaw in their early-return logic: // OLD CODE (BROKEN) - Android
static void UpdateInsetView(IScrollView scrollView, IScrollViewHandler handler, ...)
{
if (scrollView.PresentedContent == null || handler.MauiContext == null)
{
return; // ❌ Returns WITHOUT touching the native view!
}
var nativeContent = scrollView.PresentedContent.ToPlatform(handler.MauiContext);
// ... update logic
}What happens when setting
Why this is a bug:
Why iOS Works CorrectlyiOS handler ( // iOS CODE (CORRECT)
static void UpdateContentView(IScrollView scrollView, IScrollViewHandler handler)
{
var platformView = handler.PlatformView;
// ALWAYS remove existing content first
if (platformView.GetContentView() is { } currentContentPlatformView)
{
currentContentPlatformView.RemoveFromSuperview(); // ✅ Always clears first
changed = true;
}
// THEN add new content if present
if (scrollView.PresentedContent is { } content)
{
var platformContent = content.ToPlatform(mauiContext);
platformView.AddSubview(platformContent);
changed = true;
}
}Key insight: iOS separates removal from addition. It always removes existing content, then conditionally adds new content. The SolutionThis PR applies the iOS pattern to Android and Windows: Android (NEW CODE): static void UpdateInsetView(IScrollView scrollView, IScrollViewHandler handler, ...)
{
if (handler.MauiContext is null) // ✅ Only check infrastructure
{
return;
}
var currentPaddingLayer = FindInsetPanel(handler);
// ✅ Explicitly handle null case - REMOVE content
if (scrollView.PresentedContent is null)
{
currentPaddingLayer?.RemoveAllViews();
return;
}
// Only execute ToPlatform if content is not null
var nativeContent = scrollView.PresentedContent.ToPlatform(handler.MauiContext);
// Update content if changed
if (currentPaddingLayer is not null)
{
if (currentPaddingLayer.ChildCount == 0 || currentPaddingLayer.GetChildAt(0) != nativeContent)
{
currentPaddingLayer.RemoveAllViews();
currentPaddingLayer.AddView(nativeContent);
}
}
else
{
InsertInsetView(handler, scrollView, nativeContent, crossPlatformLayout);
}
}Windows follows the same pattern with Code ReviewChanges MadeFiles Modified:
Android Changes AnalysisBefore (lines 196-218): if (scrollView.PresentedContent == null || handler.MauiContext == null)
{
return; // Problem: Never removes existing content
}
var nativeContent = scrollView.PresentedContent.ToPlatform(handler.MauiContext);
if (FindInsetPanel(handler) is ContentViewGroup currentPaddingLayer)
{
if (currentPaddingLayer.ChildCount == 0 || currentPaddingLayer.GetChildAt(0) != nativeContent)
{
currentPaddingLayer.RemoveAllViews();
currentPaddingLayer.AddView(nativeContent);
}
}After (lines 196-229): if (handler.MauiContext is null) // ✅ Separated infrastructure check
{
return;
}
var currentPaddingLayer = FindInsetPanel(handler); // ✅ Find once, reuse
if (scrollView.PresentedContent is null) // ✅ Explicit null handling
{
currentPaddingLayer?.RemoveAllViews(); // ✅ Clean up
return;
}
var nativeContent = scrollView.PresentedContent.ToPlatform(handler.MauiContext); // ✅ Safe to call
if (currentPaddingLayer is not null) // ✅ Changed from pattern matching
{
// Only update if content has changed or is missing
if (currentPaddingLayer.ChildCount == 0 || currentPaddingLayer.GetChildAt(0) != nativeContent)
{
currentPaddingLayer.RemoveAllViews();
currentPaddingLayer.AddView(nativeContent);
}
}
else
{
InsertInsetView(handler, scrollView, nativeContent, crossPlatformLayout);
}Key improvements:
Windows Changes AnalysisSame pattern applied to Windows:
Code Quality AssessmentStrengths:
Architecture:
Performance:
Security & Safety:
Test CoverageTests Included in PR ✅Test Page:
NUnit Test: [Test]
[Category(UITestCategories.ScrollView)]
public void VerifyScrollViewContentWhenSetToNull()
{
App.WaitForElement("SetNullButton");
App.Tap("SetNullButton");
App.WaitForNoElement("ContentLabel"); // ✅ Verifies content removed
App.Tap("AddContentButton");
App.WaitForElement("ContentLabel"); // ✅ Verifies content re-added
}Test quality: ✅ Good
What the Test Covers✅ Covered scenarios:
What the Test Doesn't Cover🟡 Edge cases not tested (non-critical but worth considering):
Assessment: The included test is sufficient for the bug fix. Additional edge case tests would be nice-to-have but not required for merge. Edge Cases AnalysisHandled Correctly ✅
Potential Concerns (Low Priority) 🟡
Consistency with MAUI CodebaseComparison with iOS HandlerThis PR brings Android/Windows into alignment with iOS:
Verdict: ✅ Excellent consistency - All platforms now use the same logical pattern. Pattern Usage in Other HandlersSimilar patterns in MAUI codebase:
Verdict: ✅ This PR follows established MAUI patterns. Issues FoundMust FixNone - Code is correct as written. Should Fix (Non-blocking suggestions)
Won't Fix (Intentional choices)
Approval Checklist
Additional NotesFor ReviewersThis is a straightforward bug fix that addresses:
The author:
Confidence level: Very High
For Merge DecisionRecommendation: Approve and merge Why merge:
Follow-up work (optional, separate issues):
Related Issues/PRsSimilar fixes in MAUI history:
Pattern: MAUI has historically had issues with null content handling. This PR continues the trend of fixing these systematically. Review Metadata
Summary for TeamThis is a clean bug fix that resolves a memory leak and visual inconsistency when setting The change is minimal, focused, and low-risk. Ready to merge. Decision: ✅ Ready to merge |
|
Hi @devanathan-vaithiyanathan could you please add this test? [Test]
public void ScrollViewInitializedWithNullContent()
{
// Test that ScrollView can be created with Content = null
// Verifies no crashes, proper layout
} |
4 participants
Issue Details
When ScrollView.Content is set to null in .NET MAUI, iOS correctly removes the content, but Android and Windows do not. The previous content remains in the native view hierarchy, leading to memory leaks and visual inconsistencies.
Description of Change
In both platforms, the handler methods (UpdateInsetView on Android and UpdateContentPanel on Windows) were returning early when PresentedContent was null, without removing the existing native child views. As a result, stale views continued to remain attached.
Issues Fixed
Fixes #33067
Tested the behavior in the following platforms.
Before.mov
After.mov