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.

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

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

Closed
Felix-Dev opened this issue Oct 19, 2020 · 24 comments · Fixed by #3602
Assignees
Labels
area-NavigationView NavView control help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Milestone

Comments

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Oct 19, 2020

Describe the bug
Originally reported by @Parth here. With PR #1997 (shipped with WinUI 2.5.0-prerelease.200812001) adding the new FooterMenuItem APIs, we now end up with (primary) menu items pushing the NavigationView's PaneFooter items and Settings item out of view in Left navigation. See the following GIFs:

WinUI 2.4 WinUI 2.5.0-prerelease.200812001 (and later)
navview-panefooter-settings-no-push navview-panefooter-settings-push

I think the new behavior introduced in WinUI 2.5.0-prerelease.200812001 should be reverted as the Settings item should always be visible. And in Top navigation, we still follow that layout:

image

Reading through some of the discussions in PR #1997 it appears this design choice was made here. Tagging @ad1Dima and @YuliKl who discussed this before for their thoughts on this matter. Also @StephenLPeters.

The newly introduced FooterMenu items area is located between the PaneFooter area and the Settings item:
image

One option here would be to have these three areas (FooterMenu, PaneFooter, Settings item) "locked" into view while the MenuItems area (primary navigation) remains scrollable as seen in WinUI 2.4. This would also match the behavior in Top mode where we only move primary navigation items into the overflow menu and keep the footer items + Settings item locked in place.

Version Info

NuGet package version:
Introduced in WinUI 2.5.0-prerelease.200812001.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Oct 19, 2020
@ranjeshj ranjeshj added area-NavigationView NavView control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Oct 19, 2020
@YuliKl
Copy link

YuliKl commented Oct 19, 2020

I believe as part of adding the FooterMenu API we moved the Settings item into FooterMenu. We automatically add a ScrollViewer to both FooterMenu and the primary MenuItems. We don't add a ScrollViewer to PaneFooter.

I think what we need are three distinct areas: MenuItems docked to the top, FooterMenu docked to the bottom, and PaneFooter floating between them. I'm not sure that we can correctly calculate the heights of FooterMenu and MenuItems, though, especially now that items can be hierarchical and expand in place.

But I think I agree with @Felix-Dev's assertion that FooterMenu should not share a ScrollViewer with MenuItems and instead should be locked into view, at least at a min height of one item.

@ad1Dima
Copy link
Contributor

ad1Dima commented Oct 20, 2020

There is one UX concern with pinned footer: sometimes my users don't understand that there is more items in main menu.
image

But I think I agree with @Felix-Dev's assertion that FooterMenu should not share a ScrollViewer with MenuItems and instead should be locked into view, at least at a min height of one item.

Should it be first FooterItem or should it be SettingsMenuItem?

@ad1Dima
Copy link
Contributor

ad1Dima commented Oct 20, 2020

May be it should be an option to pin or unpin footer menu?

@ad1Dima
Copy link
Contributor

ad1Dima commented Oct 20, 2020

This would also match the behavior in Top mode where we only move primary navigation items into the overflow menu and keep the footer items + Settings item locked in place.

there is issue for this #3029

@YuliKl
Copy link

YuliKl commented Oct 20, 2020

Should it be first FooterItem or should it be SettingsMenuItem?

I think we should pin the last FooterItem to match the bottom-aligned behavior I'm aiming for. That will result in the settings item being the visible one most of the time, unless the has chosen to set IsSettingsVisible to False.

May be it should be an option to pin or unpin footer menu?

NavigationView is such a complex control already I worry about introducing even more APIs. @ad1Dima, is this something you feel you need, or was this just an idea to solve hypothetical future issues?

@ad1Dima
Copy link
Contributor

ad1Dima commented Oct 21, 2020

@ad1Dima, is this something you feel you need, or was this just an idea to solve hypothetical future issues?

I made same same behavior in my app before i used NavigationView. So yes i would like to have an option to have all menu in one ScrollView. I think it would be better case for non-hierarchical menu. But i understand inconveniences in hierarchical case.

And I don't like idea of scroll sized as one element at all. It will be frustrating with touchscreen and not obvious with mouse.

@ranjeshj
Copy link
Contributor

ranjeshj commented Nov 4, 2020

We need to have the settings item and footer item stick to the bottom and not scroll away to keep the behavior from changing from what we previously had. I'm not sure if two scroll viewers are the best option though. Don't we always want the settings item to be pinned to the bottom ? If footer items are in a ScrollViewer we could end up with it going out of view.

What if we took the footer and footer menu items out of the ScrollViewer and gave it all the space it can take (rowdef auto). Then give the rest of the space to the menu items which can be inside the ScrollViewer ?

That way footer/footer items will always be pinned at the bottom and visible, while the menu items can scroll if there are many of them.

The only difference here is that footer menu items will not scrollable, so if we expect lots of footer items it won't work well. But that might be a corner case situation where those could be added as part of the menu items instead to get a reasonable experience.

@marcelwgn
Copy link
Contributor

I think having the footers have row height auto is the best approach here, usually there are only a few items in the footer while the MenuItems will contain a lot of items. Of course developers could choose to do it the other way around, but I think that isn't very common if it happens at all. Trying to find a solution that works well for both cases would probably be too expensive (dynamically determining which of those areas occupies too much space and decide during runtime). The important thing is that the settings is pinned at the bottom, having to scroll for that doesn't seem like a good user experience.

@YuliKl
Copy link

YuliKl commented Nov 4, 2020

The important thing is that the settings is pinned at the bottom, having to scroll for that doesn't seem like a good user experience.

I agree with that.

And while I also agree with @ranjeshj's suggestion of giving footer menu items all the room they want, I worry about the corner cases. Can we define their row height as auto but also specify a max height? I just don't know what to set the max to...ideally something like (window height) - NavigationViewItem.Height - PaneToggleButton.ActualHeight - BackButton.ActualHeight - NavigationView.AutoSuggestBox.ActualHeight...which is a bit unwieldy.

And of course all this is for left nav. We'll need to consider top nav separately.

@ranjeshj
Copy link
Contributor

ranjeshj commented Nov 4, 2020

we could set a min height on the menu items (at least one item height) so that it doesn't just disappear if there are too many footer items. @YuliKl If we had lots of menu items and lots of footer items, how much space should each get ? I'm wondering if there is a simpler way to achieve the calculation you have above. If we set a max height on the footer items, we would end up clipping once it goes past that height.

@YuliKl
Copy link

YuliKl commented Nov 4, 2020

If we set a max height on the footer items, we would end up clipping once it goes past that height.

Right. I think footer menu items need to be in a scroll view to prevent clipping from happening. Hopefully we can make their max height tall enough that most of the time (assuming a small number of footer items) the scroll bar would not show.

If we had lots of menu items and lots of footer items, how much space should each get ?

Let's stipulate that footer menu items are allowed to get all the space they need up to a certain max height. Main menu items will get the rest. I suspect the common case will continue to be lots of menu items, a small number of footer menu items. So this behavior should produce the desired outcome in the common case. In the less common case where there are many of both types, footer menu items will visually dominate...which is not ideal but acceptable in my opinion.

@marcelwgn
Copy link
Contributor

marcelwgn commented Nov 4, 2020

How would the panefooter play into this? What if both the PaneFooter and the FooterItems request space and that would end up being too much for the whole pane? I would argue that we should do something like this, but I am not sure if it's ideal:

<Grid.RowDefinitions>
  <!-- MenuItems -->
  <RowDefinition MinHeight="1 Item" Height="*" />
  <!-- FooterItems -->
  <RowDefinition MinHeight="1 Item" Height="Auto" />
  <!-- PaneFooter -->
  <RowDefinition Height="Auto" /> <---- This should take precedence over everything while respecting minheight.
</Grid.RowDefinitions>

Edit: Updated like Ranjesh suggested.

@ranjeshj
Copy link
Contributor

ranjeshj commented Nov 4, 2020

@chingucoding If we give auto height to menu items, when it grows, it will take up as much as it can and push everything out. I think menu items should get * size (to take up remaining space).

Footer getting auto makes sense. Probably don't need a min height on Footer, since there can be no footer also in which case we don't want to take up that space.

FooterItems is the tricky one. If we did auto, it can take up a lot and push everything else, so we want to give it room to grow, but with a max height. In an effort to simplify this corner case, we could have max height allow x number of items. I think setting max height calculated based on the sizes of the rest of the items/parent in the tree is going to end up with us chasing layout cycles.

Frankly, we should probably not have the second ScrollViewer at all and update the guidance to not use lots of items in the footer menu items. Even if we handle this corner case, I think it does not provide a good experience (Either settings scrolls away or footer items take a lot of space).

@marcelwgn
Copy link
Contributor

Good point. I think having no second scroll viewer is the best way to handle this. Two scrollviewers underneath each other would look quite odd. Putting only the main items in a scrollviewer and let the main items take whatever is left of the pane seems like a good plan to me.

@StephenLPeters
Copy link
Contributor

StephenLPeters commented Nov 5, 2020

I think that the easiest way to fix this is to make the pane for the foot items maximum height be half of the navigation views height (minus the hamburger and auto suggest box). I think the way to do this is attach to the navigation view pane's size changed event and from code behind ( 👎 )set the maximum size of the pane footer items to 1/2 the actual size of the navigation view pane. This will cause the layout system to relayout, however the navigation view pane doesn't change its size based on its children so it wont cause a cycle and the second layout will have the footer pane items positioned correctly. We would then need these items to be in their own scroll viewer an then make sure that the settings item is in the view port. I think with old scrollviewer the only way to do this is to use BringIntoView().

@RBrid is there magic we could use (other than BringIntoView) to make sure that when the view port of a scroll viewer changes we keep the last item in the scrollviewer in view? Maybe scroll anchoring?

@YuliKl
Copy link

YuliKl commented Nov 5, 2020

I like the plan @StephenLPeters just proposed. We'll need two scroll viewers to guarantee that a user can always reach all items in the pane, no matter how short their app window is. Two distinct scroll bars isn't so bad, especially because they should not both show in a majority of use cases. But we cannot make the assumption that all footer menu items will always fit without scrolling.

Also want to add that while keeping the bottom of the footer menu items (really, the Settings item) always in view is preferable, it's not a must. If BringIntoView proves to be a poor fit for some reason and there's no better method, we could ship without this.

@ad1Dima
Copy link
Contributor

ad1Dima commented Nov 5, 2020

is there magic we could use (other than BringIntoView) to make sure that when the view port of a scroll viewer changes we keep the last item in the scrollviewer in view? Maybe scroll anchoring?

In good old times of Windows Phone there was trick for messengers: Rotate ScrollView by 180° and then rotate items back by 180°. Not sure this is good method in this case.

@ranjeshj
Copy link
Contributor

ranjeshj commented Nov 7, 2020

The approach Stephen outlined about sounds like a good option. For the scroll into view behavior we are looking for here, setting ScrollViewer.VerticalAnchorRatio to 1.0 might do the trick.

@ranjeshj ranjeshj added the help wanted Issue ideal for external contributors label Nov 11, 2020
@ranjeshj
Copy link
Contributor

@chingucoding would you be interested in contributing a fix for this ?

@marcelwgn
Copy link
Contributor

Sure, feel free to assign the issue to me.

@marcelwgn
Copy link
Contributor

To clarify, the Settings item will stay a part of the FooterMenuItems? During the API review for that (microsoft/microsoft-ui-xaml-specs#101) it seemed more natural to have the settings item be a distinct item and not be part of the FooterMenuItems while also being seperated from FooterMenuItems through the PaneFooter. @MikeHillberg @anawishnoff FYI.

@anawishnoff
Copy link
Contributor

@chingucoding Correct, we ended up not changing this after the API review. As was the case previously, the settings item is technically a part of FooterMenuItems, but it doesn't get added to that list until the app runs. Thanks Marcel!

@marcelwgn
Copy link
Contributor

Alright, thanks for the clarification Ana!

@Noemata
Copy link

Noemata commented Dec 11, 2020

Please see notes I added to : #3771

On further reflection, having IsSettingsVisible="False" should do all of the needed work to get the behavior I desire.

Lastly, this used to work, so the addition of a ScrollViewer in the footer introduced a regression. Please try not to break existing code when adding new functionality. It's been a constant battle fixing existing working code due to design changes that do not preserve previous functionality. And if you are going to make a breaking change, add in the ability to revert back to old behavior. That way we can opt in to new features as we become ready for them without having to hunt down all the breaking bits in a large existing code base.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Dec 11, 2020
@StephenLPeters StephenLPeters removed the needs-triage Issue needs to be triaged by the area owners label Dec 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NavigationView NavView control help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants