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

[MacOS/iOS] Fixed incorrect color of RadioButton in Dark Mode using Default Control Template. #13215

Merged
merged 16 commits into from
Mar 14, 2023

Conversation

dustin-wojciechowski
Copy link
Contributor

Description of Change

The border color and fill colors of the RadioButton were not being resolved correctly due to it being unable to determine the application's current theme. I moved their initialization to the method where the ellipses are drawn.

Added test CorrectDefaultRadioButtonThemeColorsInLightAndDarkModes in AppThemeTests to validate that the Default Control Template for RadioButton was indeed producing the correct colors in light mode and dark mode.
image
image

Issues Fixed

Fixes #5931

…kThemeColor into BuildDefaultTemplate() in order for correct colors to display when MacOS/iOS is in Dark Mode.
@dnfadmin
Copy link

dnfadmin commented Feb 8, 2023

CLA assistant check
All CLA requirements met.

@dustin-wojciechowski dustin-wojciechowski marked this pull request as ready for review February 8, 2023 23:35
@Eilon Eilon added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Feb 8, 2023
@jsuarezruiz jsuarezruiz added platform/macOS 🍏 macOS / Mac Catalyst platform/iOS 🍎 area-controls-radiobutton RadioButton, RadioButtonGroup labels Feb 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

@dustin-wojciechowski
Copy link
Contributor Author

I implemented requested changes for outer ellipse as well as the checkmark. I also brought the changes over to the Tizen version, although I'm not a fan of repeating stuff across two files :(

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, lets try to have the smaller number of strings in the assembly.

src/Controls/src/Core/RadioButton.cs Outdated Show resolved Hide resolved
// First, Light/Dark App themes for outer ellipse, then for the check mark.
// Then check for older themecolor.
// If nothing set, use the defaults for light/dark mode.
if (Application.Current.TryGetResource(RadioButtonOuterEllipseStrokeLight, out var outerLight) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it's a bit funky because of needing to test that old key. Is there a way we can consolidate this and the similar code below this block into an extension method?

if (normalEllipse.TrySetAppTheme(
   lightResourceKey: RadioButtonOuterEllipseStrokeLight,
   darkResourceKey: RadioButtonOuterEllipseStrokeDark,
   bindableProperty: Ellipse.StrokeProperty,
   defaultDark: SolidColorBrush.Black,
   defaultLight: SolidColorBrush.White,
   out var outerLight,
   out var outerDark))

I also wonder if we want to make this if statement an OR vs an AND.

If the user has only specified RadioButtonOuterEllipseStrokeLight then we probably still want this logic to run and then we'd just set the other side of the AppThemeBinding to whatever the default is?

@mattleibow
Copy link
Member

I see this issue is related to color things, so maybe this is useful to test as well. #12805

…, TrySetDynamicTheme, TrySetFill. Updated RadioButton code to use those methods. Removed old ResolveThemeColor method.
BindableProperty bindableProperty,
out object outerColor)
{
if (Application.Current.TryGetResource(resourceKey, out outerColor))
Copy link
Member

@PureWeen PureWeen Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StephaneDelcroix is this the best way to retrieve this resource? It seems like we'd retrieve the static resource once the DataTemplate is realized and added to the visual tree, that way it can check all the merged dictionaries?

Like, ideally this could use
radioButton.TryGetResource right? That way if the resources are specified on the page or the RadioButton itself they'll work?

Just not sure off hand how to do that in the code.

@stcmz
Copy link

stcmz commented May 10, 2023

Could you also merge the fix for .net 7.0?

@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
@samhouts samhouts added the fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-radiobutton RadioButton, RadioButtonGroup fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS Radio Button displays with same black coloring in Dark Mode RadioButton has a weird dark background
9 participants