Skip to content
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

[Android] Changing the style dynamically leaves the colors from the previous style #6183

Open
ivan-todorov-progress opened this issue Apr 18, 2022 · 12 comments
Labels
area-xaml XAML, CSS, Triggers, Behaviors p/2 Work that is important, but is currently not scheduled for release partner Issue or Request from a partner team platform/android 🤖 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working

Comments

@ivan-todorov-progress
Copy link

ivan-todorov-progress commented Apr 18, 2022

Description

It seems this is a new issue related to the Release Candidate 1. When changing a style dynamically, that style can override any number of properties via its setters. The problem occurs when we have non-overlapping setters like in the following example:

<Style x:Key="firstStyle" TargetType="local:CustomControl">
    <Setter Property="HeaderBackgroundColor" Value="LightGray" />
</Style>
<Style x:Key="secondStyle" TargetType="local:CustomControl">
    <Setter Property="HeaderTextColor" Value="LightPink" />
    <Setter Property="HeaderBackgroundColor" Value="DarkRed" />
</Style>

The first style changes the HeaderBackgroundColor property only, while the second one changes both the HeaderTextColor and HeaderBackgroundColor properties (check the attached project). When switching between these styles dynamically, the value of the HeaderTextColor property of the second style can affect the same property of the first style, even though the first style does not have a setter for that property. Similar behavior can be reproduced with just one style and VisualStateManager - to change a certain color based on the state of the control, when the base style does not define a value for that color.

This is rather unexpected and not consistent between the platforms. You can easily reproduce this behavior on Android (the color of the second style leaks to the first one), while it works differently on Windows (the color resets to the system default when switching to the first style). I believe the behavior on Windows is the correct one and is consistent with what XAML developers would normally expect.

After some investigation, I have found another issue that is meant to introduce this behavior on purpose: #1485. I do not know what is the reasoning behind this change, but introducing such an odd inconsistency between the platforms is a flawed decision in my books. Moreover, without having access to the system palette of the underlying platform, this makes it impossible to express the default platform colors in XAML. The proposed "solution" was to resort to platform-specific code to get the default colors. I do not know what other developers think, but forcing us to use platform-specific code to get the default look of the controls defeats the purpose of a "cross-platform" framework such as .NET MAUI.

Steps to Reproduce

  1. Run the attached sample project
    • The main page contains a custom control with a header and a content
  2. Click the "Set First Style" button
    • The background color of the header changes to gray
    • The text color of the header remains the system default color
  3. Click the "Set Second Style" button
    • The background color of the header changes to dark red
    • The text color of the header changes to pink
  4. Click the "Set First Style" button again
    • The background color of the header changes to gray
    • [Android]: The text color of the header remains pink
    • [Windows]: The text color of the header resets to the system default

TestApp.zip

Version with bug

Release Candidate 1 (current)

Last version that worked well

Preview 14

Affected platforms

Android

Affected platform versions

I have not tested on various versions of the different platforms.

Did you find any workaround?

The only workaround I have found is to always specify a value for the color. This might override the system default value leading to an inconsistent look and feel of the control.

Relevant log output

No response

@ivan-todorov-progress ivan-todorov-progress added s/needs-verification Indicates that this issue needs initial verification before further triage will happen t/bug Something isn't working labels Apr 18, 2022
@XamlTest XamlTest added s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage and removed s/needs-verification Indicates that this issue needs initial verification before further triage will happen labels Apr 18, 2022
@XamlTest
Copy link
Collaborator

Verified this issue with Visual Studio Enterprise 17.3.0 Preview 1.0 [32417.6.main]. Repro on Android with above project.

@jsuarezruiz jsuarezruiz added area-xaml XAML, CSS, Triggers, Behaviors partner Issue or Request from a partner team labels Apr 18, 2022
@jsuarezruiz
Copy link
Contributor

Related with #1485

@jsuarezruiz jsuarezruiz added this to the 6.0.300-rc.2 milestone Apr 18, 2022
@J-Swift
Copy link
Contributor

J-Swift commented Apr 18, 2022

Ive had similar issues and thoughts. I like “no default values” philosophy to avoid things like margin/padding that needs reset but I think the ergonomics of “reset to default” are lacking.

@PureWeen
Copy link
Member

@ivan-todorov-progress how much do you care specifically about the platform themes vs the style not resetting to a default? Do you typically modify the platform level themes?

@PureWeen PureWeen modified the milestones: 6.0.300-rc.2, 6.0.300-rc.3 Apr 18, 2022
@ivan-todorov-progress
Copy link
Author

ivan-todorov-progress commented Apr 19, 2022

@PureWeen My biggest concern is that the behavior is not consistent between the platforms. The change affects Android only, but the original behavior is kept on the other platforms. Additionally, this is a breaking change, because many Styles now have to specify hard-coded colors explicitly. For example:

<Style TargetType="CustomControl">
    <Setter Property="VisualStateManager.VisualStateGroups">
        <VisualStateGroupList>
            <VisualStateGroup Name="CommonStates">
                <VisualState Name="Normal" />
                <VisualState Name="Disabled">
                    <VisualState.Setters>
                        <Setter Property="TextColor" Value="Gray" />
                    </VisualState.Setters>
                </VisualState>
                <VisualState Name="Selected">
                    <VisualState.Setters>
                        <Setter Property="TextColor" Value="{x:Static Application.AccentColor}" />
                    </VisualState.Setters>
                </VisualState>
            </VisualStateGroup>
        </VisualStateGroupList>
    </Setter>
</Style>

The above style won't work correctly, because it does not specify TextColor by default. Once the component goes to a non-default state (e.g. Disabled, Selected) the TextColor is changed, but returning to the Normal state won't restore it back.

The workaround is to always specify the TextColor like this:

<Style TargetType="CustomControl">
    <Setter Property="TextColor" Value="Black" />
    <Setter Property="VisualStateManager.VisualStateGroups">
        <!-- Code omitted for brevity -->
    </Setter>
</Style>

The problem with the above approach is it won't work well with the dark theme. Now we need to specify different colors for the different themes:

<Style TargetType="CustomControl">
    <Setter Property="TextColor" Value="{AppThemeBinding Light='Black', Dark='White'}" />
    <Setter Property="VisualStateManager.VisualStateGroups">
        <!-- Code omitted for brevity -->
    </Setter>
</Style>

The above code would look somewhat better, but we have different default colors on the different platforms and for the different themes, so:

<Style TargetType="CustomControl">
    <Setter Property="TextColor">
        <OnPlatform>
            <On Platform="Android" Value="{AppThemeBinding Light='light Android color', Dark='dark Android color'}" />
            <On Platform="iOS" Value="{AppThemeBinding Light='light iOS color', Dark='dark iOS color'}" />
            <On Platform="UWP" Value="{AppThemeBinding Light='light UWP color', Dark='dark UWP color'}" />
            <On Platform="MacCatalyst" Value="{AppThemeBinding Light='light MacCatalyst color', Dark='dark MacCatalyst color'}" />
        </OnPlatform>
    </Setter>
    <Setter Property="VisualStateManager.VisualStateGroups">
        <!-- Code omitted for brevity -->
    </Setter>
</Style>

I believe you see the direction where this goes. Moreover, because changing the style of a control does not reset the colors, unless they are explicitly set, now we have to do this for all colors for just in case.

I mean, the bug is not a blocker, but leads to a rather unpleasant workaround that is hard to maintain.

Another option is to implement our own infrastructure to access the system palette in a platform-specific way:

public static class PlatformColors
{
	public static readonly Color LabelTextColor;
	public static readonly Color ButtonTextColor;

    static PlatformColors()
    {
#if ANDROID
		// Code for Android
#elif IOS
		// Code for iOS
#elif WINDOWS
		// Code for Windows
#elif MACCATALYST
		// Code for MacCatalyst
#endif
	}
}

This is the best approach IMHO, but you should consider the fact, that almost all developers using MAUI would have to implement such code themselves at some point, otherwise their applications won't look consistent with the current OS theme.

To lighten up the mood somewhat, Color.Default is like the janitor of a building: you won't notice his/her importance, unless he/she is missing. ;-)

@Redth Redth modified the milestones: 6.0.300-rc.3, 6.0.300 Apr 27, 2022
@davidortinau davidortinau added the p/0 Work that we can't release without label Apr 30, 2022
@StephaneDelcroix
Copy link
Contributor

The first style changes the HeaderBackgroundColor property only, while the second one changes both the HeaderTextColor and HeaderBackgroundColor properties (check the attached project). When switching between these styles dynamically, the value of the HeaderTextColor property of the second style can affect the same property of the first style, even though the first style does not have a setter for that property. Similar behavior can be reproduced with just one style and VisualStateManager - to change a certain color based on the state of the control, when the base style does not define a value for that color.

when you replace a style by another, the first one is unapplied first. this is why HeaderTextColor can be modified while applying firstStyle after secondStyle

@Redth Redth self-assigned this May 2, 2022
@Redth Redth modified the milestones: 6.0.300, 6.0.300-servicing May 2, 2022
@Redth Redth removed their assignment May 2, 2022
@PureWeen
Copy link
Member

PureWeen commented May 2, 2022

The ultimate plan here is to define these colors in a more consumable and visible way for the users.

We're currently unable to do this in an easy manner because of this
#6350

I have a spiked PR that isn't the happiest of solutions but it basically achieves the end goal through means of code.

Overall there are two different discussion points here though

  1. do the platform colors matter to such an extent that we need to draw from them? Are there cases where we need to assert that providing an xplat representation of a color is impossible and we have to revert to the theme? This point has nothing to do with resetting to defaults in cases like a VSM. The discussion here is more about users that want to do all of their themes at a platform level and then mix that with xplat colors. If this is what users want then we should figure out APIs for this. Bring back Color.PlatformDefault or create platform binding markup extensions.

  2. how do we provide the best xplat theming experience for users? I put forward that relying on the platform theme's is overall a cop-out and a leaky abstraction for an xplat user. In order to color something on the screen I shouldn't need to be aware of how theming works in all .NET MAUI supported frameworks. Also, if you look at the android defaults for Forms our accent color was hot pink :-) You know how you can tell a Forms app? the status bar is that light blue color because most users don't want to figure out how to change it to a different color.

Overall this comes down to what we currently have time to do. The styles we've included with the templates can be copied over to a users app. They will have better Dark/Light support than they had in Forms and they will have a central place to style all the colors for the app. I see that as taking steps forward to a better xplat world. I also realize this doesn't fix the "set to null case" but I put forward that explicitly setting something to null to get back to a defaultValue inside a XAML based framework is wrong. This also doesn't work on WinUI. In XAML if you want to reset to your default value then you should use ClearValue which works perfectly if we have logical defaults defined inside an xplat style. Defining smart defaults is also useful when we get property explorer working with Live Visual Tree. Because now all of the colors will have actual values instead of just being empty fields.

@ivan-todorov-progress
Copy link
Author

ivan-todorov-progress commented May 3, 2022

The first style changes the HeaderBackgroundColor property only, while the second one changes both the HeaderTextColor and HeaderBackgroundColor properties (check the attached project). When switching between these styles dynamically, the value of the HeaderTextColor property of the second style can affect the same property of the first style, even though the first style does not have a setter for that property. Similar behavior can be reproduced with just one style and VisualStateManager - to change a certain color based on the state of the control, when the base style does not define a value for that color.

when you replace a style by another, the first one is unapplied first. this is why HeaderTextColor can be modified while applying firstStyle after secondStyle

@StephaneDelcroix This is correct, however un-applying a Style does not work for null values, i.e. if a Style sets the TextColor property, but the original value of that property is null, un-applying the Style won't restore the original color - the value from the Style would still remain applied to the native Control. This is "by design" and this is for Android only, on other platforms the value would be unapplied correctly. I believe you can see the problem.

@PureWeen
Copy link
Member

PureWeen commented May 3, 2022

@PureWeen My biggest concern is that the behavior is not consistent between the platforms.

@ivan-todorov-progress this is also my current biggest concern :-( The plan for null to be a no-op wasn't consistently applied as developers worked on core. So now here we are. Ideally things would be symmetrically "broken"

I'm hoping we can make this all more symmetric pretty soon after GA

@Redth Redth added p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint and removed p/0 Work that we can't release without labels May 3, 2022
@ghost
Copy link

ghost commented Aug 30, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@samhouts samhouts modified the milestones: Backlog, .NET 8 Planning Jan 26, 2023
@samhouts samhouts removed s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Apr 5, 2023
@homeyf homeyf added the s/triaged Issue has been reviewed label Apr 20, 2023
@homeyf
Copy link
Collaborator

homeyf commented Apr 20, 2023

Verified this issue with Visual Studio Enterprise 17.6.0 Preview 2.0. Repro on android emulator with above project.
image

@homeyf homeyf added the s/verified Verified / Reproducible Issue ready for Engineering Triage label Apr 20, 2023
@samhouts samhouts modified the milestones: .NET 8 Planning, .NET 8 May 16, 2023
@samhouts samhouts modified the milestone: .NET 8 + Servicing Sep 26, 2023
@RoyM33-T
Copy link

This is still an issue, Setting in the visual state group causes no ovvrride to work unless you also do the visual state group in that xaml as well.

<Application.Resources>
    <ResourceDictionary>
        <Style TargetType="Button" ApplyToDerivedTypes="True">
            <Setter Property="VisualStateManager.VisualStateGroups">
                <VisualStateGroupList>
                    <VisualStateGroup x:Name="CommonStates">
                          <VisualState x:Name="Normal">
                              <VisualState.Setters>
                                  <Setter Property="CornerRadius" Value="5" />                                 
                              </VisualState.Setters>
                          </VisualState>
                        </VisualStateGroup>
                    </VisualStateGroupList>
                </Setter>
            </Style>
        </ResourceDictionary>
    </Application.Resources>

Makes

<Button CornerRadius="100"/>

Not Work unless you do it like this

<Button >
    <VisualStateManager.VisualStateGroups>
        <VisualStateGroupList>
            <VisualStateGroup x:Name="CommonStates">
                <VisualState x:Name="Normal">
                    <VisualState.Setters>
                        <Setter Property="CornerRadius" Value="100" />
                    </VisualState.Setters>
                </VisualState>
            </VisualStateGroup>
        </VisualStateGroupList>
    </VisualStateManager.VisualStateGroups>
</Button>

@PureWeen PureWeen added p/2 Work that is important, but is currently not scheduled for release and removed p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint labels Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-xaml XAML, CSS, Triggers, Behaviors p/2 Work that is important, but is currently not scheduled for release partner Issue or Request from a partner team platform/android 🤖 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
None yet
Development

No branches or pull requests