Proposal: Remove unused theme resources from NavigationView

[Felix-Dev]

Proposal: Remove unused theme resources from NavigationView

While checking the NavigationView theme resources file for proposal #2754 I've noticed certain theme resources not being used anywhere in the NavigationView. These theme resources are the following:

NavigationViewItemBackgroundChecked
NavigationViewItemBackgroundCheckedPointerOver
NavigationViewItemBackgroundCheckedPressed
NavigationViewItemBackgroundCheckedDisabled

NavigationViewItemForegroundChecked
NavigationViewItemForegroundCheckedPointerOver
NavigationViewItemForegroundCheckedPressed
NavigationViewItemForegroundCheckedDisabled

NavigationViewItemBorderBrushChecked
NavigationViewItemBorderBrushCheckedPointerOver
NavigationViewItemBorderBrushCheckedPressed

A WinUI solution-wide search in Visual Studio only showed these theme resources being defined in the NavigationView_rs1_themeresources.xaml and NavigationView_rs2_themeresources.xaml files but completely unused otherwise. (I also didn't see them in use when these files were initially uploaded to the repo in November 2018.) To the best of my knowledge, there is also no "Checked" visual state for a NavigationViewItem and this is also reinforced by the fact that no matching TopNavigationViewItemForegroundChecked theme resource exists.

Removing these theme resources would be a breaking change if they are used in WinUI projects. However, I can't imagine these theme resources being consumed in significant numbers. As such, based on my current understanding, I propose removing them (and perhaps document their removal in the changelog for the WinUI release affected).

The theme resource NavigationViewItemBorderBrushCheckedDisabled is used, for example, here https://github.com/microsoft/microsoft-ui-xaml/blob/1c6717881cbcf990056625912039f89a7c5781fb/dev/NavigationView/NavigationView_rs1_themeresources.xaml#L601 though it appears as an error to me. I think the theme resource NavigationViewItemBorderBrushDisabled was meant to be used here (it's currently unused) as it makes much more sense given the visual state ("Disabled" and not "CheckedDisabled"). The use of that theme resource would also match the other visual states above it, like this one: https://github.com/microsoft/microsoft-ui-xaml/blob/1c6717881cbcf990056625912039f89a7c5781fb/dev/NavigationView/NavigationView_rs1_themeresources.xaml#L590 Instead, NavigationViewItemBorderBrushDisabled is currently unused in the NavigationView and thus might confuse developers because having to use NavigationViewItemBorderBrushCheckedDisabled here is not intuitive simply by looking at the name of the resource. Especially when we see that a theme resource like NavigationViewItemBorderBrush is used instead of the NavigationViewItemBorderBrushChecked theme resource throughout the NavigationView (same for the corresponding PointerOver/Pressed theme resources).

Another use of NavigationViewItemBorderBrushCheckedDisabled can be found here https://github.com/microsoft/microsoft-ui-xaml/blob/1c6717881cbcf990056625912039f89a7c5781fb/dev/NavigationView/NavigationView_rs1_themeresources.xaml#L960 and again this appears as an error to me as there is no "CheckedDisabled" visual state. The theme resource used here should probably be TopNavigationViewItemBorderBrushDisabled which currently does not exist and likely should be added.

As such, I also propose replacing the - I believe mistakenly - used NavigationViewItemBorderBrushCheckedDisabled theme resource with correct theme resources such as NavigationViewItemBorderBrushDisabled and TopNavigationViewItemBorderBrushDisabled and document its replacement accordingly in the changelog of the WinUI release this breaking change would be included in.

Thoughts by the team?

[StephenLPeters]

@ojhad FYI. I think we might run into issues that it is possible that customers have taken a dependency on these and that by deleting them we could break there apps. If we were to do this we'd want to investigate and if so make the messaging clear.

[StephenLPeters]

@YuliKl and @anawishnoff FYI

[Felix-Dev]

@StephenLPeters I have exactly the same concerns. There will probably be a few customers who are using one or more of these resources in some of their apps. I don't know how many and I also don't know the history about these theme resources which appear to have never been used in WinUI (but perhaps in an older Windows SDK release).

I do think though that there is considerable benefit to get by removing/replacing these theme resources as they at best appear to be quite outdated and at worst just confusingly and inconsistently used (like the case of NavigationViewItemBorderBrushCheckedDisabled vs NavigationViewItemBorderBrushDisabled as outlined above). Semantic Versioning and a well-documented release changelog can be used by us here to mitigate the effect of this breaking change - if we decide to go through with this.

[Felix-Dev]

@StephenLPeters As a compromise for now if the team views this as too risky a change in terms of breaking apps, I could move these theme resources to the bottom of their respective theme directories and add a note directly above them stating that these theme resources are no longer in use and could be removed in a future WinUI release. How does that sound?

Ultimately, I still want them to be removed though and we definitely should - from time-to-time and with the right communication - cleanup some "waste" accumulated over the years in the theme resources.