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.

Sign up for GitHub

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

Jump to bottom

[iOS][Regression] Fix ToolbarItem color when used with IconImageSource is always default color #26048

Merged
merged 14 commits into from
Dec 6, 2024
Merged

[iOS][Regression] Fix ToolbarItem color when used with IconImageSource is always default color #26048

merged 14 commits into from
Dec 6, 2024

Conversation

devanathan-vaithiyanathan
Copy link
Contributor

@devanathan-vaithiyanathan devanathan-vaithiyanathan commented Nov 22, 2024 •
edited
Loading

Root cause

The regression occurred because a previous PR changed the UIImageRenderingMode to Automatic, which caused the ToolbarItem icon to ignore the FontImageSource.Color.

Description of Change

To address this issue, the TintColor is explicitly set to the platform-specific equivalent of FontImageSource.Color. This ensures the toolbar icon respects and correctly displays the specified color.

Regressed PR

Issues Fixed

Fixes #25912

Tested the behaviour in the following platforms

  • Android
  • Windows
  • iOS
  • Mac

Output Screenshot

Before After

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Nov 22, 2024
@devanathan-vaithiyanathan devanathan-vaithiyanathan changed the title [iOS][Regression] Fix ToolbarItem color when used with IconImageSource is always white [iOS][Regression] Fix ToolbarItem color when used with IconImageSource is always default color Nov 22, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz added area-controls-toolbar ToolBar t/bug Something isn't working labels Nov 22, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@devanathan-vaithiyanathan devanathan-vaithiyanathan marked this pull request as ready for review November 22, 2024 15:01
@devanathan-vaithiyanathan devanathan-vaithiyanathan requested a review from a team as a code owner November 22, 2024 15:01
@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen added the p/0 Work that we can't release without label Nov 23, 2024
jsuarezruiz
jsuarezruiz previously requested changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

The regression is due this change https://github.com/dotnet/maui/pull/22437/files#diff-48977a1f6983e847819315f0fcab9bc88a0bf15ba282e24616895a7c65713774 and impact:

Could we fix also the menus in this PR?

src/Controls/tests/TestCases.HostApp/Issues/Issue25912.xaml Outdated


<ContentPage.Content>
<Label Text="ToolbarItemIconColorTest" AutomationId="Label"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could modify the sample to change the ToolbarItem color at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per your suggestion, I have modified the test case. Could you please validate the changes?

src/Controls/tests/TestCases.HostApp/Issues/Issue25912.xaml Show resolved Hide resolved
src/Controls/src/Core/Compatibility/iOS/Extensions/ToolbarItemExtensions.cs Show resolved Hide resolved
@devanathan-vaithiyanathan
Copy link
Contributor Author

The regression is due this change https://github.com/dotnet/maui/pull/22437/files#diff-48977a1f6983e847819315f0fcab9bc88a0bf15ba282e24616895a7c65713774 and impact:

Could we fix also the menus in this PR?

The issue does not occur when using IconImageSource with MenuItem.

@rmarinho
Copy link
Member

/azp run

@rmarinho rmarinho added this to the .NET 9 SR2 milestone Nov 26, 2024
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@sheiksyedm sheiksyedm added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024
@PureWeen
Copy link
Member

PureWeen commented Dec 6, 2024

I think this fix works for this scenario but I'm curious if we can more generally apply this to ImageSourceExtensions

The color gets applied via an AttributeString

image

Would it makes sense to just apply the TintColor here

image

So it's always just applied when using FontImages?

@PureWeen PureWeen merged commit 23e1522 into dotnet:main Dec 6, 2024
104 checks passed
@samhouts samhouts added fixed-in-9.0.21 fixed-in-net8.0-nightly This may be available in a nightly release! labels Dec 16, 2024
@Tamilarasan-Paranthaman Tamilarasan-Paranthaman mentioned this pull request Dec 20, 2024
Draft
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PureWeen PureWeen PureWeen approved these changes

@jsuarezruiz jsuarezruiz jsuarezruiz left review comments

@rmarinho rmarinho Awaiting requested review from rmarinho rmarinho is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

Assignees
No one assigned
Labels
area-controls-toolbar ToolBar community ✨ Community Contribution fixed-in-9.0.21 fixed-in-net8.0-nightly This may be available in a nightly release! p/0 Work that we can't release without partner/syncfusion Issues / PR's with Syncfusion collaboration platform/iOS 🍎 t/bug Something isn't working
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 9 SR2
Development

Successfully merging this pull request may close these issues.

[9.0] [iOS] [Regression] - ToolbarItem color when used with IconImageSource is always white.
6 participants
@devanathan-vaithiyanathan @jsuarezruiz @PureWeen @rmarinho @samhouts @sheiksyedm