Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Aug 17, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2b1145d
Add NavigationViewItemExpandedGlyphFontSize theme resource.
Felix-Dev Aug 6, 2020
c5cb50a
Applied added TopNavigationViewItemContentOnlyContentPresenterMargin …
Felix-Dev Aug 6, 2020
d145698
Updated existing TopNavigationViewOverflow button styles to match the…
Felix-Dev Aug 6, 2020
b5ed60e
Add tooltip to the Overflow button.
Felix-Dev Aug 6, 2020
f467596
Add API test for Overflow button tooltip.
Felix-Dev Aug 6, 2020
d9a7789
Merge branch 'master' into user/Felix-Dev/navview-itemchevron-and-mor…
Felix-Dev Aug 6, 2020
787c0b2
Added comment and expand/collapse chevron UI fix when in flyout.
Felix-Dev Aug 6, 2020
a852c6d
Fix automation name for the overflow button.
Felix-Dev Aug 7, 2020
f8da599
Remove code setting the content of the overflow button to "More" on A…
Felix-Dev Aug 7, 2020
4471ccd
Revert commit f8da599878f73c5609831ef2015333e16fe66565.
Felix-Dev Aug 11, 2020
c8b7a95
Changed overflow button tooltip and UIA name to "More".
Felix-Dev Aug 11, 2020
f5b27c1
Merge branch 'master' into user/Felix-Dev/navview-itemchevron-and-mor…
Felix-Dev Aug 11, 2020
0909d87
Increased width of the overflow button to 48px and added border/backg…
Felix-Dev Aug 11, 2020
a56b2c0
Fix failing test.
Felix-Dev Aug 12, 2020
bfcb391
Updated visual verification files.
Felix-Dev Aug 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions dev/NavigationView/NavigationView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

#include "pch.h"
Expand Down Expand Up @@ -469,11 +469,19 @@ void NavigationView::OnApplyTemplate()
if (auto topNavOverflowButton = GetTemplateChildT<winrt::Button>(c_topNavOverflowButton, controlProtected))
{
m_topNavOverflowButton.set(topNavOverflowButton);
winrt::AutomationProperties::SetName(topNavOverflowButton, ResourceAccessor::GetLocalizedStringResource(SR_NavigationOverflowButtonText));
winrt::AutomationProperties::SetName(topNavOverflowButton, ResourceAccessor::GetLocalizedStringResource(SR_NavigationOverflowButtonName));
topNavOverflowButton.Content(box_value(ResourceAccessor::GetLocalizedStringResource(SR_NavigationOverflowButtonText)));
auto visual = winrt::ElementCompositionPreview::GetElementVisual(topNavOverflowButton);
CreateAndAttachHeaderAnimation(visual);

auto const toolTip = winrt::ToolTipService::GetToolTip(topNavOverflowButton);
if (!toolTip)
{
auto const tooltip = winrt::ToolTip();
tooltip.Content(box_value(ResourceAccessor::GetLocalizedStringResource(SR_NavigationOverflowButtonToolTip)));
winrt::ToolTipService::SetToolTip(topNavOverflowButton, tooltip);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (auto const flyoutBase = topNavOverflowButton.Flyout())
{
if (winrt::IFlyoutBase6 topNavOverflowButtonAsFlyoutBase6 = flyoutBase)
Expand Down
19 changes: 18 additions & 1 deletion dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,6 @@ public void VerifyAutomationPeerExpandCollapsePatternBehavior()
});
}


[TestMethod]
public void VerifySettingsItemToolTip()
{
Expand Down Expand Up @@ -854,5 +853,23 @@ public void VerifyExpandCollapseChevronVisibility()
});
}

[TestMethod]
public void VerifyOverflowButtonToolTip()
{
RunOnUIThread.Execute(() =>
{
var navView = new NavigationView();
navView.PaneDisplayMode = NavigationViewPaneDisplayMode.Top;

Content = navView;
Content.UpdateLayout();

var overflowButton = VisualTreeUtils.FindVisualChildByName(navView, "TopNavOverflowButton") as Button;
var toolTipObject = ToolTipService.GetToolTip(overflowButton);

bool testCondition = toolTipObject is ToolTip toolTip && toolTip.Content.Equals("More");
Verify.IsTrue(testCondition, "ToolTip text should have been \"More\".");
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Felix-Dev Felix-Dev Aug 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Felix-Dev Felix-Dev Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

image

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
230 changes: 129 additions & 101 deletions dev/NavigationView/NavigationView_rs1_themeresources.xaml
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 5 additions & 1 deletion dev/NavigationView/Strings/en-us/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,11 +150,15 @@
<comment>Automation name for the nav view provided close button</comment>
</data>
<data name="NavigationOverflowButtonName" xml:space="preserve">
<value>More Button</value>
<value>More</value>
<comment>Automation name for the nav view more button when panel is on top</comment>
</data>
<data name="NavigationOverflowButtonText" xml:space="preserve">
Felix-Dev marked this conversation as resolved.
Show resolved Hide resolved
<value>More</value>
<comment>Text for the nav view more button when panel is on top</comment>
</data>
<data name="NavigationOverflowButtonToolTip" xml:space="preserve">
<value>More</value>
<comment>ToolTip caption for the nav view more button when panel is on top</comment>
Felix-Dev marked this conversation as resolved.
Show resolved Hide resolved
</data>
</root>
1 change: 1 addition & 0 deletions dev/ResourceHelper/ResourceAccessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class ResourceAccessor sealed
#define SR_NavigationCloseButtonName L"NavigationCloseButtonName"
#define SR_NavigationOverflowButtonName L"NavigationOverflowButtonName"
#define SR_NavigationOverflowButtonText L"NavigationOverflowButtonText"
#define SR_NavigationOverflowButtonToolTip L"NavigationOverflowButtonToolTip"
#define SR_SettingsButtonName L"SettingsButtonName"
#define SR_NavigationViewSearchButtonName L"NavigationViewSearchButtonName"
#define SR_TextAlphaLabel L"TextAlphaLabel"
Expand Down
Loading