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: Fix IsPaneOpen=false ignored on launch when pane is launched in Left expanded state #3197

Conversation

Felix-Dev
Copy link
Contributor

@Felix-Dev Felix-Dev commented Aug 25, 2020

Description

This PR fixes an issue where setting the IsPaneOpen property of the NavigationView to false before launching the NavigationView in the Left expanded state (either explicitly or via the Auto pane display mode) would have no effect (the pane is launched in an open state).

The updated behavior of the NavigationView.IsPaneOpen API is the following on NavigationView launch:

PaneDisplayMode IsPaneOpen value Pane will be opened Remarks
Auto True If NavigationView is set into LeftMinimal/LeftCompact mode the pane will be launched in a closed state and IsPaneOpen will be set to false. If the NavigationView is set into Left mode the pane will be launched in an opened state
LeftMinimal True IsPaneOpen will be set to false
LeftCompact True IsPaneOpen will be set to false
Left True -
Top True - No pane to open
Auto False Pane will be launched in a closed state for every app window width (*)
LeftMinimal False -
LeftCompact False -
Left False (*)
Top False - No pane to open

(*) = This behavior has been updated as part of this PR. The pane is now closed when IsPaneOpen is set to false and the pane is launched in Left expanded state (either explicitly or via the Auto display mode). All other configurations you see in the table above are the current behavior and unchanged by this PR.

Once this PR has been merged we should also consider providing this information about the PaneDisplayMode and IsPaneOpen API interplay on NavigationView launch on the NavigationView.IsPaneOpen API documentation page (for example in a "Remarks" section) starting with whatever WinUI version will include this change.

Motivation and Context

Fixes #2391.

How Has This Been Tested?

Tested manually and added an API test.

Additional Context

@YuliKl FYI.

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Aug 25, 2020
@YuliKl YuliKl 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 Aug 26, 2020
@StephenLPeters
Copy link
Contributor

I'd like @ojhad to weigh in, we had a conversation about this behavior during a debug session a few weeks ago. Do you remember the context of that conversation?

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad
Copy link
Contributor

ojhad commented Sep 2, 2020

I'd like @ojhad to weigh in, we had a conversation about this behavior during a debug session a few weeks ago. Do you remember the context of that conversation?

If I remember correctly, we were launching into compact mode and that the pane was being forced into the closed state regardless of the IsPaneOpen property (which, according to above, seems like the intended behavior).

However changing the IsPaneOpen property was causing different codepaths to get taken (hence the content dialog issue). When it was set to true, during setup, that value propagated to splitview which caused it to try add children to the popup root during measure.

The problem was that the DisplayMode on splitview was being influenced by this property, which is why I deferred the update of splitview's displaymode to OnLoaded: https://github.com/microsoft/microsoft-ui-xaml/pull/2920/files

@StephenLPeters
Copy link
Contributor

@Felix-Dev The pipeline fix just went in, could you please merge master into this PR?

@Felix-Dev
Copy link
Contributor Author

@StephenLPeters Merged with master.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

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

:shipit:

@StephenLPeters StephenLPeters merged commit 1907990 into microsoft:master Sep 8, 2020
Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Sep 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NavigationView NavView control team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show NavigationPane on collapsed by default on PaneDisplayMode Left
5 participants