NavigationView: Fix overflow button not being collapsed when the overflow menu only contains one item and NavigationView menu items are removed to empty the overflow menu

[Felix-Dev]

Description

This PR fixes the top NavigationView's overflow button not being collapsed when the overflow menu only contains a single item and

• that overflow menu item is removed or
• enough primary menu item(s) are removed to create enough room for the overflow menu item be displayed in the NavigationView pane area.

Currently, the visibility of the overflow button is set during a NavigationView::MeasureOverride() run, however, that method is not called in the aforementioned cases. To update the visibility in those cases, we listen to changes to the internal overflow menu item collection and collapse the overflow button when that collection no longer contains any item.

Motivation and Context

Fixes #3086.

How Has This Been Tested?

Tested visually and added interaction test.

Gif:

(The GIF above shows the newly added test page for the interaction test.)

[StephenLPeters]

@Felix-Dev can you merge master to pick up the CI build fix that just went in?

[StephenLPeters]

@Felix-Dev Ping in case updating this PR got missed

[Felix-Dev]

@StephenLPeters Thanks for the ping, missed it indeed. Merged with master now.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

void NavigationView::SetOverflowButtonVisibility(winrt::Visibility const& visibility)

@Felix-Dev do you understand why this is a template setting and not a visual state?

---

Refers to: dev/NavigationView/NavigationView.cpp:3296 in a1d6b9f. [](commit_id = a1d6b9f, deletion_comment = False)

[StephenLPeters]

[Felix-Dev]

do you understand why this is a template setting and not a visual state?

@StephenLPeters I'm afraid not. A visual state group for the overflow button visibility works perfectly fine here based on my tests (both manual and automatic ones). I'm not sure why this was made a public TemplateSettings API instead. Perhaps @ojhad knows more?

We could, for now, change the internal logic of the NavigationView so that the visibility of the overflow button is controlled by a visual state group instead. The template settings updating logic would have to be kept, however, to avoid breaking changes.

Barring additional info on the reasoning behing the TemplateSettings API, I would always prefer using visual states here.

Thoughts?

[StephenLPeters]

do you understand why this is a template setting and not a visual state?

@StephenLPeters I'm afraid not. A visual state group for the overflow button visibility works perfectly fine here based on my tests (both manual and automatic ones). I'm not sure why this was made a public TemplateSettings API instead. Perhaps @ojhad knows more?

We could, for now, change the internal logic of the NavigationView so that the visibility of the overflow button is controlled by a visual state group instead. The template settings updating logic would have to be kept, however, to avoid breaking changes.

Barring additional info on the reasoning behing the TemplateSettings API, I would always prefer using visual states here.

Thoughts?

I also definitely prefer a visual state group to this template settings method... and also agree we can't remove it :'( @ojhad or @YuliKl do either of you know?

[ojhad]

I also definitely prefer a visual state group to this template settings method... and also agree we can't remove it :'( @ojhad or @YuliKl do either of you know?

I do not know what the use case in mind was when the decision was made to place this in template settings.
I think @YuliKl or @licanhua might know?

[licanhua]

Most of time, templatesetting is recommended:

1. A lot of user likes re-template. it's easy for them to copy control template, and then make small change and show up in a new appearance.
2. UI changed from release to release, but we should support the UI template which shipped in old release. For example, we may rename the button

In the start, using code to control visual is widely used, but it becauses harder and harder to maintain.

1. the visual state becomes complex and complex. to maintain the combination of "states" and 'states group', but also keep back compatibility is not easy
2. interaction with measurement. In the measure, we disable overflow to see all items can be fit in, then enable it and measure again. You can think visual state is an async call, but in measure, I needs an immediate result.
3. use templatesettings can decouple the code from UI.

[licanhua]

Of course, If we just make overflow button as an example, there is no significant difference to use "visual state" or templatesetting.
but if you need to support five variables(for example, control five button's visibility), templatesettings would be a better choice.

[StephenLPeters]

Template settings has the very large downside of being an API though, which we can't change and have to cary with us everywhere we go. In general we've prefered using VSM to using template settings. However, your point about navigation view's measure override needing this explains why we need a template setting here. Thanks.

[Felix-Dev]

I see. Thanks for the explanation, @licanhua!