[Registry Preview] Update all AppBarButtons to use an explicit AppBarButton.Icon (Fix for 27766) #27770
Conversation
Removes the Icon property from all AppBarButtons and adds FontFamily for all Glyph's
randyrants
added
the
Product-Registry Preview
| @@ -95,7 +95,7 @@ | |||
| x:Uid="OpenButton" | |||
| Click="OpenButton_Click"> | |||
| <AppBarButton.Icon> | |||
| <FontIcon Glyph=""/> | |||
| <FontIcon FontFamily="{StaticResource SymbolThemeFontFamily}" Glyph=""/> | |||
There was a problem hiding this comment.
Can't hurt to declare it, but it's default and thus redundant.
See https://learn.microsoft.com/nl-nl/windows/windows-app-sdk/api/winrt/microsoft.ui.xaml.controls.fonticon?view=windows-app-sdk-1.2#default-font-family
To use glyphs from the default system font, don't set the FontFamily property, let it use its default value.
There was a problem hiding this comment.
I keep going back to this from when I made the change, regardless of what the docs tell us:
"I agree it should, but the fallback behavior with zh-Hans hasn't, well, fallen back as it should. On Win11, it should use Segoe Fluent Icons, but on Win10 it should look for Segou MDL2 Assets, and if Windows 8 was supported, it should look for Segoe UI Symbol. For LIXiangChen, Windows 10 with Simplified Chinese, the original code picked up an unexpected font for the icon, which is the oddity. My hope is that by calling out the StaticResource explicitly, it will behave better."
I'll think it's better to have it.
| @@ -114,9 +116,11 @@ | |||
| <AppBarButton | |||
| x:Name="saveButton" | |||
| x:Uid="SaveButton" | |||
| Icon="Save" | |||
There was a problem hiding this comment.
Good point. I know that the SymbolIcon element is bad, but looks like the Appbar icon leverages the same thing. 👍
There was a problem hiding this comment.
Completely agree, if the fallback logic for all the OS's and all the languages worked. It started using the Icon property and only went the more pedantic route after the bug for Win 10 in traditional Chinese was reported.
There was a problem hiding this comment.
What I was thinking of was more the glyphs it uses; all in E1** range
No ask for a hotfix but getting it out there with 0.73.0 should be good. |
src/modules/registrypreview/RegistryPreviewUI/RegistryPreviewXAML/MainWindow.xaml
Outdated
Show resolved
Hide resolved
Removing a "Windows 11-only" glyph to prevent UI issues.
Summary of the Pull Request
A recent update of awesomeness came through this app, cleaning up the UI in various ways, which is amazing. However, a bug may have snuck back into the code around the icons used in the commandbar in various languages; this brings the fix back into the code.
PR Checklist
Detailed Description of the Pull Request / Additional comments
In the XAML file, I've removed all references to Icon="Name" and replaced it with a corresponding FontIcon; I also added an explicit FontFamily to the FontIcon to avoid issues across Windows versions and languages, per the "best practices" called out in the WinUI 3 documentation.
Validation Steps Performed
Visually verified the UI:
