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 16 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
66 changes: 38 additions & 28 deletions dev/NavigationView/NavigationView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ static constexpr auto c_paneHeaderContentBorderRow = L"PaneHeaderContentBorderRo

static constexpr auto c_separatorVisibleStateName = L"SeparatorVisible";
static constexpr auto c_separatorCollapsedStateName = L"SeparatorCollapsed";
static constexpr auto c_visualItemsSeparator = L"VisualItemsSeparator"sv;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not look like you use this anymore.


static constexpr int c_backButtonHeight = 40;
static constexpr int c_backButtonWidth = 40;
Expand All @@ -93,7 +94,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 @@ -658,6 +658,8 @@ void NavigationView::OnApplyTemplate()
m_itemsContainerRow.set(GetTemplateChildT<winrt::RowDefinition>(c_itemsContainerRow, controlProtected));
m_menuItemsScrollViewer.set(GetTemplateChildT<winrt::FrameworkElement>(c_menuItemsScrollViewer, controlProtected));
m_footerItemsScrollViewer.set(GetTemplateChildT<winrt::FrameworkElement>(c_footerItemsScrollViewer, controlProtected));
m_visualItemsSeparator.set(GetTemplateChildT<winrt::NavigationViewItemSeparator>(c_visualItemsSeparator, controlProtected));


m_itemsContainerSizeChangedRevoker.revoke();
if (const auto itemsContainer = GetTemplateChildT<winrt::FrameworkElement>(c_itemsContainer, controlProtected))
Expand Down Expand Up @@ -1491,27 +1493,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 +1512,36 @@ 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]() {
const auto footerItemsRepeaterMargin = footerItemsRepeater.Margin();
return footerItemsRepeater.ActualHeight() + footerItemsRepeaterMargin.Top + footerItemsRepeaterMargin.Bottom;
}();

const auto menuItemsActualHeight = [this, menuItems]() {
const auto menuItemsMargin = menuItems.Margin();
return menuItems.ActualHeight() + menuItemsMargin.Top + menuItemsMargin.Bottom;
}();

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

const auto visualItemsSeparatorHeight = [this]() {
if (const auto& visualItemsSeparator = m_visualItemsSeparator.get())
{
const auto visualItemsSeparatorMargin = visualItemsSeparator.Margin();
return visualItemsSeparator.ActualHeight() + visualItemsSeparatorMargin.Top + visualItemsSeparatorMargin.Bottom;
}
return 0.0;
}();

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

Choose a reason for hiding this comment

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

Adding the visualItemsSeparatorHeight feels incorrect to me: you don't want the separator to be shown because of a lack of space due to the separator being shown. You know what I mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that too... but it was part of the calculation before so I simply included it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline, will remove.


if (m_footerItemsSource.Count() == 0 && !IsSettingsVisible())
{
Expand All @@ -1544,12 +1554,12 @@ void NavigationView::UpdatePaneLayout()
winrt::VisualStateManager::GoToState(*this, c_separatorCollapsedStateName, false);
return 0.0;
}
else if (totalAvailableHeight > menuItemsActualHeight + footersActualHeight)
else if (totalAvailableHeight > menuItemsActualHeight + footerGroupActualHeight)
Copy link
Contributor

Choose a reason for hiding this comment

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

In that MUXControlsTestPage first NavigationView test page, totalAvailableHeight happens to be menuItemsActualHeight + footerGroupActualHeight + 4.0

The 4.0 comes from the missing paneFooter.Margin.Bottom mentioned above. Once that is fixed, you will end up with
totalAvailableHeight == menuItemsActualHeight + footerGroupActualHeight and this branch will incorrectly be skipped.

I believe this is because instead of:
else if (totalAvailableHeight > menuItemsActualHeight + footerGroupActualHeight)
we should be using:
else if (totalAvailableHeight >= menuItems.DesiredSize.Height + footerGroupActualHeight)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is being incorrectly skipped. If totalAvailableHeight == menuItemsActualHeight + footerGroupActualHeight, logically, there should be a pane separator to distinguish the two sections rather than it being collapsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. You're right, it would make more sense to add the >=

{
// 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)
{
Expand All @@ -1558,12 +1568,12 @@ void NavigationView::UpdatePaneLayout()
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
1 change: 1 addition & 0 deletions dev/NavigationView/NavigationView.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ class NavigationView :
tracker_ref<winrt::Border> m_topNavContentOverlayAreaGrid{ this };
tracker_ref<winrt::Grid> m_shadowCaster{ this };
tracker_ref<winrt::Storyboard> m_shadowCasterEaseOutStoryboard{ this };
tracker_ref<winrt::NavigationViewItemSeparator> m_visualItemsSeparator{ this };
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not look like you use this anymore.


// Indicator animations
tracker_ref<winrt::UIElement> m_prevIndicator{ this };
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