[Android] Fix empty space above TabBar after navigating back when TabBar visibility is toggled#34324
Open
praveenkumarkarunanithi wants to merge 11 commits intodotnet:mainfrom
Open
[Android] Fix empty space above TabBar after navigating back when TabBar visibility is toggled#34324praveenkumarkarunanithi wants to merge 11 commits intodotnet:mainfrom
praveenkumarkarunanithi wants to merge 11 commits intodotnet:mainfrom
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34324Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34324" |
dotnet-policy-service
bot
added
the
partner/syncfusion
- Fix .xaml.cs matching: strip .xaml extension for HostApp file matching - Relax file naming: only flag files in Issues/ that start with Issue - AutomationId: bidirectional check, reverse direction is informational - Wait check: per-interaction instead of file-level - Unit test helpers: only flag missing test methods if filename contains Test - Fix duplicate numbering in copilot-instructions.md (8,8,9,9 → 8,9,10,10) - Skip [Issue()] check on .xaml files (attribute lives in code-behind) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
praveenkumarkarunanithi
force-pushed
the
fix-33703
branch
from
d9ae703 to
653056a
Compare
sheiksyedm
added
platform/android
area-controls-shell
Contributor
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an Android Shell regression where content retains stale bottom safe-area padding after the TabBar transitions from Gone → Visible (e.g., navigating back from a page that hides the TabBar), leaving empty space above the TabBar.
Changes:
- Android: when the TabBar becomes visible again, explicitly re-request window insets on the displayed page’s platform view after layout (
ViewCompat.RequestApplyInsets). - Adds a new Android UI test (Issue33703) and corresponding HostApp repro page to validate the layout after back navigation.
- Adds an Android screenshot baseline for the new test.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellItemRenderer.cs | Requests insets after TabBar Gone → Visible to refresh safe-area padding and remove the empty gap. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue33703.cs | Adds a Shell-based repro with a ScrollView and a detail page that hides the TabBar. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33703.cs | Adds an Android-only Appium test that navigates, returns, scrolls to bottom, and verifies via screenshot. |
| src/Controls/tests/TestCases.Android.Tests/snapshots/android/TabBarShouldNotHaveEmptySpaceAfterNavigatingBack.png | Adds the screenshot baseline for the new Android test. |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33703.cs
Outdated
Show resolved
Hide resolved
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33703.cs
Outdated
Show resolved
Hide resolved
This was referenced Mar 13, 2026
Open
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
PureWeen
pushed a commit
that referenced
this pull request
Mar 18, 2026
## Description Adds a new Copilot skill (`evaluate-pr-tests`) that evaluates the quality, coverage, and appropriateness of tests added in PRs. ## Why This Is Useful **Problem:** When reviewing PRs, evaluating test quality is one of the most time-consuming and inconsistent parts of the process. Common issues slip through: - Tests that only verify a page loads but never exercise the actual fix - UI tests for behavior that could be caught by a unit test (which runs in seconds vs minutes) - Missing edge cases (null inputs, boundary values, repeated actions) that lead to regressions later - Convention violations (wrong base class, missing attributes, obsolete APIs) that cause CI failures - Flaky patterns (arbitrary delays, missing `WaitForElement`) that waste CI time These issues are tedious to catch manually and easy to miss — especially across 4 platforms, 4 test types (unit, XAML, device, UI), and hundreds of existing test patterns. **Solution:** This skill automates the mechanical checks and guides the agent through deeper analysis: 1. **Automated (script):** File categorization, convention compliance, anti-pattern detection, AutomationId consistency, existing similar test search, platform scope analysis 2. **Agent-guided (SKILL.md):** Fix coverage analysis, edge case gap identification, test type recommendation (prefer unit > XAML > device > UI), assertion quality review **Real-world example:** Running against PR #34324 (Android Shell TabBar fix), the skill: - ✅ Correctly identified 1 fix file + 1 UI test + 1 HostApp page + 1 snapshot -⚠️ Flagged an inline `#if` platform directive (real convention violation) - ℹ️ Noted 3 HostApp AutomationIds not exercised by the test (informational) - ✅ Found existing Shell tests for cross-reference - ✅ Identified this as Android-only based on fix file paths **Quality bar:** This skill went through 3 rounds of multi-model review (10 total reviews across Opus, GPT-5.1, Gemini, Sonnet 4.5) to catch regex bugs, false positive/negative edge cases, and documentation mismatches. ## Evaluation Criteria (9 checks) | # | Criterion | What It Checks | |---|-----------|---------------| | 1 | **Fix Coverage** | Does the test exercise the actual code paths changed by the fix? | | 2 | **Edge Cases & Gaps** | Boundary conditions, null handling, async/timing, repeated actions | | 3 | **Test Type Appropriateness** | Could a lighter test type work? (unit > XAML > device > UI) | | 4 | **Convention Compliance** | Naming, attributes, base classes, obsolete APIs | | 5 | **Flakiness Risk** | Delays, missing WaitForElement, screenshot hygiene | | 6 | **Duplicate Coverage** | Does a similar test already exist? | | 7 | **Platform Scope** | Does test coverage match platforms affected by the fix? | | 8 | **Assertion Quality** | Are assertions specific enough to catch regressions? | | 9 | **Fix-Test Alignment** | Do test and fix target the same code paths? | ## Components - **`SKILL.md`** — Evaluation criteria with decision trees, examples, and structured report template - **`Gather-TestContext.ps1`** — Automated convention checks, anti-pattern detection, AutomationId consistency, platform scope analysis - **`copilot-instructions.md`** — Skill registration with trigger phrases ## Usage ```bash # Auto-detect PR context pwsh .github/skills/evaluate-pr-tests/scripts/Gather-TestContext.ps1 # Or ask the agent # "evaluate the tests in this PR" # "are these tests good enough?" # "review test quality for PR #XXXXX" ``` - Fixes #34139 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
Collaborator
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34324 | Detect Gone → Visible TabBar transition; post ViewCompat.RequestApplyInsets for displayed page's platform view after layout pass. |
✅ PASSED (Gate) | ShellItemRenderer.cs |
Original PR fix |
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34324 | UpdateTabBarVisibility(): detect wasGone && showTabs, Post(() => RequestApplyInsets(platformView)) with IsAttachedToWindow guard |
✅ PASSED (Gate) | ShellItemRenderer.cs |
Original PR |
| 1 | try-fix (claude-opus-4.6) | RequestApplyInsets(_outerLayout) via one-shot ViewTreeObserver.GlobalLayout listener on container |
✅ PASS | ShellItemRenderer.cs |
Container-level, GlobalLayout timing |
| 2 | try-fix (claude-sonnet-4.6) | One-shot IOnLayoutChangeListener on _bottomView; fires when bottom > 0, then RequestApplyInsets(platformView) |
✅ PASS | ShellItemRenderer.cs |
Tied to bottomView layout completion |
| 3 | try-fix (gpt-5.3-codex) | DispatchApplyWindowInsets preemptive approach on Gone → Visible |
❌ FAIL | ShellItemRenderer.cs |
15.40% visual diff — no insets redispatch |
| 4 | try-fix (gpt-5.4) | IOnLayoutChangeListener on _navigationArea; fires when content area shrinks while TabBar visible, requests insets on page |
✅ PASS | ShellItemRenderer.cs |
Content-area geometry trigger |
| 5 | try-fix (claude-opus-4.6) | MauiWindowInsetListener: MeasuredHeight > 0 → Visibility == ViewStates.Visible (root-cause fix alone) |
❌ FAIL | MauiWindowInsetListener.cs |
Correct but insufficient — needs redispatch trigger |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Fix MauiWindowInsetListener MeasuredHeight → Visibility → ran as attempt 5 → ❌ FAIL (insufficient alone) |
| claude-sonnet-4.6 | 1 | No | NO NEW IDEAS |
| gpt-5.3-codex | 1 | Yes | Clear cached safe-area via FindListenerForView?.ResetAppliedSafeAreas before RequestApplyInsets — minor variation of PR fix |
| gpt-5.4 | 1 | No | NO NEW IDEAS |
| claude-opus-4.6 | 2 | No | NO NEW IDEAS — design space saturated |
| gpt-5.3-codex | 2 | Yes | ResetAppliedSafeAreas + RequestApplyInsets — minor variation of PR fix, not meaningfully different |
| gpt-5.4 | 2 | Yes | FindListenerForView + ResetAppliedSafeAreas + MarkSafeAreaEdgeConfigurationChanged — complex variation, same mechanism |
Exhausted: Yes (claude-opus and claude-sonnet exhausted; remaining ideas are minor variations of PR fix)
Selected Fix: PR #34324 — Reason: Simplest and most targeted. Detects wasGone && showTabs precisely, uses Post() for next-frame timing, has IsAttachedToWindow guard. Single-file change (18 lines). Alternatives 1, 2, 4 all pass but are more complex. PR fix is the cleanest implementation of the correct mechanism.
📋 Report — Final Recommendation
✅ Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issues #33703, #34256; Android-only; 1 impl + 2 test + 1 baseline |
| Gate | ✅ PASSED | Android — tests fail without fix, pass with fix |
| Try-Fix | ✅ COMPLETE | 5 attempts: 3 passing alternatives, 2 failures; PR fix selected |
| Report | ✅ COMPLETE |
Summary
PR #34324 fixes an Android-only regression where navigating back from a page that hid the TabBar leaves empty space above the TabBar due to stale safe-area bottom padding. The fix is minimal (18 lines in one implementation file), well-targeted, and passes Gate. Three independent alternative approaches also passed (attempts 1, 2, 4), all converging on the same correct mechanism — ViewCompat.RequestApplyInsets after layout — confirming the fix direction is sound. The PR's own approach is the cleanest of all candidates.
Root Cause
Android does not re-dispatch window insets when a sibling view transitions from Gone to Visible. When Shell.TabBarIsVisible returns true after navigation-back, the BottomNavigationView becomes visible but no insets re-dispatch occurs. The displayed page retains stale bottom padding (computed when TabBar was absent from layout), creating empty space above the TabBar.
Fix Quality
PR's fix is correct and clean:
- Captures
wasGonebefore toggling visibility — preciseGone → Visibledetection - Uses
platformView.Post()to defer until after the layout pass — correct timing - Guards with
IsAttachedToWindow— prevents calls during teardown - Single-file change, 18 lines added, clear inline comment explaining the mechanism
- Pattern
IPlatformViewHandler { PlatformView: { } platformView }is idiomatic MAUI
Minor nits (non-blocking):
- Both
Issue33703.csfiles use"after navigate back"— should be"after navigating back"(flagged by Copilot bot review, cosmetic only)
Alternative approaches validated:
- Attempts 1, 2, 4 all passed but require more code (25–38 lines vs 18) or introduce timing-sensitive patterns (
GlobalLayoutfires for all layout events, not just TabBar transitions) - Attempt 5 confirmed
MauiWindowInsetListenerMeasuredHeight → Visibilityis a valid root-cause insight but insufficient alone — still needs the redispatch trigger
Test coverage:
- UI test (
TabBarShouldNotHaveEmptySpaceAfterNavigatingBack) correctly reproduces the exact reported scenario (navigate to detail with TabBar hidden, navigate back, verify layout) - Screenshot baseline added for visual regression
#if ANDROIDguard is appropriate — issue is Android-specific
MauiBot
added
s/agent-review-incomplete
kubaflo
requested changes
Mar 20, 2026
Contributor
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Looks like the test if failing
Contributor
Author
Fixed the Issue string mismatch — test should pass now. |
MauiBot
added
s/agent-review-incomplete
Collaborator
🚦 Gate — Test Verification📊 Expand Full Gate —
|
| # | Type | Test Name | Filter |
|---|---|---|---|
| 1 | UITest | Issue33703 | Issue33703 |
Verification
| Step | Expected | Actual | Result |
|---|---|---|---|
| Without fix | FAIL | FAIL | ✅ |
| With fix | PASS | PASS | ✅ |
Fix Files Reverted
eng/pipelines/ci-copilot.ymlsrc/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellItemRenderer.cs
Base Branch: main | Merge Base: 720a9d4
MauiBot
added
s/agent-approved
KarthikRajaKalaimani
pushed a commit
to KarthikRajaKalaimani/maui
that referenced
this pull request
Mar 30, 2026
## Description Adds a new Copilot skill (`evaluate-pr-tests`) that evaluates the quality, coverage, and appropriateness of tests added in PRs. ## Why This Is Useful **Problem:** When reviewing PRs, evaluating test quality is one of the most time-consuming and inconsistent parts of the process. Common issues slip through: - Tests that only verify a page loads but never exercise the actual fix - UI tests for behavior that could be caught by a unit test (which runs in seconds vs minutes) - Missing edge cases (null inputs, boundary values, repeated actions) that lead to regressions later - Convention violations (wrong base class, missing attributes, obsolete APIs) that cause CI failures - Flaky patterns (arbitrary delays, missing `WaitForElement`) that waste CI time These issues are tedious to catch manually and easy to miss — especially across 4 platforms, 4 test types (unit, XAML, device, UI), and hundreds of existing test patterns. **Solution:** This skill automates the mechanical checks and guides the agent through deeper analysis: 1. **Automated (script):** File categorization, convention compliance, anti-pattern detection, AutomationId consistency, existing similar test search, platform scope analysis 2. **Agent-guided (SKILL.md):** Fix coverage analysis, edge case gap identification, test type recommendation (prefer unit > XAML > device > UI), assertion quality review **Real-world example:** Running against PR dotnet#34324 (Android Shell TabBar fix), the skill: - ✅ Correctly identified 1 fix file + 1 UI test + 1 HostApp page + 1 snapshot -⚠️ Flagged an inline `#if` platform directive (real convention violation) - ℹ️ Noted 3 HostApp AutomationIds not exercised by the test (informational) - ✅ Found existing Shell tests for cross-reference - ✅ Identified this as Android-only based on fix file paths **Quality bar:** This skill went through 3 rounds of multi-model review (10 total reviews across Opus, GPT-5.1, Gemini, Sonnet 4.5) to catch regex bugs, false positive/negative edge cases, and documentation mismatches. ## Evaluation Criteria (9 checks) | # | Criterion | What It Checks | |---|-----------|---------------| | 1 | **Fix Coverage** | Does the test exercise the actual code paths changed by the fix? | | 2 | **Edge Cases & Gaps** | Boundary conditions, null handling, async/timing, repeated actions | | 3 | **Test Type Appropriateness** | Could a lighter test type work? (unit > XAML > device > UI) | | 4 | **Convention Compliance** | Naming, attributes, base classes, obsolete APIs | | 5 | **Flakiness Risk** | Delays, missing WaitForElement, screenshot hygiene | | 6 | **Duplicate Coverage** | Does a similar test already exist? | | 7 | **Platform Scope** | Does test coverage match platforms affected by the fix? | | 8 | **Assertion Quality** | Are assertions specific enough to catch regressions? | | 9 | **Fix-Test Alignment** | Do test and fix target the same code paths? | ## Components - **`SKILL.md`** — Evaluation criteria with decision trees, examples, and structured report template - **`Gather-TestContext.ps1`** — Automated convention checks, anti-pattern detection, AutomationId consistency, platform scope analysis - **`copilot-instructions.md`** — Skill registration with trigger phrases ## Usage ```bash # Auto-detect PR context pwsh .github/skills/evaluate-pr-tests/scripts/Gather-TestContext.ps1 # Or ask the agent # "evaluate the tests in this PR" # "are these tests good enough?" # "review test quality for PR #XXXXX" ``` - Fixes dotnet#34139 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue.
Thank you!
Root Cause
When
Shell.TabBarIsVisibleis set tofalse, Android hides the TabBar by setting its visibility to Gone. As a result, the page content expands to occupy the full screen, including the area previously used by the TabBar. At this point, the safe area padding is calculated correctly based on the full screen height.When navigating back and the TabBar becomes visible again, Android reduces the height of the page content to accommodate the TabBar. However, Android does not automatically re-apply window insets when a sibling view changes from Gone to Visible. Because of this, the previously calculated bottom padding (based on the full screen height) remains unchanged, leaving an empty space between the page content and the TabBar.
Description of Change
When the TabBar transitions from Gone to Visible, the fix explicitly requests Android to re-apply window insets for the currently displayed page.
The request is posted to run after the layout pass completes, ensuring the page size has been updated before safe area padding is recalculated. An additional check ensures that the page is still attached to the window before requesting insets to avoid unnecessary calls.
By forcing Android to re-apply window insets after the layout update, the safe area padding is recalculated using the updated page height, eliminating the empty space above the TabBar. The fix is scoped to run only when the TabBar transitions from hidden to visible.
Issues Fixed
Fixes #33703
Fixes #34256
Tested the behaviour in the following platforms
Note
The UI test is added for Android only as this issue is specific to Android — the empty space above the TabBar after navigating back only reproduces on Android.
Output Video
Beforefix.mov
Afterfix.mov