microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.3k stars 676 forks source link

Icon on MenuFlyoutItem within a dark container turns black on pointer exit (with OS in light mode theme color) #5381

Open lyahdav opened 3 years ago

lyahdav commented 3 years ago

Steps to reproduce the bug

  1. Clone or download https://github.com/lyahdav/CppWinRtUwpSandbox
  2. Run solution https://github.com/lyahdav/CppWinRtUwpSandbox/blob/main/cppwinrt/Calendar.sln
  3. Click MenuFlyout button
  4. Hover over the 2nd menu item and then hover away from it

Expected behavior Icon for the menu item stays white. Instead it changes to black.

Screenshots

https://user-images.githubusercontent.com/359157/124684321-b002fd00-de83-11eb-91cb-c3d780cf5215.mov

Version Info Windows 10 21H1 / 19043.928 Islands

Device form factor Saw the problem?
Desktop Yes
Xbox
Surface Hub
IoT

Additional context The relevant code is here: https://github.com/lyahdav/CppWinRtUwpSandbox/blob/main/SharedContent/cppwinrt/MainPage.cpp#L65-L91. I have two workarounds there as well:

  1. Reload the icon on PointerExited. This isn't ideal since it flickers.
  2. Explicitly set BitmapIcon.Foreground(nullptr).
asklar commented 3 years ago

@jonthysell

lyahdav commented 3 years ago

I realized my workaround with setting Foreground breaks light mode. Here's a better workaround:

            menuFlyout.Opened([=](const auto &...) {
                if (menuFlyoutItem.ActualTheme() == ElementTheme::Dark) {
                    menuFlyoutItem.Icon().Foreground(nullptr);
                }
                });

Note Opening would be nicer to use but it doesn't work the first time a MenuFlyout is shown with ShowAt. It seems the ActualTheme on the MenuFlyoutItem gets set after the Opening event if using ShowAt.

ranjeshj commented 3 years ago

If we are updating the foreground in the style during pointer over, it is likely breaking the propagation of the foreground property. The fix might be to explicitly to template bind the foreground property in menuflyoutitem.

lyahdav commented 2 years ago

One more consideration with the Foreground(nullptr) workaround: you need to not do this workaround if the menu item is disabled, otherwise it'll have the wrong icon color in that case.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 5 days.

lyahdav commented 1 year ago

Likely still relevant

Marv51 commented 7 months ago

I am encountering this a lot in our app. Maybe 2024 can be the year the this gets fixed?

This makes it really difficult to implement a dark/light mode switch that applies the theme live without an app restart.

My use-case is like this: I have a menu and also various context menus in my app. On the root control I set RequestedTheme and the BitmapIcons all experience this bug.

I will investigate the nullptr workaround, but this should just work.

Marv51 commented 7 months ago

I looked into this issue, It took me less than 15 minutes to figure out what is wrong and how to fix it.

@ranjeshj is right, almost 3 years ago, but nobody in a position to get this fixed was able to look at this.

In controls/dev/CommonStyles/MenuFlyout_themeresources.xaml (approx. line 385) this is the fix:

<Style TargetType="MenuFlyoutItem" x:Key="DefaultMenuFlyoutItemStyle">
[...]
<Setter Property="Template">
[...]
 <Viewbox x:Name="IconRoot" HorizontalAlignment="Left" VerticalAlignment="Center" Width="16" Height="16" Visibility="Collapsed">
              <ContentPresenter x:Name="IconContent" **Foreground="{TemplateBinding Foreground}"**
Content="{TemplateBinding Icon}" />
            </Viewbox>
[...]
</Setter>
[...]
</Style>

The ContentPresenter "IconContent" is missing that Foreground property I added above.

I will make a custom style that is a copy of that style with only that little fix for my app, but please don't let this easy fix sit around for another 3 years.

Note: I have verified that ToggleMenuFlyoutItem has the same issue and the same fix applies. In that file a couple more FlyoutItem Styles exists, it looks like they all suffer from the same bug.