-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix appBarLayout so it extends correctly full screen #32060
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
base: net10.0
Are you sure you want to change the base?
Conversation
/backport to release/10.0.1xx |
Started backporting to release/10.0.1xx: https://github.com/dotnet/maui/actions/runs/18618999271 |
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 adjusts Android AppBarLayout padding logic for correct full-screen extension, enhances device screen size compatibility checks in UI tests, and adds new safe area test cases for Shell and Navigation pages on iOS/Android.
- Updated AppBarLayout padding to use system bar insets for left/right edges.
- Added Shell and NavigationPage safe area UI tests plus corresponding HostApp pages.
- Expanded emulator screen size validation to accept rotated width/height formats.
Reviewed Changes
Copilot reviewed 6 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
src/Core/src/Platform/Android/GlobalWindowInsetListener.cs | Modifies AppBarLayout padding to use system bar insets for better safe area handling. |
src/Controls/tests/TestCases.Shared.Tests/UITest.cs | Broadens device screen size checks to accept alternative orientation formats. |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue28986_Shell.cs | Adds Shell safe area UI test exercising landscape/portrait transitions. |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue28986_NavigationPage.cs | Adds NavigationPage safe area UI test for edge extension validation. |
src/Controls/tests/TestCases.HostApp/Issues/Issue28986_Shell.cs | Introduces Shell host page used by the new safe area test. |
src/Controls/tests/TestCases.HostApp/Issues/Issue28986_NavigationPage.cs | Introduces NavigationPage host page used by the new safe area test. |
} | ||
|
||
if (!((deviceApiLevel == 30 && deviceScreenSize == "1080x1920" && deviceScreenDensity == 420) || (deviceApiLevel == 36 && deviceScreenSize == "1080x2424" && deviceScreenDensity == 420))) | ||
if (!((deviceApiLevel == 30 && (deviceScreenSize == "1080x1920" || deviceScreenSize == "1920X1080" ) && deviceScreenDensity == 420) || |
Copilot
AI
Oct 18, 2025
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 string literal "1920X1080" has an uppercase X while other comparisons use lowercase x; this exact match will never succeed if the reported size follows the existing lowercase pattern, reducing intended compatibility. Change it to "1920x1080".
if (!((deviceApiLevel == 30 && (deviceScreenSize == "1080x1920" || deviceScreenSize == "1920X1080" ) && deviceScreenDensity == 420) || | |
if (!((deviceApiLevel == 30 && (deviceScreenSize == "1080x1920" || deviceScreenSize == "1920x1080" ) && deviceScreenDensity == 420) || |
Copilot uses AI. Check for mistakes.
if (!((deviceApiLevel == 30 && (deviceScreenSize == "1080x1920" || deviceScreenSize == "1920X1080" ) && deviceScreenDensity == 420) || | ||
(deviceApiLevel == 36 && (deviceScreenSize == "1080x2424" || deviceScreenSize == "2424x1080") && deviceScreenDensity == 420))) |
Copilot
AI
Oct 18, 2025
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 nested conditional combines multiple screen size and API checks inline, reducing readability and increasing duplication. Consider normalizing deviceScreenSize (e.g., ensure lowercase, order-independent) and extracting a helper like IsSupportedAndroidTestEnvironment(deviceApiLevel, deviceScreenSize, deviceScreenDensity) or using a HashSet of allowed tuples for clarity.
Copilot uses AI. Check for mistakes.
public void ToolbarExtendsAllTheWayLeftAndRight_Shell() | ||
{ | ||
// 1. Test loads - verify essential elements are present | ||
App.WaitForElement("ContentGrid"); | ||
App.SetOrientationLandscape(); | ||
App.WaitForElement("ContentGrid"); | ||
VerifyScreenshot(); |
Copilot
AI
Oct 18, 2025
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 only waits for the element and captures a screenshot; it does not assert that the toolbar actually extends edge-to-edge (behavior under test). Add explicit assertions (e.g., measure toolbar bounds vs device screen width or verify absence of horizontal padding) instead of relying solely on manual/visual screenshot inspection.
Copilot uses AI. Check for mistakes.
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue28986_NavigationPage.cs
Show resolved
Hide resolved
using System.Drawing; | ||
|
Copilot
AI
Oct 18, 2025
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 System.Drawing namespace is not used in this file (Colors.Blue comes from Microsoft.Maui.Graphics), so this using directive can be removed to reduce noise.
using System.Drawing; |
Copilot uses AI. Check for mistakes.
// Pad the AppBarLayout to avoid the navigation bar in landscape orientation and system UI in portrait. | ||
// In landscape, the navigation bar is on the left or right edge; in portrait, we account for the status bar and display cutouts. | ||
// Without this padding, the AppBarLayout would extend behind these system UI elements and be partially hidden or non-interactive. | ||
appBarLayout.SetPadding(systemBars?.Left ?? 0, topInset, systemBars?.Right ?? 0, 0); |
Copilot
AI
Oct 18, 2025
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 padding now mixes sources (systemBars?.Left/Right with topInset) and removes previous left/right inset padding entirely when the layout is empty; this change reduces clarity on why top uses a different inset source. Consider consistently using systemBars values (including top) or add a brief inline note explaining why topInset is preferred over systemBars?.Top and why padding is fully cleared in the else branch.
appBarLayout.SetPadding(systemBars?.Left ?? 0, topInset, systemBars?.Right ?? 0, 0); | |
// Use the maximum of systemBars and displayCutout for all edges to ensure consistent handling of system UI and cutouts. | |
appBarLayout.SetPadding(leftInset, topInset, rightInset, 0); |
Copilot uses AI. Check for mistakes.
// Without this padding, the AppBarLayout would extend behind these system UI elements and be partially hidden or non-interactive. | ||
appBarLayout.SetPadding(systemBars?.Left ?? 0, topInset, systemBars?.Right ?? 0, 0); | ||
} | ||
else | ||
{ |
Copilot
AI
Oct 18, 2025
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 padding now mixes sources (systemBars?.Left/Right with topInset) and removes previous left/right inset padding entirely when the layout is empty; this change reduces clarity on why top uses a different inset source. Consider consistently using systemBars values (including top) or add a brief inline note explaining why topInset is preferred over systemBars?.Top and why padding is fully cleared in the else branch.
// Without this padding, the AppBarLayout would extend behind these system UI elements and be partially hidden or non-interactive. | |
appBarLayout.SetPadding(systemBars?.Left ?? 0, topInset, systemBars?.Right ?? 0, 0); | |
} | |
else | |
{ | |
// Without this padding, the AppBarLayout would extend behind these system UI elements and be partially hidden or non-interactive. | |
// Note: topInset is used for the top padding instead of systemBars?.Top because it may include additional insets (e.g., display cutouts) not present in systemBars. | |
appBarLayout.SetPadding(systemBars?.Left ?? 0, topInset, systemBars?.Right ?? 0, 0); | |
} | |
else | |
{ | |
// When the AppBarLayout is empty, all padding is cleared to avoid reserving space for system bars unnecessarily. |
Copilot uses AI. Check for mistakes.
/backport to release/10.0.1xx |
Started backporting to release/10.0.1xx: https://github.com/dotnet/maui/actions/runs/18619646300 |
/backport to release/10.0.1xx |
Started backporting to release/10.0.1xx: https://github.com/dotnet/maui/actions/runs/18620579686 |
} | ||
|
||
if (!((deviceApiLevel == 30 && deviceScreenSize == "1080x1920" && deviceScreenDensity == 420) || (deviceApiLevel == 36 && deviceScreenSize == "1080x2424" && deviceScreenDensity == 420))) | ||
if (!((deviceApiLevel == 30 && (deviceScreenSize == "1080x1920" || deviceScreenSize == "1920X1080" ) && deviceScreenDensity == 420) || |
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.
"1920X1080" uses uppercase 'X' while "1080x1920" uses lowercase 'x'. Should normalize with .ToLower()
?
App.SetOrientationLandscape(); | ||
App.WaitForElement("ContentGrid"); | ||
VerifyScreenshot(); | ||
App.SetOrientationPortrait(); |
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 sets orientation back to portrait but doesn't verify the UI in portrait mode, should verify with a screenshot the layout again?
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.
@PureWeen Let me know if want myself to include more tests.
/backport to release/10.0.1xx |
Started backporting to release/10.0.1xx: https://github.com/dotnet/maui/actions/runs/18687597856 |
/rebase |
/backport to release/10.0.1xx |
Started backporting to release/10.0.1xx: https://github.com/dotnet/maui/actions/runs/18729373002 |
0476879
to
f0e909e
Compare
f0e909e
to
74bd62b
Compare
/backport to release/10.0.1xx |
Started backporting to release/10.0.1xx: https://github.com/dotnet/maui/actions/runs/18730925262 |
/backport to release/10.0.1xx |
Started backporting to release/10.0.1xx: https://github.com/dotnet/maui/actions/runs/18733384677 |
@PureWeen backporting to "release/10.0.1xx" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix appBarLayout so it extends correctly full screen
Applying: - update screen shots
Applying: - update notch images
Applying: - generate iOS images
Applying: - fix based on review comments
Applying: - fix insetting on MaterialToolBar so its content doesn't go into the display cutout
Applying: - fix insets
Applying: - fix screen shots
Using index info to reconstruct a base tree...
A src/Controls/tests/TestCases.Android.Tests/snapshots/android/CollectionViewSelectionModeOnDarkTheme.png
A src/Controls/tests/TestCases.Android.Tests/snapshots/android/CollectionViewSelectionModeOnLightTheme.png
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/Controls/tests/TestCases.Android.Tests/snapshots/android/CollectionViewSelectionModeOnLightTheme.png deleted in HEAD and modified in - fix screen shots. Version - fix screen shots of src/Controls/tests/TestCases.Android.Tests/snapshots/android/CollectionViewSelectionModeOnLightTheme.png left in tree.
warning: Cannot merge binary files: src/Controls/tests/TestCases.Android.Tests/snapshots/android/SelectedTabIconShouldChangeColor.png (HEAD:src/Controls/tests/TestCases.Android.Tests/snapshots/android/SelectedTabIconShouldChangeColor.png vs. - fix screen shots:src/Controls/tests/TestCases.Android.Tests/snapshots/android/CollectionViewSelectionModeOnDarkTheme.png)
Auto-merging src/Controls/tests/TestCases.Android.Tests/snapshots/android/SelectedTabIconShouldChangeColor.png
CONFLICT (content): Merge conflict in src/Controls/tests/TestCases.Android.Tests/snapshots/android/SelectedTabIconShouldChangeColor.png
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0008 - fix screen shots
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
Description of Change
This pull request adds new test cases and improves safe area handling for navigation and shell pages on Android and iOS. It also enhances device compatibility checks for UI tests and clarifies padding logic for the Android app bar layout.
New Safe Area Test Cases:
Issue28986_NavigationPage
andIssue28986_Shell
test pages to verify per-edge safe area control in navigation and shell scenarios (Issue28986_NavigationPage.cs
,Issue28986_Shell.cs
). [1] [2]Issue28986_NavigationPage.cs
,Issue28986_Shell.cs
). [1] [2]Device Compatibility Improvements:
UITest.cs
to accept both "width x height" and "height x width" formats for better emulator compatibility.Safe Area Padding Logic:
AppBarLayout
on Android to correctly account for system bars and navigation bar placement, improving toolbar visibility and interaction across device orientations (GlobalWindowInsetListener.cs
).Generate any new Notch related screen shots
I did a local rerun of all the notch images just to make sure those are all up to date once we get that CI running.
Before and After
The picture below is on a Pixel 9.
The white part is where the camera is on the front of the screen, it's basically a NOTCH.