NavigationView BottomMenuItems implementation #1997

ad1Dima · 2020-02-19T06:15:15Z

API spec for this feature can be found here.

Description

I added FooterMenuItems property to NavigationView. My goal also was not to break current behavior of NavigationView.
So i kept both PaneFooter and IsSettingsVisible propertes. However SettingsItem itself now adds to end of FooterItems list if IsSettingsVisible is true

Only braking change is that SettingsItem now have selection when PaneDisplayMode is set to Top. (however there is #1572)
but you can use SelectsOnInvoked property to avoid this

Motivation and Context

Fix #375

also fixes #1572, #148, #2813, #2814

How Has This Been Tested?

manually on Desktop and Xbox

Checked switching between Top and Left PaneMode
SettingsItem Visibility changing
Dynamically updating of FooterMenuItems
SelectedItem Property and ItemInvoked event

ad1Dima · 2020-02-20T05:45:12Z

I need help with tests and Accessibility in this PR.

ranjeshj · 2020-02-20T13:26:38Z

/azp run

azure-pipelines · 2020-02-20T13:27:04Z

Azure Pipelines successfully started running 1 pipeline(s).

dev/NavigationView/NavigationView.xaml

ranjeshj · 2020-02-20T13:33:38Z

dev/NavigationView/NavigationView.h

@@ -357,6 +372,7 @@ class NavigationView :
    TopNavigationViewDataProvider m_topDataProvider{ this };

    winrt::SelectionModel m_selectionModel{};
+    winrt::SelectionModel m_footerSelectionModel{};


@YuliKl Are items in the footer considered separately for selection compared to the rest ? Should they be in the same selection model ? I don't think you can have two selections at the same time, so having two selection models might not be correct. Would it be simpler to merge the FooterItems and menu items into a single source for selection model ?

Would it be simpler to merge the FooterItems and menu items into a single source for selection model ?

Yes, I believe so. Ideally all items, whether in the main list or in the footer list, should have at most one item selected at any given time. The items are navigation destinations, and it's not really possible to navigate to more than one destination page at a time.

Ok. I'll move code to single selection model. This should make code simpler

Hm. I looked through code again. SelectionModel has Source property. For now this is MenuItemsSource or MenuItems for main menu items.

For FooterMenuItems i had to create collection and dynamically add or remove SettingsItem to the end of it.

So should i create some array of array as Source for SelectionModel or just have two selection models synchronized for keep single selection behavior (just like this code do now)?

@ojhad any ideas. I think a type that takes two collections and exposes itself as one larger collection would be useful here (just exposure, without copying the items from the individual source collections). That way you can feed MenuItems and FooterMenuItems and expose it as a single merged collection. It can be a completely decoupled implementation that can be useful in other scenarios as well. Thoughts ?

Is there a way we are handling a large footer list?

For now, it just grow till it can. And it can take all available space in both left and top mode.
For me, bottomMenuItems is purposed for 2 or 3 items like in Groove or MS Store. And may be it should be app developer responsibility to deal with a lot of items on small screen.

While app developers do need to keep general layout in mind, NavigationView needs to gracefully support scrolling or overflow. On Windows, a user can resize their window at any time, so no one can guarantee that all items will always fit. This is the reason the primary nav list scrolls in left mode and have overflow in top mode. The main and bottom lists may end up visually merged depending on both the number of items and also window size.

For left mode i can try to make single scroll experience for whole pane (with primary nav and footer nav). It will look nice. I did this once.

For top nav i'm not sure what is the best approach. Two overflow menus will look odd

@YuliKl
That's what i did for left menu, is it ok:

But i have no idea what to do with Top menu. For now it can be clipped even without footer items. just if menu footer will be too wide:

Let's not block this PR on getting bottom items to overflow correctly when in Top mode. I think it's fine to create a separate issue to track this limitation and make a note in the docs.

ranjeshj · 2020-02-20T13:44:27Z

dev/NavigationView/NavigationView.xaml

@@ -452,12 +443,15 @@
                                            VerticalContentAlignment="Stretch"
                                            HorizontalContentAlignment="Stretch"
                                            Grid.Row="7" />
-                                        <Grid
+                                        <!-- Left footer menu ItemsRepeater -->
+                                        <local:ItemsRepeater


@YuliKl what should the automation index/size be, is it separate for the footer items or is it part of the main items ?

Right now, the main items are one list and the Settings item is another list with index "1 of 1". I think the right behavior would be to make the bottom items into a list that includes the Settings item. We also want to make sure the keyboard behavior matches this.

So given a list:

main1

main2

bottom1

bottom2

settings

Users should be able to Tab between these two lists.
When keyboard focus is on main2, Narrator should announce it as "2 of 2"; pressing Up arrow moves focus to main1; pressing Down arrow moves focus to bottom1; pressing Tab moves focus to bottom1 (unless an item in the bottom list is selected, in which case focus should go to that item).
When keyboard focus is on bottom1, Narrtor should announce it as "1 of 3"; pressing Up arrow moves focus to main2; pressing Down arrow moves focus to bottom2; pressing Tab moves focus to the first focusable item in NavigationView's Header or Content (same behavior as on Settings item today).
When keyboard focus is on settings, Narrator should announce it as "3 of 3"; pressing Up arrow moves focus to bottom2; pressing Down arrow does nothing (same as today); pressing Tab moves focus to a Header or Content item (same as today).

I think the right behavior would be to make the bottom items into a list that includes the Settings item.

that's just what i did

(unless an item in the bottom list is selected, in which case focus should go to that item)

this is broken in PR

Narrator should announce it as "3 of 3"

Strange, on Bottom1 Narrator announce "1 of 3", but on Settings "1 of 1". I'll check this.

Everything else working as you described.

@YuliKl How should Home and End work when main2 is selected? When bottom2 is selected?

Home and End move focus within a list. We've said that this work will result in two lists, so

if focus is on main2 - Home should move focus to main1; End should do nothing

is focus is on bottom2 - Home should move focus to bottom1; End should move focus to settings

ranjeshj · 2020-02-20T13:47:27Z

@ojhad Can you please review this change. Please keep an eye out for things that are changing in your upcoming hierarchical navigation view updates, some of which are likely to cause merge conflicts.

ad1Dima · 2020-02-21T17:13:36Z

/azp run

azure-pipelines · 2020-02-21T17:13:42Z

Commenter does not have sufficient privileges for PR 1997 in repo microsoft/microsoft-ui-xaml

StephenLPeters · 2020-03-02T22:24:51Z

/azp run

azure-pipelines · 2020-03-02T22:25:12Z

Azure Pipelines successfully started running 1 pipeline(s).

dev/NavigationView/NavigationView.xaml

dev/NavigationView/NavigationView.idl

ojhad · 2020-03-03T18:48:17Z

Please add some tests exercising the new functionality

Edit: Just noticed your comment above regarding tests. Are you having issues running them locally?

ad1Dima · 2020-03-04T04:34:29Z

Edit: Just noticed your comment above regarding tests. Are you having issues running them locally?

I just didn't find any advices or instructions

ranjeshj · 2020-03-04T14:59:14Z

Edit: Just noticed your comment above regarding tests. Are you having issues running them locally?

I just didn't find any advices or instructions

@ad1Dima There is some documentation here, but it does look like that could be improved. For running tests on your dev machine, make the test explorer visible through the menu Test->TestExplorer. Once you build the muxcontrols.sln file, you will see all the test listed in test explorer. you can select and run specific tests from that window.

ad1Dima · 2020-03-04T19:19:21Z

you can select and run specific tests from that window.

Thx this works. How do you generate reference for VerifyVisualTree tests?

StephenLPeters · 2020-03-06T23:18:17Z

you can select and run specific tests from that window.

Thx this works. How do you generate reference for VerifyVisualTree tests?

The originals are checked into the repo, if you need to update them the pipeline produces updated masters and publishes them in the artifacts section, which you can download and replace in your copy of the enlistment.

ad1Dima · 2020-08-01T18:10:09Z

Find out that i didn't work with FooterItemsSource properly. Will fix this also

anawishnoff · 2020-08-03T16:57:04Z

Just filed a bug for the top mode overflow issue - #3029

Implement FooterItemsSource properly

ad1Dima · 2020-08-03T20:36:24Z

@ojhad Addressed comments and implemented FooterItemsSource. Plz check changes

StephenLPeters · 2020-08-03T21:04:13Z

/azp run

azure-pipelines · 2020-08-03T21:04:25Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-08-03T21:29:04Z

dev/NavigationView/NavigationView.cpp

@@ -624,6 +709,80 @@ void NavigationView::UpdateItemsRepeaterItemsSource(const winrt::ItemsRepeater&
    }
 }

+void NavigationView::UpdateFooterRepeaterItemsSource(bool sourceCollectionReseted, bool sourceCollectionChanged)


sourceCollectionReseted [](start = 58, length = 23)

sourceCollectionReset

StephenLPeters · 2020-08-03T21:30:32Z

dev/NavigationView/NavigationView.cpp

@@ -4290,20 +4528,23 @@ template<typename T> T NavigationView::GetContainerForData(const winrt::IInspect
        return nvi;
    }

-    if (auto settingsItem = m_settingsItem.get())
+    // First conduct a basic top level search in mai menu, which should succeed for a lot of scenarios.


mai [](start = 49, length = 3)

main?

StephenLPeters · 2020-08-04T07:32:29Z

/azp run

azure-pipelines · 2020-08-04T07:32:42Z

Azure Pipelines successfully started running 1 pipeline(s).

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

/azp run

azure-pipelines · 2020-08-05T17:39:10Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-08-05T22:35:35Z

/azp run

azure-pipelines · 2020-08-05T22:35:47Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-08-06T02:22:26Z

After nearly 6 months this is going to finally get merged in! Thank you @ad1Dima for being such a trooper!

YuliKl · 2020-08-06T06:18:37Z

Thank you @ad1Dima for all your hard work on this!

Felix-Dev · 2020-08-06T07:52:08Z

How fitting then that with this merge, WinUI reached another milestone.

1000 closed issues!

ad1Dima · 2020-08-06T08:00:53Z

@Felix-Dev how did you get this dark theme?

Felix-Dev · 2020-08-06T08:07:25Z

@ad1Dima It's an addon called Dark Background and Light Text. I linked to a firefox version but it is also available for Chrome and related browsers. And it's fully customizable so you can configure its colors. Generally works pretty good on Github with just 1-2 minor color issues.

…-xaml#1997)

ad1Dima added 2 commits February 19, 2020 01:08

NavigationView BottomMenuItems

9e2a21c

fix ItemInvoked fired twice when SelectionFollowFocus in enabled

fd4493d

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

ad1Dima requested review from ojhad and StephenLPeters February 19, 2020 06:15

ranjeshj requested a review from YuliKl February 19, 2020 21:31

ad1Dima mentioned this pull request Feb 20, 2020

Fix NavView issue with SelectionChanged being raised upon displaymode change #1956

Merged

ranjeshj added area-NavigationView NavView control team-Controls Issue for the Controls team labels Feb 20, 2020

ranjeshj reviewed Feb 20, 2020

View reviewed changes

dev/NavigationView/NavigationView.xaml Outdated Show resolved Hide resolved

ranjeshj reviewed Feb 20, 2020

View reviewed changes

ranjeshj removed the needs-triage Issue needs to be triaged by the area owners label Feb 20, 2020

Fix SettingsItem AutomationProperties

20f1311

ojhad reviewed Mar 3, 2020

View reviewed changes

dev/NavigationView/NavigationView.xaml Show resolved Hide resolved

ojhad reviewed Mar 3, 2020

View reviewed changes

dev/NavigationView/NavigationView.idl Outdated Show resolved Hide resolved

ojhad reviewed Mar 3, 2020

View reviewed changes

dev/NavigationView/NavigationView.idl Outdated Show resolved Hide resolved

anawishnoff mentioned this pull request Aug 3, 2020

Bug/Feature Request: NavigationView FooterMenu - No overflow handling for Top mode #3029

Closed

Address PR comments

d192202

Implement FooterItemsSource properly

StephenLPeters reviewed Aug 3, 2020

View reviewed changes

fix typo and navigationViewItem Error

17854be

Merge from master

aece0a2

ojhad approved these changes Aug 6, 2020

View reviewed changes

StephenLPeters approved these changes Aug 6, 2020

View reviewed changes

StephenLPeters merged commit 2ec9b1c into microsoft:master Aug 6, 2020

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

NavigationView BottomMenuItems implementation (microsoft/microsoft-ui…

f74c8c7

…-xaml#1997)

Felix-Dev mentioned this pull request Aug 26, 2020

NavigationView: Fix app crash in Top mode when clearing the item collection while a selected item exists #3202

Merged

dependabot bot mentioned this pull request Oct 3, 2020

Bump ModernWpfUI from 0.9.1 to 0.9.2 astenlund/fs2ff#4

Merged

This was referenced Oct 19, 2020

NavigationView: PaneFooter containing NavigationViewItem crashes app when navigating focus via TAB to Settings item (Regression) #3445

Closed

NavigationView: Menu items push Footer area + Settings item out of view (starting with 2.5 prerelease) #3447

Closed

NavigationView BottomMenuItems implementation #1997

NavigationView BottomMenuItems implementation #1997

Conversation

ad1Dima commented Feb 19, 2020 • edited by ranjeshj Loading

Description

Motivation and Context

How Has This Been Tested?

ad1Dima commented Feb 20, 2020

ranjeshj commented Feb 20, 2020

azure-pipelines bot commented Feb 20, 2020

ranjeshj Feb 20, 2020 • edited by ojhad 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

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

ranjeshj Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

YuliKl Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

ad1Dima Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranjeshj commented Feb 20, 2020

ad1Dima commented Feb 21, 2020

azure-pipelines bot commented Feb 21, 2020

StephenLPeters commented Mar 2, 2020

azure-pipelines bot commented Mar 2, 2020

ojhad commented Mar 3, 2020 • edited Loading

ad1Dima commented Mar 4, 2020

ranjeshj commented Mar 4, 2020

ad1Dima commented Mar 4, 2020

StephenLPeters commented Mar 6, 2020

ad1Dima commented Aug 1, 2020

anawishnoff commented Aug 3, 2020

ad1Dima commented Aug 3, 2020

StephenLPeters commented Aug 3, 2020

azure-pipelines bot commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenLPeters commented Aug 4, 2020

azure-pipelines bot commented Aug 4, 2020

StephenLPeters commented Aug 5, 2020

azure-pipelines bot commented Aug 5, 2020

StephenLPeters commented Aug 5, 2020

azure-pipelines bot commented Aug 5, 2020

StephenLPeters commented Aug 6, 2020

YuliKl commented Aug 6, 2020

Felix-Dev commented Aug 6, 2020

ad1Dima commented Aug 6, 2020

Felix-Dev commented Aug 6, 2020 • edited Loading

ad1Dima commented Feb 19, 2020 •

edited by ranjeshj

Loading

ranjeshj Feb 20, 2020 •

edited by ojhad

Loading

ranjeshj Feb 20, 2020 •

edited

Loading

YuliKl Feb 20, 2020 •

edited

Loading

ad1Dima Feb 21, 2020 •

edited

Loading

ojhad commented Mar 3, 2020 •

edited

Loading

Felix-Dev commented Aug 6, 2020 •

edited

Loading