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

Layout Cycle Crash + Misc Fixes for NavigationView #5084

Merged
merged 22 commits into from
Jun 12, 2021
Merged

Layout Cycle Crash + Misc Fixes for NavigationView #5084

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
275cdc9
separator margn fix
karkarl May 24, 2021
e36718c
add border and cornerRadius
karkarl May 4, 2021
06f90d7
fix jumping borders
karkarl May 5, 2021
18b686a
fix pixel jumping issue
karkarl May 5, 2021
026ab45
tweak resources
karkarl May 24, 2021
1ee640f
set NavigationViewContentGridBorderThickness default to 0
karkarl May 24, 2021
4c7ed20
fix bad merge
karkarl May 24, 2021
c0ae763
add border to content when in topnav
karkarl May 25, 2021
24a3ad8
changes from comments
karkarl May 25, 2021
24c4ed4
update verification files
karkarl May 26, 2021
4b91172
fix pane behaviour testS
karkarl May 26, 2021
74d03fe
fix tests
karkarl Jun 4, 2021
642124d
refactor updatePaneLayout
karkarl Jun 10, 2021
c3c4811
clean up
karkarl Jun 10, 2021
db5b373
changes from comments
karkarl Jun 10, 2021
81ab023
add separator and margin top bottom to Height calculation
karkarl Jun 10, 2021
2b6a3ea
changes from comments
karkarl Jun 10, 2021
35bd4b0
changes from comments
karkarl Jun 11, 2021
34a930a
fix tests
karkarl Jun 11, 2021
30a4f0c
update verification files
karkarl Jun 11, 2021
c273364
revert separator default visiblity
karkarl Jun 11, 2021
c89a7a5
update verification files
karkarl Jun 11, 2021
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
79 changes: 50 additions & 29 deletions dev/NavigationView/NavigationView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ static constexpr int c_paneToggleButtonWidth = 40;
static constexpr int c_toggleButtonHeightWhenShouldPreserveNavigationViewRS3Behavior = 56;
static constexpr int c_backButtonRowDefinition = 1;
static constexpr float c_paneElevationTranslationZ = 32;
static constexpr int c_paneItemsSeparatorHeight = 9;

static constexpr int c_mainMenuBlockIndex = 0;
static constexpr int c_footerMenuBlockIndex = 1;
Expand Down Expand Up @@ -659,6 +658,7 @@ void NavigationView::OnApplyTemplate()
m_menuItemsScrollViewer.set(GetTemplateChildT<winrt::FrameworkElement>(c_menuItemsScrollViewer, controlProtected));
m_footerItemsScrollViewer.set(GetTemplateChildT<winrt::FrameworkElement>(c_footerItemsScrollViewer, controlProtected));


m_itemsContainerSizeChangedRevoker.revoke();
if (const auto itemsContainer = GetTemplateChildT<winrt::FrameworkElement>(c_itemsContainer, controlProtected))
{
Expand Down Expand Up @@ -1491,27 +1491,7 @@ void NavigationView::UpdatePaneLayout()
}
return 0.0;
}();
auto availableHeight = paneContentRow.ActualHeight() - itemsContainerMargin;

// The c_paneItemsSeparatorHeight is to account for the 9px separator height that we need to subtract.
if (PaneFooter())
{
availableHeight -= c_paneItemsSeparatorHeight;
if (const auto& paneFooter = m_leftNavFooterContentBorder.get())
{
availableHeight -= paneFooter.ActualHeight();
}
}
else if (IsSettingsVisible())
{
availableHeight -= c_paneItemsSeparatorHeight;
}
else if (m_footerItemsSource && m_menuItemsSource && m_footerItemsSource.Count() * m_menuItemsSource.Count() > 0)
{
availableHeight -= c_paneItemsSeparatorHeight;
}

return availableHeight;
return paneContentRow.ActualHeight() - itemsContainerMargin;
}
return 0.0;
}();
Expand All @@ -1530,8 +1510,49 @@ void NavigationView::UpdatePaneLayout()
// We know the actual height of footer items, so use that to determine how to split pane.
if (const auto& menuItems = m_leftNavRepeater.get())
{
const auto footersActualHeight = footerItemsRepeater.ActualHeight();
const auto menuItemsActualHeight = menuItems.ActualHeight();
const auto footersActualHeight = [this, footerItemsRepeater]() {
double footerItemsRepeaterTopBottomMargin = 0.0;
if (footerItemsRepeater.Visibility() == winrt::Visibility::Visible)
{
const auto footerItemsRepeaterMargin = footerItemsRepeater.Margin();
footerItemsRepeaterTopBottomMargin = footerItemsRepeaterMargin.Top + footerItemsRepeaterMargin.Bottom;
}
return footerItemsRepeater.ActualHeight() + footerItemsRepeaterTopBottomMargin;
}();

const auto paneFooterActualHeight = [this]() {
if (const auto& paneFooter = m_leftNavFooterContentBorder.get())
{
double paneFooterTopBottomMargin = 0.0;
if (paneFooter.Visibility() == winrt::Visibility::Visible)
{
const auto paneFooterMargin = paneFooter.Margin();
paneFooterTopBottomMargin = paneFooterMargin.Top + paneFooterMargin.Bottom;
}
return paneFooter.ActualHeight() + paneFooterTopBottomMargin;
}
return 0.0;
}();

const auto menuItemsTopBottomMargin = [this, menuItems]() {
if (menuItems.Visibility() == winrt::Visibility::Visible)
{
const auto menuItemsMargin = menuItems.Margin();
return menuItemsMargin.Top + menuItemsMargin.Bottom;
}
return 0.0;
}();

// This is the value computed during the measure pass of the layout process. This will be the value used to determine
// the partition logic between menuItems and footerGroup, since the ActualHeight may be taller if there's more space.
const auto menuItemsDesiredHeight = menuItems.DesiredSize().Height + menuItemsTopBottomMargin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike ActualHeight/ActualWidth, UIElement.DesiredSize already includes the Margin. So it should be:

const auto menuItemsDesiredHeight = menuItems.DesiredSize().Height;


// This is what the height ended up being, so will be the value that is used to calculate the partition
// between menuItems and footerGroup.
const auto menuItemsActualHeight = menuItems.ActualHeight() + menuItemsTopBottomMargin;

// Footer and PaneFooter are included in the footerGroup to calculate available height for menu items.
const auto footerGroupActualHeight = footersActualHeight + paneFooterActualHeight;

if (m_footerItemsSource.Count() == 0 && !IsSettingsVisible())
{
Expand All @@ -1544,26 +1565,26 @@ void NavigationView::UpdatePaneLayout()
winrt::VisualStateManager::GoToState(*this, c_separatorCollapsedStateName, false);
return 0.0;
}
else if (totalAvailableHeight > menuItemsActualHeight + footersActualHeight)
else if (totalAvailableHeight >= menuItemsDesiredHeight + footerGroupActualHeight)
{
// We have enough space for two so let everyone get as much as they need.
footerItemsScrollViewer.MaxHeight(footersActualHeight);
winrt::VisualStateManager::GoToState(*this, c_separatorCollapsedStateName, false);
return totalAvailableHeight - footersActualHeight;
return totalAvailableHeight - footerGroupActualHeight;
}
else if (menuItemsActualHeight <= totalAvailableHeightHalf)
else if (menuItemsDesiredHeight <= totalAvailableHeightHalf)
{
// Footer items exceed over the half, so let's limit them.
footerItemsScrollViewer.MaxHeight(totalAvailableHeight - menuItemsActualHeight);
winrt::VisualStateManager::GoToState(*this, c_separatorVisibleStateName,false);
return menuItemsActualHeight;
}
else if (footersActualHeight <= totalAvailableHeightHalf)
else if (footerGroupActualHeight <= totalAvailableHeightHalf)
{
// Menu items exceed over the half, so let's limit them.
footerItemsScrollViewer.MaxHeight(footersActualHeight);
winrt::VisualStateManager::GoToState(*this, c_separatorVisibleStateName, false);
return totalAvailableHeight - footersActualHeight;
return totalAvailableHeight - footerGroupActualHeight;
}
else
{
Expand Down
23 changes: 15 additions & 8 deletions dev/NavigationView/NavigationView.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@
</VisualState.Setters>
</VisualState>

<VisualState x:Name="TopNavigationMinimal" />
<VisualState x:Name="TopNavigationMinimal">
<VisualState.Setters>
<Setter Target="ContentGrid.BorderThickness" Value="{ThemeResource TopNavigationViewContentGridBorderThickness}" />
</VisualState.Setters>
</VisualState>

<VisualState x:Name="MinimalWithBackButton">
<VisualState.Setters>
Expand Down Expand Up @@ -181,10 +185,10 @@
</VisualStateGroup>

<VisualStateGroup x:Name="PaneSeparatorStates">
<VisualState x:Name="SeparatorCollapsed"/>
<VisualState x:Name="SeparatorVisible">
<VisualState x:Name="SeparatorVisible"/>
<VisualState x:Name="SeparatorCollapsed">
<VisualState.Setters>
<Setter Target="VisualItemsSeparator.Visibility" Value="Visible"/>
<Setter Target="VisualItemsSeparator.Visibility" Value="Collapsed"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, why is this switched now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh oops. I was previously getting the separator ActualHeight from codebehind, and for it to evaluate to a non-zero value, it needed to be visible by default. The separator is no longer used for pane layout calculations, so I should revert this.

</VisualState.Setters>
</VisualState>
</VisualStateGroup>
Expand Down Expand Up @@ -585,7 +589,7 @@
x:Name="VisualItemsSeparator"
Grid.Row="1"
Margin="0,0,0,2"
Visibility="Collapsed"
Visibility="Visible"
VerticalAlignment="Center"
HorizontalAlignment="Stretch"/>

Expand Down Expand Up @@ -614,8 +618,11 @@
<SplitView.Content>
<Grid
x:Name="ContentGrid"
BorderBrush="{ThemeResource NavigationViewContentGridBorderBrush}"
BorderThickness="{ThemeResource NavigationViewContentGridBorderThickness}"
Background="{ThemeResource NavigationViewContentBackground}"
Margin="{ThemeResource NavigationViewContentMargin}">
Margin="{ThemeResource NavigationViewContentMargin}"
contract7Present:CornerRadius="{ThemeResource NavigationViewContentGridCornerRadius}">
<Grid.RowDefinitions>
<RowDefinition Height="Auto" />
<RowDefinition Height="Auto" />
Expand Down Expand Up @@ -646,6 +653,7 @@
Style="{StaticResource NavigationViewTitleHeaderContentControlTextStyle}"/>

<ContentPresenter
x:Name="ContentPresenter"
AutomationProperties.LandmarkType="Main"
Grid.Row="2"
Grid.ColumnSpan="2"
Expand Down Expand Up @@ -899,7 +907,6 @@
<Setter Property="IsEnabled" Value="False" />
<Setter Property="IsTabStop" Value="False" />
<Setter Property="MinHeight" Value="0" />
<Setter Property="Margin" Value="{ThemeResource NavigationViewItemSeparatorMargin}" />
<Setter Property="AutomationProperties.AccessibilityView" Value="Raw" />
<Setter Property="Template">
<Setter.Value>
Expand Down Expand Up @@ -928,7 +935,7 @@
x:Name="SeparatorLine"
Height="{ThemeResource NavigationViewItemSeparatorHeight}"
Fill="{ThemeResource NavigationViewItemSeparatorForeground}"
Margin="{TemplateBinding Margin}"/>
Margin="{ThemeResource NavigationViewItemSeparatorMargin}"/>
beervoley marked this conversation as resolved.
Show resolved Hide resolved
</Grid>
</ControlTemplate>
</Setter.Value>
Expand Down
13 changes: 6 additions & 7 deletions dev/NavigationView/NavigationView_InteractionTests/PaneBehaviorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,14 +1112,13 @@ public void VerifyPaneLayoutDynamicallyUpdatingCollectionsFooterPrecedence()

// Fill footer items
AddFooterItem(200, 200);
AddFooterItem(200, 200);
AddFooterItem(200, 200);
// Reached maximum height
AddFooterItem(200, 200);
AddFooterItem(190, 205);
AddFooterItem(190, 210);
AddFooterItem(180, 215);

AddMenuItem(200, 200);
AddMenuItem(200, 200);
AddMenuItem(200, 200);
AddMenuItem(180, 220);
AddMenuItem(170, 225);
AddMenuItem(195, 200);

void VerifyHeights(double menuItemsHeight, double footerItemsHeight)
{
Expand Down
Loading