Microsoft-ui-xaml: Proposal: Allow for full control over the styling of NavigationViewItemSeparator
Proposal: Allow for full control over the styling of NavigationViewItemSeparator
Summary
@joshkoon:
NavigationViewItemSeparator supports the Style property, but ignores many of the setters. Proposal is to update the control to allow apps to have more control over the appearance.
@anawishnoff:
ThemeResources will be added for various styling properties of NavigationViewItemSeparator to allow developers to have a lightweight styling experience and customize the look of their separators without having to edit a control template.
Solution proposed by @Felix-Dev.
Rationale
@joshkoon:
- Inflexibility of control appearance forces developers to retemplate for trivial changes, such as margins.
Scope
| Capability | Priority |
| :---------- | :------- |
| This proposal will allow developers to customize the appearance of the Separator | Must |
| ThemeResources will be created for various styling properties of NavigationViewItemSeparator | Must |
| NavigationViewItemSeparator will use new ThemeResources within its ControlTemplate | Must |
Important Notes
This proposal aims to create ThemeResources in the NavigationView Resource Dictionary:
Margin:
<Thickness x:Key="TopNavigationViewItemSeparatorMargin">10,0</Thickness>
```xml
```xml
<Thickness x:Key="NavigationViewCompactItemSeparatorMargin">16,10</Thickness>
Foreground:
<StaticResource x:Key="NavigationViewItemSeparatorForeground" ResourceKey="SystemControlForegroundBaseLowBrush" />
```xml
Height for left-docked NavigationView:
```xml
<x:Double x:Key="NavigationViewItemSeparatorHeight">1</x:Double>
Width for top-docked NavigationView:
<x:Double x:Key="TopNavigationViewItemSeparatorWidth">1</x:Double>
joshkoon
All 20 comments
@ojhad fyi
StephenLPeters
on 20 Mar 2020
This is the control template for NavigationViewItemSeparator:
<Style TargetType="local:NavigationViewItemSeparator">
<Setter Property="IsEnabled" Value="False" />
<Setter Property="IsTabStop" Value="False" />
<Setter Property="MinHeight" Value="0" />
<Setter Property="AutomationProperties.AccessibilityView" Value="Raw" />
<Setter Property="Template">
<Setter.Value>
<ControlTemplate>
<Grid x:Name="NavigationViewItemSeparatorRootGrid">
<VisualStateManager.VisualStateGroups>
<VisualStateGroup x:Name="NavigationSeparatorLineStates">
<VisualState x:Name="HorizontalLine" />
<VisualState x:Name="VerticalLine">
<VisualState.Setters>
<Setter Target="SeparatorLine.Height" Value="20" />
<Setter Target="SeparatorLine.Width" Value="1" />
<Setter Target="SeparatorLine.Margin" Value="10,0" />
<Setter Target="SeparatorLine.VerticalAlignment" Value="Center" />
</VisualState.Setters>
</VisualState>
</VisualStateGroup>
</VisualStateManager.VisualStateGroups>
<Rectangle
x:Name="SeparatorLine"
Height="1"
Margin="16,10"
Fill="{ThemeResource SystemControlForegroundBaseLowBrush}" />
</Grid>
</ControlTemplate>
</Setter.Value>
</Setter>
</Style>
@ojhad
Could this be fixed by using TemplateBindings in the setter property values for Height, Width, Margin, and VerticalAlignment (or whichever ones we choose to make customizable)?
@joshkoon
Are there any other properties that you were specifically interested in changing?
anawishnoff
on 26 Mar 2020
@anawishnoff I'm currently looking at issue https://github.com/microsoft/microsoft-ui-xaml/issues/1961 locally here and would give it a shot if theme resources should be introduced. As such, I would also add a theme resource for the NavigationViewItemSeparator specifying its margin. The rest of the NavigationViewItemSeparator properties I would leave untouched in that PR. Any thoughts?
I.e. the margin resources could look like this:
<Thickness x:Key="TopNavigationViewItemSeparatorMargin">10,0</Thickness>
<Thickness x:Key="NavigationViewItemSeparatorMargin">16,10</Thickness>
Felix-Dev
on 26 Mar 2020
@Felix-Dev - how would this ThemeResource be used? Would it serve as the default for the NavigationViewItemSeparator Margin property as defined above in this snippet?
<Rectangle
x:Name="SeparatorLine"
Height="1"
Margin="16,10"
Fill="{ThemeResource SystemControlForegroundBaseLowBrush}" />
Looking more deeply into #1961 and your comment, I think adding the ThemeResources is a good idea. Just want to make sure it won't conflict with anything that we implement to solve this issue/clarify what issue the ThemeResources would be solving.
anawishnoff
on 26 Mar 2020
Yes, these theme resources would act as the default margins for the NavigationViewItemSeparator, customizable by the developer/designer. As such, the updated style would look like this:
<Style TargetType="local:NavigationViewItemSeparator">
<Setter Property="IsEnabled" Value="False" />
<Setter Property="IsTabStop" Value="False" />
<Setter Property="MinHeight" Value="0" />
<Setter Property="AutomationProperties.AccessibilityView" Value="Raw" />
<Setter Property="Template">
<Setter.Value>
<ControlTemplate>
<Grid x:Name="NavigationViewItemSeparatorRootGrid">
<VisualStateManager.VisualStateGroups>
<VisualStateGroup x:Name="NavigationSeparatorLineStates">
<VisualState x:Name="HorizontalLine" />
<VisualState x:Name="VerticalLine">
<VisualState.Setters>
<Setter Target="SeparatorLine.Height" Value="20" />
<Setter Target="SeparatorLine.Width" Value="1" />
<Setter Target="SeparatorLine.Margin" Value="{ThemeResource TopNavigationViewItemSeparatorMargin}" />
<Setter Target="SeparatorLine.VerticalAlignment" Value="Center" />
</VisualState.Setters>
</VisualState>
</VisualStateGroup>
</VisualStateManager.VisualStateGroups>
<Rectangle
x:Name="SeparatorLine"
Height="1"
Margin="{ThemeResource NavigationViewItemSeparatorMargin}"
Fill="{ThemeResource SystemControlForegroundBaseLowBrush}" />
</Grid>
</ControlTemplate>
</Setter.Value>
</Setter>
</Style>
The theme resources would be defined in NavigationView_rs1_themeresources.xaml presumably.
NavigationViewItem/NavigationViewItemSeparator margins are probably different for each PaneDisplayMode of the NavigationView (left/top) which is why theme resources for each specific navigation view display mode gives app designers much more flexibility than just relying on TemplateBindings here I believe.
Felix-Dev
on 26 Mar 2020
@Felix-Dev Thanks for clarifying! Make sense now. So with these ThemeResources in place, is the idea that you could edit them directly rather than having to edit the control template/use a setter property to edit the styling on a NavigationViewItemSeparator?
That would conflict a little bit with this issue, in my opinion. I think we'd want to make sure that any customizable property of NavigationViewItemSeparator can be customized in the same manner. If only margin had a ThemeResource defined, wouldn't that throw a wrench in things?
anawishnoff
on 26 Mar 2020
So with these ThemeResources in place, is the idea that you could edit them directly rather than having to edit the control template/use a setter property to edit the styling on a NavigationViewItemSeparator?
Absolutely! Here's an example how the to-be-introduced TopNavigationViewItemSeparatorMargin theme resource could be used:
<muxc:NavigationView PaneDisplayMode="Top">
<muxc:NavigationView.Resources>
<Thickness x:Key="TopNavigationViewItemSeparatorMargin">2,0,2,0</Thickness>
</muxc:NavigationView.Resources>
</muxc:NavigationView>
That would conflict a little bit with this issue, in my opinion. I think we'd want to make sure that any customizable property of NavigationViewItemSeparator can be customized in the same manner. If only margin had a ThemeResource defined, wouldn't that throw a wrench in things?
In case I would get the go-ahead to try working on improving lightweight styling for the NavigationView, I would gladly introduce other theme resources excluded from my initial thought which was specifically focused on making the margins of NavigationView pane items easily customizable (as asked for in issue #1961). This issue here could warrant adding additional resources outside of those specific margin resources.
For one, I definitely think we should add a NavigationViewItemSeparatorForeground theme resource. This would match today's lightweight styling support for the NavigationViewItem's foreground (i.e. using TopNavigationViewItemForeground) so we should also have such a resource for the separator:
<StaticResource x:Key="NavigationViewItemSeparatorForeground" ResourceKey="SystemControlForegroundBaseLowBrush" />
<StaticResource x:Key="TopNavigationViewItemSeparatorForeground" ResourceKey="SystemControlForegroundBaseLowBrush" />
As for other NavigationViewItemSeparator properties...should we also make Width (in top mode) or Height in left mode for the separator stylable via lightweight theme resources (@joshkoon to comment)? Is there demand to customize these values too?
Thinking further, if we are going to introduce *NavigationViewItemMargin theme resources, we perhaps should think about adding a NavigationViewItemPadding theme resource too, for example. Otherwise we might end up in a situation you are hinting at it...with the developer/designer left scratching their head because one resource is available and some other - perhaps closely related - resource is missing.
Given this, we might need to open a bigger work item combining at least this issue and issue #1961 together with additions if necessary.
Felix-Dev
on 26 Mar 2020
Ok, I made some quick tests with the width and height of a NavigationViewItemSeparator (width in display mode top, height in left mode) and the results seem presentable enough:
(That's a height of 2px for the separator, which originally has a height of 1px). And what's a nice looking UI is subject to differ for any two folks anyway. The point of lightweight styling is that the UI can easily be modified without having to create a new Style (let alone a hard-copy of a Style!). As such, I now think we should also add Width and Height controlling theme resources for the NavigationViewItem separator:
<x:Double x:Key="NavigationViewItemSeparatorHeight">1</x:Double>
<x:Double x:Key="TopNavigationViewItemSeparatorWidth">1</x:Double>
Overall, I think this is a great approach.
As for other NavigationViewItemSeparator properties...should we also make Width (in top mode) or Height in left mode for the separator stylable via lightweight theme resources (@joshkoon to comment)? Is there demand to customize these values too?
I wonder if specifically for height and width, TemplateBinding to the properties would be more intuitive than using lightweight styling. But I'd love for @kikisaints, the mistress of all things light weight styling, to weigh in.
YuliKl
on 27 Mar 2020
Side comment - I'm seeing something really strange happening with the padding of separators in the latest version of the Controls Gallery.
Maybe new padding theme resources will fix this bug 馃檪
YuliKl
on 27 Mar 2020
@YuliKl I was thinking that if we already introduce lightweight styling theme resources like Foreground and Margin for the separator, we could also just add those two Width and Height theme resources as well. That way, the developer would not have to create a new style just because they wanted to change the "thickness" of multiple/all the separators used in a NavigationView. Especially if all the other properties of the separator they might want to change are available as theme resources.
Another benefit of having two specific thickness-controlling theme resources of the NavigationViewSeparator (one for Left mode, the other for Top mode) is that the developer only needs to specify them once and then can freely change the NavigationView's PaneDisplayMode back and forth without having to update the NavigationViewItemSeparators used to match them with the new display mode.
I guess we could achieve the same with TemplateBinding only by using the Width property in Top mode (so Separator height would be fixed) and use the Height property in Left mode. That would potentially introduce some confusion though...you set the width and height on a separator but only one value gets respected depending on the pane display mode (Height in Left mode, Width in Top mode).
To avoid this confusion, width and height could be applied to the separator in every display mode but this a) would make the Height value in Top mode variable (currently fixed at 20) and more importantly b) makes a NavigationViewItemSeparator specific to a PaneDisplayMode. If the developer now changes the display mode, they now have to to update Width and Height of their separators too.
As such, I believe having two theme resources which cleanly define the thickness of a separator for each pane display mode has advantages over the TemplateBinding approach.
Felix-Dev
on 27 Mar 2020
@Felix-Dev I'm excited about this approach and agree with your point above regarding the Width and Height ThemeResources - best those be included as well to avoid confusion.
@joshkoon: Just an FYI - I'm going to update the proposal to include this solution!
@Felix-Dev: Is this something you were looking to tackle in a PR? 馃槉 Let me know if you have comments or concerns with the updated proposal once it's complete.
anawishnoff
on 30 Mar 2020
@anawishnoff Yes, I would like to address this issue in a PR 馃檪 Let me know when you think we are ready to start working it!
Felix-Dev
on 30 Mar 2020
@Felix-Dev Awesome! As we aren't necessarily changing an API, I don't think this requires a full spec. @ranjeshj correct me if I'm wrong? If I'm correct, a PR can be started whenever you have capacity.
anawishnoff
on 30 Mar 2020
@anawishnoff Working on the PR, I have made two additional observations for now:
Observation 1:
Introducing the theme resource
<Thickness x:Key="NavigationViewItemSeparatorMargin">16,10</Thickness>
can cause issues for a navigation view in left compactmode when this resource is set to a margin using larger values for the left and right margin. Take for example the setting
<Thickness x:Key="NavigationViewItemSeparatorMargin">32,10</Thickness>
which causes the separator to not be visible any longer in left compact mode:
With the original values 16,10 the separator is clearly visible (in red):
As such, I propose the following solution here:
Currently, the NavigationViewItemSeparator has only two visual states: HorizontalLine and VerticalLine. We could introduce a third visual state named HorizontalLineCompact which would set a specific margin value to the NavigationViewItemSeparator:
<Thickness x:Key="NavigationViewCompactItemSeparatorMargin">16,10</Thickness>
As such, the updated style would now look like this (excluding the irrelevant parts):
<Style TargetType="local:NavigationViewItemSeparator">
<!-- ... -->
<Setter Property="Template">
<Setter.Value>
<ControlTemplate>
<Grid x:Name="NavigationViewItemSeparatorRootGrid">
<VisualStateManager.VisualStateGroups>
<VisualStateGroup x:Name="NavigationSeparatorLineStates">
<VisualState x:Name="HorizontalLine" />
<VisualState x:Name="HorizontalLineCompact">
<VisualState.Setters>
<Setter Target="SeparatorLine.Margin" Value="{ThemeResource NavigationViewCompactItemSeparatorMargin}" />
</VisualState.Setters>
</VisualState>
<VisualState x:Name="VerticalLine">
<VisualState.Setters>
<!-- ... -->
<Setter Target="SeparatorLine.Margin" Value="{ThemeResource TopNavigationViewItemSeparatorMargin}" />
</VisualState.Setters>
</VisualState>
</VisualStateGroup>
</VisualStateManager.VisualStateGroups>
<Rectangle
x:Name="SeparatorLine"
Margin="{ThemeResource NavigationViewItemSeparatorMargin}"/>
</Grid>
</ControlTemplate>
</Setter.Value>
</Setter>
</Style>
Observation 2:
We can set the foreground of a NavigationViewItem by setting its Foreground property. This currently does not work for a NavigationViewItemSeparator (settings its Foreground property has no effect as we are not using TemplateBinding here). Should we thus "enable" the Foreground property on it too?
If yes, we probably need to think some more about the potential implementation because if we use TemplateBinding in the current style, the user-provided foreground color would be overriden as soon as the separator is used in navigation view display mode "top" (because we are overriding the foreground color in a visual state setter with the specific TopNavigationViewItemSeparatorForeground resource). As such, supporting setting the Foreground property and having it work for all navigation view display modes might not actually be straight-forward.
Thoughts?
Felix-Dev
on 31 Mar 2020
@Felix-Dev Just to clarify about your first point: If we made a NavigationViewCompactItemSeparatorMargin resource, one would have to define their desired item separator styling for non-compact mode with NavigationViewItemSeparatorMargin and separately define their desired item separator styling for compact mode with NavigationViewCompactItemSeparatorMargin?
I'm trying to weigh the pros and cons of either having the separator's margin be un-editable in left compact mode, or adding another resource that could make the styling process less intuitive, i.e. "why isn't the margin changing in non-compact mode but not in compact mode?"
Do we understand why the separator is disappearing in compact mode? I was under the impression that the margin values defined were for top and bottom, which shouldn't be pushing the separator out of the frame.
anawishnoff
on 31 Mar 2020
@anawishnoff Yes, my proposed solution would mean we would now have two theme resources to set the margin of the separator in left mode. One resource to be applied when the navigation view pane is expanded and one resource to be used when the pane is closed (compact mode). This additional theme resource in compact mode is required because of the following reason:
Each theme resource is a Thickness value which enables to set the margin to the left, top, right and bottom. In other words, a resource like
<Thickness x:Key="NavigationViewItemSeparatorMargin">16,10</Thickness>
could also be written equivalently as
<Thickness x:Key="NavigationViewItemSeparatorMargin">16,10,16,10</Thickness>
with Left = Right =16 & Top = Bottom = 10
In top mode, we only need one theme resource because there is no compact mode. In left mode, however, if we decide to add a theme resource for the margin, the user might change our default 16,10 margin to a different margin, like
<Thickness x:Key="NavigationViewItemSeparatorMargin">32,10,32,10</Thickness>
This causes the separator to not show up any longer in the left compact mode of the Navigation view (with the navigation pane in a closed state), because there is no space left for it any longer. The first 32 pixel from the left border to the center and the first 32 pixel from the right border to the center are now "margin pixels", that is 64 pixel of the navigation view compact mode (on the horizontal axis) are no longer available to our actual separator line. And since the navigation view column doesn't have more than 64 pixel in its compact state with the pane closed, there are no pixels left for the actual separator. To aid in understanding this, please look at the following resource values and their corresponding visual representation:
<Thickness x:Key="NavigationViewItemSeparatorMargin">16,10,0,10</Thickness>
Now we are pushing it further to the right by increasing its left margin:
<Thickness x:Key="NavigationViewItemSeparatorMargin">32,10,0,10</Thickness>
and finally we are also adding some margin to the right now:
<Thickness x:Key="NavigationViewItemSeparatorMargin">32,10,32,10</Thickness>
The whole images were probably overkill right now as I'm sure you got the problem the moment you read the margins are applied to to not only top and bottom, but also left and right 馃槄
That said, this should illustrate the problem why we probably need two margin resources for the left mode of the navigation view if we consider users could also change the left and right margin of the separator and not just the top and bottom margins. Left mode has both an expanded mode (pane opened) and a compact mode (pane closed) and the two margin resources for the separator would reflect that.
Of course, we could decide, as you said, to not allow separator margins to be changeable if in closed compact mode. In that case, I would still have the third internal visual state, but this time, there would be no additional theme resource NavigationViewCompactItemSeparatorMargin. I am wondering though...suppose the user did provide their own values for the NavigationViewItemSeparatorMargin theme resource (so they modified the separator margin in left mode) this could introduce unwelcome "visual jumps" for the separator when switching from compact mode to expanded mode and vice-versa (as now different margins for top and bottom could be used, causing the separator to visually move up or down).
Felix-Dev
on 31 Mar 2020
@Felix-Dev Thanks so much for the detailed explanation! Definitely makes sense, and I agree with your point about having weird visual jumps if we allowed editing in expanded mode but not compact mode.
I think adding another resource for NavigationViewCompactItemSeparatorMargin is a good and necessary approach. I'll update the proposal to include this.
As for the Foreground issue: I don't think this PR should be blocked on needing to implement foreground resources for NavigationViewItemSeparator. Changing the foreground color specifically was not requested, and if it's going to require non-trivial API changes it's best that it becomes its own issue, in my opinion. I'd go ahead and focus on the other resources if you agree.
anawishnoff
on 31 Mar 2020
As for the Foreground issue: I don't think this PR should be blocked on needing to implement foreground resources for NavigationViewItemSeparator.
That's good to know :) And not "TemplateBinding" the Foreground property doesn't mean the foreground won't be customizable! The PR would still add theme resources the developer/designer can use to set a custom foreground color for the separator in either display mode, if so required. Only the Foreground property approach can't be used but we can address this in a future commit if there is user demand adding this support too, just as you suggested.
Felix-Dev
on 31 Mar 2020
@Felix-Dev Thanks again for clarifying - sounds good!
anawishnoff
on 1 Apr 2020
Related issues
YuliKl
路
3Comments
adrientetar
路
3Comments
ocalvo
路
3Comments
MartinZikmund
路
3Comments
MuziburRahman
路
3Comments