-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix Shell.Background not working #35491
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: net11.0
Are you sure you want to change the base?
Changes from all commits
3518f9c
ce45092
54687e2
3343558
082e786
4aec226
af58706
ab64db9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,18 +21,22 @@ public ShellToolbarAppearanceTracker(IShellContext shellContext) | |
| public virtual void SetAppearance(AToolbar toolbar, IShellToolbarTracker toolbarTracker, ShellAppearance appearance) | ||
| { | ||
| var foreground = appearance.ForegroundColor; | ||
| var background = appearance.BackgroundColor; | ||
| var background = !Brush.IsNullOrEmpty(appearance.Background) | ||
| ? appearance.Background | ||
| : appearance.BackgroundColor is not null | ||
| ? new SolidColorBrush(appearance.BackgroundColor) | ||
| : null; | ||
| var titleColor = appearance.TitleColor; | ||
|
|
||
| SetColors(toolbar, toolbarTracker, foreground, background, titleColor); | ||
| } | ||
|
|
||
| public virtual void ResetAppearance(AToolbar toolbar, IShellToolbarTracker toolbarTracker) | ||
| { | ||
| SetColors(toolbar, toolbarTracker, ShellRenderer.DefaultForegroundColor, ShellRenderer.DefaultBackgroundColor, ShellRenderer.DefaultTitleColor); | ||
| SetColors(toolbar, toolbarTracker, ShellRenderer.DefaultForegroundColor, new SolidColorBrush(ShellRenderer.DefaultBackgroundColor), ShellRenderer.DefaultTitleColor); | ||
| } | ||
|
|
||
| protected virtual void SetColors(AToolbar toolbar, IShellToolbarTracker toolbarTracker, Color foreground, Color background, Color title) | ||
| protected virtual void SetColors(AToolbar toolbar, IShellToolbarTracker toolbarTracker, Color foreground, Brush background, Color title) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This changes the shipped This is intentional and correctly tracked, targets (found by: gemini-3.1-pro-preview, claude-opus-4.8) |
||
| { | ||
| if (_disposed) | ||
| return; | ||
|
|
@@ -43,7 +47,7 @@ protected virtual void SetColors(AToolbar toolbar, IShellToolbarTracker toolbarT | |
| return; | ||
|
|
||
| shellToolbar.BarTextColor = title ?? ShellRenderer.DefaultTitleColor; | ||
| shellToolbar.BarBackground = new SolidColorBrush(background ?? ShellRenderer.DefaultBackgroundColor); | ||
| shellToolbar.BarBackground = background ?? new SolidColorBrush(ShellRenderer.DefaultBackgroundColor); | ||
| shellToolbar.IconColor = foreground ?? ShellRenderer.DefaultForegroundColor; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,11 +131,26 @@ void UpdateiOS13NavigationBarAppearance(UINavigationController controller, Shell | |
| navBar.TintColor = _defaultTint; | ||
| } | ||
|
|
||
| // Set BackgroundColor | ||
| var background = appearance.BackgroundColor; | ||
|
|
||
| if (background != null) | ||
| navigationBarAppearance.BackgroundColor = background.ToPlatform(); | ||
| // Set Background (prefer Brush over Color for gradient support) | ||
| if (!Brush.IsNullOrEmpty(appearance.Background)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The translucency decision a few lines above (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches the existing NavigationRenderer behavior, where translucency is also driven by color alpha and brush backgrounds are applied afterward (see NavigationRenderer.cs:881 and NavigationRenderer.cs:896).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In Fix: factor the brush alpha into the translucency decision (e.g. (found by: gemini-3.1-pro-preview, claude-opus-4.8)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches existing NavigationRenderer behavior, where translucency is determined from BackgroundColor alpha first, and brush imagery is applied afterward (NavigationRenderer.cs:881, NavigationRenderer.cs:898, NavigationRenderer.cs:900). Opaque gradients, which are the primary scenario for this PR, are unaffected. Semi-transparent brush parity can be handled as a follow-up enhancement. |
||
| { | ||
| if (appearance.Background is SolidColorBrush solidBrush && solidBrush.Color is not null) | ||
| { | ||
| navigationBarAppearance.BackgroundColor = solidBrush.Color.ToPlatform(); | ||
| } | ||
| else | ||
| { | ||
| var backgroundImage = navBar.GetBackgroundImage(appearance.Background); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion: Gradient navbar background is rendered to a fixed-size image and not refreshed on rotation/resize For non-solid brushes the iOS path renders the gradient to a (found by: claude-opus-4.6)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a valid concern |
||
| if (backgroundImage is not null) | ||
| { | ||
| navigationBarAppearance.BackgroundImage = backgroundImage; | ||
| } | ||
| } | ||
| } | ||
| else if (appearance.BackgroundColor is not null) | ||
| { | ||
| navigationBarAppearance.BackgroundColor = appearance.BackgroundColor.ToPlatform(); | ||
| } | ||
|
|
||
| // Set TitleColor | ||
| var titleColor = appearance.TitleColor; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -529,6 +529,13 @@ static void OnFlyoutBehaviorChanged(BindableObject bindable, object oldValue, ob | |
| BindableProperty.CreateAttached("UnselectedColor", typeof(Color), typeof(Shell), null, | ||
| propertyChanged: OnShellAppearanceValueChanged); | ||
|
|
||
| /// <summary> | ||
| /// Defines the background brush for the Shell toolbar. Supports gradient brushes. | ||
| /// </summary> | ||
| public static readonly new BindableProperty BackgroundProperty = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Concrete footgun verified at HEAD: This is the dominant unresolved item on the PR (already raised in the PR discussion and a prior net11 fleet-review). It is an API-design decision that should go through API review before shipping, even on net11.0. Consider honoring/reusing (found by: gpt-5.5, claude-opus-4.6, claude-opus-4.8)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This follows the existing Shell pattern — Shell.BackgroundColorProperty (line 467) also uses public static readonly new to shadow VisualElement.BackgroundColorProperty. The same attached-vs-instance split already exists for BackgroundColor. This is established Shell architecture, not new to this PR. |
||
| BindableProperty.CreateAttached("Background", typeof(Brush), typeof(Shell), Brush.Default, | ||
| propertyChanged: OnShellAppearanceValueChanged); | ||
|
|
||
| /// <summary> | ||
| /// The backdrop of the flyout, which is the appearance of the flyout overlay. | ||
| /// </summary> | ||
|
|
@@ -713,6 +720,20 @@ static void OnFlyoutBehaviorChanged(BindableObject bindable, object oldValue, ob | |
| /// <param name="value">The brushed used in the backdrop of the flyout.</param> | ||
| public static void SetFlyoutBackdrop(BindableObject obj, Brush value) => obj.SetValue(FlyoutBackdropProperty, value); | ||
|
|
||
| /// <summary> | ||
| /// Gets the background brush for the Shell toolbar. | ||
| /// </summary> | ||
| /// <param name="obj">The object from which to get the background brush.</param> | ||
| /// <returns>The background brush for the Shell toolbar.</returns> | ||
| public static Brush GetBackground(BindableObject obj) => (Brush)obj.GetValue(BackgroundProperty); | ||
|
|
||
| /// <summary> | ||
| /// Sets the background brush for the Shell toolbar. | ||
| /// </summary> | ||
| /// <param name="obj">The object on which to set the background brush.</param> | ||
| /// <param name="value">The brush to use as the Shell toolbar background.</param> | ||
| public static void SetBackground(BindableObject obj, Brush value) => obj.SetValue(BackgroundProperty, value); | ||
|
|
||
| static void OnShellAppearanceValueChanged(BindableObject bindable, object oldValue, object newValue) | ||
| { | ||
| var item = (Element)bindable; | ||
|
|
@@ -816,7 +837,14 @@ void UpdateToolbarAppearanceFeatures(Element pivot, ShellAppearance appearance) | |
| { | ||
| appearance = appearance ?? GetAppearanceForPivot(pivot); | ||
| Toolbar.BarTextColor = appearance?.TitleColor ?? DefaultTitleColor; | ||
| Toolbar.BarBackground = appearance?.BackgroundColor ?? DefaultBackgroundColor; | ||
| if (!Brush.IsNullOrEmpty(appearance?.Background)) | ||
| { | ||
| Toolbar.BarBackground = appearance.Background; | ||
| } | ||
| else | ||
| { | ||
| Toolbar.BarBackground = appearance?.BackgroundColor ?? DefaultBackgroundColor; | ||
| } | ||
| Toolbar.IconColor = appearance?.ForegroundColor ?? DefaultForegroundColor; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 10445, "Shell.Background does not support gradient brushes", PlatformAffected.All)] | ||
| public class Issue10445 : TestShell | ||
| { | ||
| protected override void Init() | ||
| { | ||
| FlyoutBehavior = FlyoutBehavior.Disabled; | ||
| var gradientBrush = new LinearGradientBrush | ||
| { | ||
| StartPoint = new Point(0, 0), | ||
| EndPoint = new Point(1, 1), | ||
| GradientStops = new GradientStopCollection | ||
| { | ||
| new GradientStop(Colors.Yellow, 0.0f), | ||
| new GradientStop(Colors.Green, 1.0f) | ||
| } | ||
| }; | ||
|
|
||
| Shell.SetBackground(this, gradientBrush); | ||
|
|
||
| var page = new ContentPage | ||
| { | ||
| Title = "Gradient Shell", | ||
| Content = new VerticalStackLayout | ||
| { | ||
| Padding = 20, | ||
| Spacing = 10, | ||
| VerticalOptions = LayoutOptions.Center, | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| Children = | ||
| { | ||
| new Label | ||
| { | ||
| Text = "Shell.Background should display a gradient (Yellow to Green) in the navigation bar above.", | ||
| AutomationId = "GradientInfoLabel", | ||
| HorizontalTextAlignment = TextAlignment.Center | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| AddContentPage(page, "Home"); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue10445 : _IssuesUITest | ||
| { | ||
| public Issue10445(TestDevice device) : base(device) | ||
| { | ||
| } | ||
|
|
||
| public override string Issue => "Shell.Background does not support gradient brushes"; | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.Shell)] | ||
| public void ShellBackgroundSupportsGradientBrush() | ||
| { | ||
| // Wait for the page to be fully loaded | ||
| App.WaitForElement("GradientInfoLabel"); | ||
|
|
||
| // Verify the Shell renders correctly with a gradient background | ||
| VerifyScreenshot(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add the Windows and Mac baseline images once CI is triggered.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ Error: New
Verified at HEAD Fix: commit the (found by: claude-opus-4.8)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the Windows and Mac base images. |
||
| } | ||
| } | ||
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.
This changes the
protected virtualsignature fromColor backgroundtoBrush background. It is correctly tracked in PublicAPI with a*REMOVED*/added pair and is reasonable for the net11.0 target, but it is binary/source-breaking for subclasses that override the oldColoroverload: after this changeSetAppearancecalls theBrushoverload, so an existing override of theColoroverload silently stops being invoked. Worth confirming during API review that dropping the old overload (vs. keeping it as an[Obsolete]overload that delegates to the new one) is the intended net11 break — this is the kind of decision that pairs with the broaderShell.BackgroundAPI-design discussion on this PR.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.
Thanks for flagging this! Yes, this is an intentional change for .NET 11 — we've updated the signature from Color to Brush to enable gradient background support. It's properly tracked in PublicAPI.Unshipped.txt with the REMOVED/added pair. For anyone with a custom subclass overriding the old Color overload, the migration is straightforward — just update the parameter type to Brush. Happy to discuss further if there are concerns!