NavView: Expand/Collapse chevron and Overflow button improvements in Top mode #3063

Felix-Dev · 2020-08-06T14:03:51Z

Description

This PR adds a NavigationViewItemExpandedGlyphFontSize resource to allow developers to customize the font size of the expand/collapse chevron.

In addition, it also aligns the expand/collapse chevron in Top mode with the chevron in Left mode by updating its FontSize and increasing its hit target area.

This PR furthermore re-designs the Overflow button in Top mode to match the design proposal given by the design team. Its content has been changed to an ellipsis and the NavigationView.OverflowLabelMode API is no longer relevant for the default control design. The Overflow button now has also received a tooltip matching that of the CommandBar's Overflow button ("See more").

The changes to the Overflow button have been made with maximum backwards compatibility in mind. While the NavigationView.OverflowLabelMode API becomes obsolete with this change, it is being fully retained alongside its internal behavior (i.e. going to its dedicated visual states). The same applies to the existing public Overflow button styles. Both styles have been retained and have been updated to match the new Overflow button design (both styles are now identical). The benefits of this approach are that we are not introducing any style breaking changes as part of this PR. Apps which customized the existing Overflow button styles will still see their custom styles applied - in both the OverflowLabelMode.MoreLabel and OverflowLabelMode.NoLabel configurations.

A couple additional new resources have been added to make customization easier for developers:

TopNavigationViewItemContentOnlyContentPresenterMargin
TopNavigationViewItemIconOnlyExpandChevronMargin
TopNavigationViewItemContentOnlyExpandChevronMargin

Motivation and Context

Closes #2750.

How Has This Been Tested?

Tested visually and added API test for the Overflow button tooltip.

Screenshots:

Left navigation	Top navigation

(Table showing the expand/collapse glyph with <x:Double x:Key="NavigationViewItemExpandedGlyphFontSize">20.0</x:Double>)

(The updated 40px hit target area of the expand/collapse chevron inside a NavigationView item).

(Updated hit target area and chevron font size (12px) without highlighted hit target area background)

(The updated Overflow button with tooltip. (The corners of the tooltip are square because I am using the InnerLoop solution here. They will be rounded as expected in the full WinUI product.))

Open Questions

I have opened Implementation-specific questions in a review of the code. Remaining general open questions will be posted in issue #2750.

Also increased hit target area and FontSize of expand/collapse chevron in top mode to match Left mode values.

…resource in previous commit 2b1145d.

… proposed design.

…ebutton-improvements

dev/NavigationView/Strings/en-us/Resources.resw

dev/NavigationView/NavigationView_rs1_themeresources.xaml

StephenLPeters · 2020-08-06T20:41:01Z

dev/NavigationView/NavigationView_rs1_themeresources.xaml

-                                IsHitTestVisible="False" />
-                        </StackPanel>
+                        <Rectangle 
+                            x:Name="PointerRectangle" 


PointerRectangle [](start = 36, length = 16)

Maybe name this HitTestRectangle?

I'm up for the rename but would this be an issue for folks who are grabbing this element, for whatever reason, in their app code and depend on it? Would probably be a very rare situation....

Ah, I thought this was being newly introduced here.. Where did it live before this?

@StephenLPeters Sorry, it is getting late here...

The PointerRectangle line comes from the existing NavigationViewOverflowButtonNoLabelStyleWhenPaneOnTop style directly below this style (NavigationViewOverflowButtonStyleWhenPaneOnTop). I just copied that style into this one so they now match.

StephenLPeters · 2020-08-06T20:43:28Z

dev/NavigationView/NavigationView_rs1_themeresources.xaml

@@ -1095,16 +1092,18 @@
                                <VisualState x:Name="IconOnly">
                                    <VisualState.Setters>
                                        <Setter Target="PointerRectangle.Visibility" Value="Visible" />
-                                        <Setter Target="LayoutRoot.Width" Value="48" />
+                                        <Setter Target="LayoutRoot.MinWidth" Value="48" />
                                        <Setter Target="ContentPresenter.Visibility" Value="Collapsed" />
                                        <Setter Target="SelectionIndicatorGrid.Margin" Value="4,0,4,4" />


4,0,4,4 [](start = 94, length = 7)

I see what you mean about the margin resource now.. I think this could make sense..

Yep, I am tracking those in #2877 but I can also add the Margin specific resources as part of this PR (while I'm at it).

StephenLPeters · 2020-08-06T20:45:03Z

dev/NavigationView/NavigationView.cpp

+            auto const tooltip = winrt::ToolTip();
+            tooltip.Content(box_value(ResourceAccessor::GetLocalizedStringResource(SR_NavigationOverflowButtonToolTip)));
+            winrt::ToolTipService::SetToolTip(topNavOverflowButton, tooltip);
+        }


Do we need to do this from code behind or can we add this to the template?

I have generally seen tooltips in WinUI only being added in code-behind (TabView and existing NavView tooltips, for example) so I am presuming this is the recommended way here.

StephenLPeters · 2020-08-06T20:46:19Z

dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs

+                bool testCondition = toolTipObject is ToolTip toolTip && toolTip.Content.Equals("See more");
+                Verify.IsTrue(testCondition, "ToolTip text should have been \"See more\".");
+            });
+        }


I think testing that a custom tooltip isn't overwritten would be good too.

Does Content.UpdateLayout() cause NavigationView.ApplyTemplate() to run or do I need to use a new test here?

@ranjeshj do you know off the top of your head?

ApplyTemplate will be run before running the first layout pass. If you are asking if it will be run again on the next layout (UpdateLayout call), the answer is no.

There might be some trouble testing the custom tooltip not being overriden:

Developers today would modify the existing NavigationViewOverflowButtonStyleWhenPaneOnTop style to set a custom tooltip roughly like this:

<Style x:Key="NavigationViewOverflowButtonStyleWhenPaneOnTop" TargetType="Button"> <Setter Property="ToolTipService.ToolTip" Value="Custom tooltip" /> </Style>

So in order to test that "Custom tooltip" is not being overriden, my thinking was to create a test NavigationView in code-behind containing that style and then check the tooltip content in an interaction test. However, I noticed that setting this style in the NavigationView.Resources scope, or even the Page level scope, does not apply the custom style at all:

<muxc:NavigationView PaneDisplayMode="Top"> <muxc:NavigationView.Resources> <Style x:Key="NavigationViewOverflowButtonStyleWhenPaneOnTop" TargetType="Button"> <Setter Property="Background" Value="Orange" /> <Setter Property="Foreground" Value="Green" /> <Setter Property="ToolTipService.ToolTip" Value="Custom tooltip" /> </Style> </muxc:NavigationView.Resources> </muxc:NavigationView>

If I define that style on the app level it is being applied:

I would expect here that my custom style defined on the page or control level would be picked up as well (just as how I can override theme resources on the page/control level) as the NavigationView overflow button in Top mode is neither in a Popup nor flyout.

This immediately poses the problem that if I set that style on the app level, then my existing API test to check the default overflow button tooltip will fail as the tooltip content will now be my custom set tooltip.

Thoughts on how to proceed here?

This seems like a bug, you should be able to style this from any parent. If this is blocking the custom tooltip test I think its okay to leave that space uncovered until this new bug is resolved.

dev/NavigationView/NavigationView_rs1_themeresources.xaml

…pplyTemplate(). Also removed the entry for the "More" string in the ResourceHelper logic and in the default NavigationView resource file.

dev/NavigationView/Strings/en-us/Resources.resw

StephenLPeters · 2020-08-07T22:45:49Z

/azp run

azure-pipelines · 2020-08-07T22:46:01Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-08-10T18:01:59Z

@Felix-Dev looks like these tests need updating

StephenLPeters · 2020-08-10T18:04:12Z

@Felix-Dev I also just checked in the VisualVerification scripts update #3039 if you could please pull in master and then I can rerun the pipeline so you can update the verification failure

dev/NavigationView/Strings/en-us/Resources.resw

…ebutton-improvements

Felix-Dev · 2020-08-11T20:57:27Z

@StephenLPeters Merged with master.

@YuliKl We might just be done here except for whether we should change the width and background/border brushes of the overflow button here to match that of the CommandBar. More info here.

YuliKl · 2020-08-11T21:02:55Z

@YuliKl We might just be done here except for whether we should change the width and background/border brushes of the overflow button here to match that of the CommandBar. More info here.

Definitely yes to changing the width to 48px. I'm less confident about changing the brushes. If the fix is straight-forward, let's match CommandBar. If the fix is complex, that may not be a good place to apply a lot of effort.

Felix-Dev · 2020-08-11T21:53:23Z

@YuliKl Here is how the overflow button could look like with width increased to 48px and border/background brushes matching the NavigationViewItems:

Adding reveal border and background wasn't complex at all :) Let me know what you think.

…round reveal to it.

StephenLPeters · 2020-08-11T22:12:46Z

/azp run

azure-pipelines · 2020-08-11T22:12:57Z

Azure Pipelines successfully started running 1 pipeline(s).

Felix-Dev · 2020-08-11T22:27:50Z

dev/NavigationView/NavigationView_rs1_themeresources.xaml

@@ -402,8 +404,10 @@
    <Style x:Key="NavigationViewOverflowButtonStyleWhenPaneOnTop" TargetType="Button">
        <Setter Property="Background" Value="{ThemeResource NavigationViewItemBackground}" />
        <Setter Property="Foreground" Value="{ThemeResource TopNavigationViewItemForeground}" />
-        <Setter Property="Height" Value="{StaticResource PaneToggleButtonHeight}" />
-        <Setter Property="Width" Value="{StaticResource PaneToggleButtonWidth}" />


@StephenLPeters @YuliKl Please take a look at this and say if you are fine with this change or not.

The width of the overflow button was set to 40px by the PaneToggleButtonWidth resource. As we decided to increase its width to 48px, changing that resource value would also lead to an increased width for the PaneToggle button which also consumes that resource.

As such, I added a new dedicated resource here to control the width of the overflow button so that the PaneToggle button width will remain unchanged. While this will break customization for developers who used that resource to style the width of the overflow button, I think this is such a minor case here that we should be fine and that the benefits outweigh the possible small inconvenience for developers (if any are caused at all). I also don't know how we would be able to increase the width of that overflow button while keeping the width of the PaneToggle button untouched and not causing a potential customization break....

Once I introduced a dedicated TopNavigationViewOverflowButtonWidth I also created its matching counterpart for the height - TopNavigationViewOverflowButtonHeight.

Felix-Dev · 2020-08-12T15:57:52Z

@StephenLPeters Updated some visual verification files. (I left out some of included updated files in the drop archive for now - such as AppBarToggleButton-7.xml - as I don't really see right now why they are included. After all, I didn't touch the AppBarToggleButton area here, for example, nor does the default style of that button use parts of the NavigationView from what I can tell.)

The visual verification file task determing which files to ship with the drop archive still doesn't seem to work as intended in some cases. For example, I got a whole bunch of

NavigationView*-4.xml
NavigationView*-5.xml
NavigationView*-6.xml

files as part of the drop archive, yet those -4, -5 and -6 files were all identical ( so NavigationViewAuto-4.xml is identical to NavigationViewAuto-5.xml which is identical to NavigationViewAuto-6.xml). In that case, those identical -5 and -6 files shouldn't be included.

StephenLPeters · 2020-08-12T17:05:56Z

@StephenLPeters Updated some visual verification files. (I left out some of included updated files in the drop archive for now - such as AppBarToggleButton-7.xml - as I don't really see right now why they are included. After all, I didn't touch the AppBarToggleButton area here, for example, nor does the default style of that button use parts of the NavigationView from what I can tell.)

The visual verification file task determing which files to ship with the drop archive still doesn't seem to work as intended in some cases. For example, I got a whole bunch of
NavigationView*-4.xml
NavigationView*-5.xml
NavigationView*-6.xml
files as part of the drop archive, yet those -4, -5 and -6 files were all identical ( so NavigationViewAuto-4.xml is identical to NavigationViewAuto-5.xml which is identical to NavigationViewAuto-6.xml). In that case, those identical -5 and -6 files shouldn't be included.

@Felix-Dev did you run the GenerateVisualVerificationFiles.ps1 script on the archive drop? instructions were updated here: https://github.com/microsoft/microsoft-ui-xaml/blob/master/docs/developer_guide.md in the Diff & replace section. Also, looking at the Drop, I don't see any Non-NavView visual verification updates

Felix-Dev · 2020-08-12T17:13:04Z

@StephenLPeters Ah, no, I haven't run the script yet, thanks for pointing it out.

The drop archive contains a single non NavView specific file for me: AppBarToggleButton-7.xml. Weird that it does not show up for you 🤔

Should I revert my previous commit and let the script do its work or would the last commit be fine in case this test run will succeed?

StephenLPeters · 2020-08-12T17:14:41Z

Hmm, that is very strange, I just double checked here: https://dev.azure.com/ms/microsoft-ui-xaml/_build/results?buildId=100200&view=results and the drop doesn't contain that file

Felix-Dev · 2020-08-12T20:25:12Z

@StephenLPeters Did you cancel the azure test run here? I was sure you started one some hours ago. If so, what's the status here? Should I revert my commit and run the script instead?

StephenLPeters · 2020-08-12T20:52:46Z

I never started a lab build. I would prefer we use the script going forward, if you don't mind trying it out here that would be cool.

Felix-Dev · 2020-08-12T21:12:26Z

@StephenLPeters Sure, I will revert the commit and test the script here. And about that lab build I thought you started but actually never did...guess I blame that illusion on the hot weather 🥵

Felix-Dev · 2020-08-13T19:13:07Z

@StephenLPeters The script output is pretty much the same as the files I uploaded in the recent commit - except that it also lists the non numbered files, so for example:

NavigationViewAuto.xml
NavigationViewAuto-4.xml
NavigationViewAuto-7.xml
NavigationViewAuto-8.xml

As I did before, I double-checked if these non numbered files (i.e. NavigationViewAuto.xml) have actually been updated compared to the existing files - and they are exactly the same, As such, I think the script needs to be slighlty updated to exclude those files.

Seeing as the script output otherwise matches my recent commit, I think we can start a test run here.

StephenLPeters · 2020-08-14T22:56:39Z

/azp run

azure-pipelines · 2020-08-14T22:56:58Z

Azure Pipelines successfully started running 1 pipeline(s).

…Top mode (microsoft/microsoft-ui-xaml#3063)

ghost · 2020-09-24T19:09:38Z

🎉Microsoft.UI.Xaml v2.5.0-prerelease.200923002 has been released which incorporates this pull request.:tada:

Handy links:

Felix-Dev added 7 commits August 6, 2020 10:58

Add NavigationViewItemExpandedGlyphFontSize theme resource.

2b1145d

Also increased hit target area and FontSize of expand/collapse chevron in top mode to match Left mode values.

Applied added TopNavigationViewItemContentOnlyContentPresenterMargin …

c5cb50a

…resource in previous commit 2b1145d.

Updated existing TopNavigationViewOverflow button styles to match the…

d145698

… proposed design.

Add tooltip to the Overflow button.

b5ed60e

Add API test for Overflow button tooltip.

f467596

Merge branch 'master' into user/Felix-Dev/navview-itemchevron-and-mor…

d9a7789

…ebutton-improvements

Added comment and expand/collapse chevron UI fix when in flyout.

787c0b2

msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Aug 6, 2020

Felix-Dev commented Aug 6, 2020

View reviewed changes

dev/NavigationView/Strings/en-us/Resources.resw Show resolved Hide resolved

Felix-Dev commented Aug 6, 2020

View reviewed changes

dev/NavigationView/NavigationView_rs1_themeresources.xaml Show resolved Hide resolved

StephenLPeters reviewed Aug 6, 2020

View reviewed changes

StephenLPeters added area-NavigationView NavView control area-Styling team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Aug 6, 2020

Felix-Dev commented Aug 6, 2020

View reviewed changes

dev/NavigationView/NavigationView_rs1_themeresources.xaml Show resolved Hide resolved

Felix-Dev added 2 commits August 7, 2020 16:33

Fix automation name for the overflow button.

a852c6d

Remove code setting the content of the overflow button to "More" on A…

f8da599

…pplyTemplate(). Also removed the entry for the "More" string in the ResourceHelper logic and in the default NavigationView resource file.

Felix-Dev commented Aug 7, 2020

View reviewed changes

dev/NavigationView/Strings/en-us/Resources.resw Show resolved Hide resolved

Felix-Dev mentioned this pull request Aug 8, 2020

NavView: Fix CornerRadius for overflow menu and NavViewItem children flyout menu #3082

Merged

YuliKl reviewed Aug 11, 2020

View reviewed changes

dev/NavigationView/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved

Felix-Dev added 2 commits August 11, 2020 22:32

Revert commit f8da599.

4471ccd

Changed overflow button tooltip and UIA name to "More".

c8b7a95

Merge branch 'master' into user/Felix-Dev/navview-itemchevron-and-mor…

f5b27c1

…ebutton-improvements

YuliKl approved these changes Aug 11, 2020

View reviewed changes

Increased width of the overflow button to 48px and added border/backg…

0909d87

…round reveal to it.

Felix-Dev commented Aug 11, 2020

View reviewed changes

Felix-Dev added 2 commits August 12, 2020 15:48

Fix failing test.

a56b2c0

Updated visual verification files.

bfcb391

StephenLPeters merged commit 2015013 into microsoft:master Aug 17, 2020

Felix-Dev deleted the user/Felix-Dev/navview-itemchevron-and-morebutton-improvements branch August 18, 2020 08:19

Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Aug 18, 2020

NavView: Expand/Collapse chevron and Overflow button improvements in …

02f76b4

…Top mode (microsoft/microsoft-ui-xaml#3063)

Felix-Dev mentioned this pull request Aug 27, 2020

NavigationView: Cleanup overflow button logic #3204

Closed

ghost mentioned this pull request Sep 24, 2020

Add a new light weight style to navigation view to customize the item chevron font size. #2750

Closed

dependabot bot mentioned this pull request Oct 3, 2020

Bump ModernWpfUI from 0.9.1 to 0.9.2 astenlund/fs2ff#4

Merged

NavView: Expand/Collapse chevron and Overflow button improvements in Top mode #3063

NavView: Expand/Collapse chevron and Overflow button improvements in Top mode #3063

Conversation

Felix-Dev commented Aug 6, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots:

Open Questions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felix-Dev Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felix-Dev Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felix-Dev Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenLPeters commented Aug 7, 2020

azure-pipelines bot commented Aug 7, 2020

StephenLPeters commented Aug 10, 2020

StephenLPeters commented Aug 10, 2020

Felix-Dev commented Aug 11, 2020

YuliKl commented Aug 11, 2020

Felix-Dev commented Aug 11, 2020 • edited Loading

StephenLPeters commented Aug 11, 2020

azure-pipelines bot commented Aug 11, 2020

Choose a reason for hiding this comment

Felix-Dev commented Aug 12, 2020

StephenLPeters commented Aug 12, 2020 • edited Loading

Felix-Dev commented Aug 12, 2020 • edited Loading

StephenLPeters commented Aug 12, 2020

Felix-Dev commented Aug 12, 2020 • edited Loading

StephenLPeters commented Aug 12, 2020

Felix-Dev commented Aug 12, 2020

Felix-Dev commented Aug 13, 2020 • edited Loading

StephenLPeters commented Aug 14, 2020

azure-pipelines bot commented Aug 14, 2020

ghost commented Sep 24, 2020

Felix-Dev commented Aug 6, 2020 •

edited

Loading

Felix-Dev Aug 6, 2020 •

edited

Loading

Felix-Dev Aug 6, 2020 •

edited

Loading

Felix-Dev Aug 7, 2020 •

edited

Loading

Felix-Dev commented Aug 11, 2020 •

edited

Loading

StephenLPeters commented Aug 12, 2020 •

edited

Loading

Felix-Dev commented Aug 12, 2020 •

edited

Loading

Felix-Dev commented Aug 12, 2020 •

edited

Loading

Felix-Dev commented Aug 13, 2020 •

edited

Loading