Skip to content

Commit

Permalink
TabView: Fix newly-added items not respecting TabWidthMode [compact] …
Browse files Browse the repository at this point in the history 
…when using an ItemsSource collection (#3118)

* Fix the following issues when populating the TabView via its ItemsSource API:
* crash when clearing the ItemsSource
* TabWidthMode [compact] being ignored in XAML Markup and for newly-added items

* small comment improvement

* Added two API tests and modified existing test.

* Unrelated code which was commented out is run again.

* Add API test.

* Simplified API test.

* Merge API tests.

* Add API contract checks for corner radius.

* Ensure that listview selected index is not outside of the item range.

* Update TabView.cpp

Co-authored-by: Ranjesh <[email protected]>
  • Loading branch information
@Felix-Dev @ranjeshj
Felix-Dev and ranjeshj authored Nov 13, 2020
1 parent aeac5a1 commit 309c88f
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 52 deletions.
137 changes: 101 additions & 36 deletions dev/TabView/APITests/TabViewTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
using Windows.UI.Xaml.Automation.Peers;
using Windows.UI.Xaml.Automation;
using Windows.UI.Xaml.Automation.Provider;
using System.Collections.ObjectModel;
using Microsoft.UI.Xaml.Controls.Primitives;

#if USING_TAEF
using WEX.TestExecution;
Expand All @@ -31,60 +33,99 @@ namespace Windows.UI.Xaml.Tests.MUXControls.ApiTests
[TestClass]
public class TabViewTests : ApiTestBase
{
[TestMethod]
public void VerifyCompactTabWidthVisualStates_ItemsMode()
{
VerifyCompactTabWidthVisualStates();
}

[TestMethod]
public void VerifyCompactTabWidthVisualStates()
public void VerifyCompactTabWidthVisualStates_ItemsSourceMode()
{
VerifyCompactTabWidthVisualStates(isItemsSourceMode: true);
}

private void VerifyCompactTabWidthVisualStates(bool isItemsSourceMode = false)
{
TabView tabView = null;
RunOnUIThread.Execute(() =>
{
tabView = new TabView();
Content = tabView;
SetupTabViewItems();

tabView.TabItems.Add(CreateTabViewItem("Item 0", Symbol.Add));
tabView.TabItems.Add(CreateTabViewItem("Item 1", Symbol.AddFriend));
tabView.TabItems.Add(CreateTabViewItem("Item 2"));
Log.Comment("Set TabWidthMode to compact");
tabView.TabWidthMode = TabViewWidthMode.Compact;

Log.Comment("Select a tab. In TabWidthMode compact, a selected tab will not be in compact state. We verify that behavior in this test");
tabView.SelectedIndex = 0;
tabView.SelectedItem = tabView.TabItems[0];
(tabView.SelectedItem as TabViewItem).IsSelected = true;
Verify.AreEqual("Item 0", (tabView.SelectedItem as TabViewItem).Header);
Content.UpdateLayout();
});
// Waiting for layout
IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
// Now set tab width mode
tabView.TabWidthMode = TabViewWidthMode.Compact;
Content = tabView;
});

IdleSynchronizer.Wait();

// Check if switching to compact updates all items correctly
RunOnUIThread.Execute(() =>
{
VerifyTabWidthVisualStates(tabView.TabItems, true);
tabView.TabItems.Add(CreateTabViewItem("Item 3"));
});
Log.Comment("Verify a selected tab exists");
VerifySelectedItem("Tab 0");

IdleSynchronizer.Wait();
Log.Comment("Verify the created TabView displays every tab in compact mode");
VerifyTabWidthVisualStates(tabView, tabView.TabItems, true);

// Check if a newly added item has correct visual states
RunOnUIThread.Execute(() =>
{
VerifyTabWidthVisualStates(tabView.TabItems, true);
Log.Comment("Verify that adding a new item creates a tab in compact mode");
AddItem("Tab 3");
Content.UpdateLayout();

VerifyTabWidthVisualStates(tabView, new List<object>() { tabView.TabItems[tabView.TabItems.Count - 1] }, true);

Log.Comment("Change the TabWidthMode to non compact and verify that every tab is no longer in compact mode");
tabView.TabWidthMode = TabViewWidthMode.Equal;
Content.UpdateLayout();

VerifyTabWidthVisualStates(tabView, tabView.TabItems, false);

Log.Comment("Change the TabWidthMode to compact and verify that every tab is now in compact mode");
tabView.TabWidthMode = TabViewWidthMode.Compact;
Content.UpdateLayout();

VerifyTabWidthVisualStates(tabView, tabView.TabItems, true);
});

IdleSynchronizer.Wait();
void SetupTabViewItems()
{
if (isItemsSourceMode)
{
var tabItemsSource = new ObservableCollection<string>() { "Tab 0", "Tab 1", "Tab 2" };
tabView.TabItemsSource = tabItemsSource;
}
else
{
tabView.TabItems.Add(CreateTabViewItem("Tab 0", Symbol.Add));
tabView.TabItems.Add(CreateTabViewItem("Tab 1", Symbol.AddFriend));
tabView.TabItems.Add(CreateTabViewItem("Tab 2"));
}
}

// Switch back to non compact and check if every item has the correct visual state
RunOnUIThread.Execute(() =>
void VerifySelectedItem(string expectedHeader)
{
VerifyTabWidthVisualStates(tabView.TabItems, false);
});
object selectedItemHeader = isItemsSourceMode
? tabView.SelectedItem
: (tabView.SelectedItem as TabViewItem).Header;

Verify.AreEqual(expectedHeader, selectedItemHeader);
}

void AddItem(string header)
{
if (isItemsSourceMode)
{
((ObservableCollection<string>)tabView.TabItemsSource).Add(header);
}
else
{
tabView.TabItems.Add(CreateTabViewItem(header));
}
}
}

[TestMethod]
Expand Down Expand Up @@ -142,15 +183,15 @@ public void VerifyTabViewItemUIABehavior()
RunOnUIThread.Execute(() =>
{
var selectionItemProvider = GetProviderFromTVI(tvi0);
Verify.IsTrue(selectionItemProvider.IsSelected,"Item should be selected");
Verify.IsTrue(selectionItemProvider.IsSelected, "Item should be selected");

selectionItemProvider = GetProviderFromTVI(tvi1);
Verify.IsFalse(selectionItemProvider.IsSelected, "Item should not be selected");

Log.Comment("Change selection through automationpeer");
selectionItemProvider.Select();
Verify.IsTrue(selectionItemProvider.IsSelected, "Item should have been selected");

selectionItemProvider = GetProviderFromTVI(tvi0);
Verify.IsFalse(selectionItemProvider.IsSelected, "Item should not be selected anymore");

Expand All @@ -167,18 +208,43 @@ static ISelectionItemProvider GetProviderFromTVI(TabViewItem item)
}
}

private static void VerifyTabWidthVisualStates(IList<object> items, bool isCompact)
[TestMethod]
public void VerifyTabViewWithoutTabsDoesNotCrash()
{
RunOnUIThread.Execute(() =>
{
TabView tabView = new TabView();
Content = tabView;

// Creating a TabView without tabs should not crash the app.
Content.UpdateLayout();

var tabItemsSource = new ObservableCollection<string>() { "Tab 1", "Tab 2" };
tabView.TabItemsSource = tabItemsSource;

// Clearing the ItemsSource should not crash the app.
Log.Comment("Clear the specified tab items source");
tabItemsSource.Clear();
});
}

private static void VerifyTabWidthVisualStates(TabView tabView, IList<object> items, bool isCompact)
{
var listView = VisualTreeUtils.FindVisualChildByName(tabView, "TabListView") as TabViewListView;

foreach (var item in items)
{
var tabItem = item as TabViewItem;
var tabItem = item is TabViewItem
? (TabViewItem)item
: listView.ContainerFromItem(item) as TabViewItem;

var rootGrid = VisualTreeHelper.GetChild(tabItem, 0) as FrameworkElement;

foreach (var group in VisualStateManager.GetVisualStateGroups(rootGrid))
{
if (group.Name == "TabWidthModes")
{
if(tabItem.IsSelected || !isCompact)
if (tabItem.IsSelected || !isCompact)
{
Verify.AreEqual("StandardWidth", group.CurrentState.Name, "Verify that this tab item is rendering in standard width");
}
Expand All @@ -188,7 +254,6 @@ private static void VerifyTabWidthVisualStates(IList<object> items, bool isCompa
}
}
}

}
}

Expand Down
12 changes: 6 additions & 6 deletions dev/TabView/TabView.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,11 +635,6 @@ void TabView::OnItemsChanged(winrt::IInspectable const& item)
}
else
{
if (const auto newItem = TabItems().GetAt(args.Index()).try_as<TabViewItem>())
{
newItem->OnTabViewWidthModeChanged(TabWidthMode());
newItem->SetParentTabView(*this);
}
UpdateTabWidths();
}
}
Expand Down Expand Up @@ -1003,7 +998,12 @@ void TabView::UpdateSelectedIndex()
{
if (auto&& listView = m_listView.get())
{
listView.SelectedIndex(SelectedIndex());
const auto selectedIndex = SelectedIndex();
// Ensure that the selected index is within range of the items
if (selectedIndex < static_cast<int>(listView.Items().Size()))
{
listView.SelectedIndex(selectedIndex);
}
}
}

Expand Down
18 changes: 13 additions & 5 deletions dev/TabView/TabView.xaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
xmlns:contract4Present="http://schemas.microsoft.com/winfx/2006/xaml/presentation?IsApiContractPresent(Windows.Foundation.UniversalApiContract,4)"
xmlns:contract6Present="http://schemas.microsoft.com/winfx/2006/xaml/presentation?IsApiContractPresent(Windows.Foundation.UniversalApiContract,6)"
xmlns:contract7Present="http://schemas.microsoft.com/winfx/2006/xaml/presentation?IsApiContractPresent(Windows.Foundation.UniversalApiContract,7)"
xmlns:contract7NotPresent="http://schemas.microsoft.com/winfx/2006/xaml/presentation?IsApiContractNotPresent(Windows.Foundation.UniversalApiContract,7)"
xmlns:local="using:Microsoft.UI.Xaml.Controls"
xmlns:primitives="using:Microsoft.UI.Xaml.Controls.Primitives">

Expand Down Expand Up @@ -133,7 +134,12 @@
<Setter Property="Template">
<Setter.Value>
<ControlTemplate TargetType="primitives:TabViewListView">
<Border BorderBrush="{TemplateBinding BorderBrush}" Background="{TemplateBinding Background}" BorderThickness="{TemplateBinding BorderThickness}" CornerRadius="{TemplateBinding CornerRadius}">
<Border
BorderBrush="{TemplateBinding BorderBrush}"
Background="{TemplateBinding Background}"
BorderThickness="{TemplateBinding BorderThickness}"
contract7Present:CornerRadius="{TemplateBinding CornerRadius}"
contract7NotPresent:CornerRadius="{ThemeResource ControlCornerRadius}">
<ScrollViewer x:Name="ScrollViewer"
Grid.Column="1"
AutomationProperties.AccessibilityView="Raw"
Expand Down Expand Up @@ -230,7 +236,7 @@
<Setter Property="FontFamily" Value="Segoe MDL2 Assets"/>
<Setter Property="FontWeight" Value="Normal"/>
<Setter Property="FontSize" Value="{ThemeResource ControlContentThemeFontSize}"/>
<Setter Property="CornerRadius" Value="{ThemeResource ControlCornerRadius}"/>
<contract7Present:Setter Property="CornerRadius" Value="{ThemeResource ControlCornerRadius}"/>
<Setter Property="UseSystemFocusVisuals" Value="{StaticResource UseSystemFocusVisuals}"/>
<Setter Property="FocusVisualMargin" Value="-3"/>
<Setter Property="Template">
Expand All @@ -244,7 +250,8 @@
BorderBrush="{TemplateBinding BorderBrush}"
ContentTemplate="{TemplateBinding ContentTemplate}"
Content="{TemplateBinding Content}"
CornerRadius="{TemplateBinding CornerRadius}"
contract7Present:CornerRadius="{TemplateBinding CornerRadius}"
contract7NotPresent:CornerRadius="{ThemeResource ControlCornerRadius}"
ContentTransitions="{TemplateBinding ContentTransitions}"
HorizontalContentAlignment="{TemplateBinding HorizontalContentAlignment}"
Padding="{TemplateBinding Padding}"
Expand Down Expand Up @@ -290,7 +297,7 @@
<Style x:Name="TabViewButtonStyle" TargetType="Button">
<Setter Property="Background" Value="{ThemeResource TabViewButtonBackground}"/>
<Setter Property="Foreground" Value="{ThemeResource TabViewButtonForeground}"/>
<Setter Property="CornerRadius" Value="{Binding Source={ThemeResource OverlayCornerRadius}, Converter={StaticResource TopCornerRadiusFilterConverter}}"/>
<contract7Present:Setter Property="CornerRadius" Value="{Binding Source={ThemeResource OverlayCornerRadius}, Converter={StaticResource TopCornerRadiusFilterConverter}}"/>
<Setter Property="FontSize" Value="11"/>
<Setter Property="FontFamily" Value="Segoe MDL2 Assets"/>
<Setter Property="VerticalAlignment" Value="Bottom"/>
Expand All @@ -305,7 +312,8 @@
Content="{TemplateBinding Content}"
ContentTemplate="{TemplateBinding ContentTemplate}"
ContentTransitions="{TemplateBinding ContentTransitions}"
CornerRadius="{TemplateBinding CornerRadius}"
contract7Present:CornerRadius="{TemplateBinding CornerRadius}"
contract7NotPresent:CornerRadius="{ThemeResource ControlCornerRadius}"
FontSize="{TemplateBinding FontSize}"
FontFamily="{TemplateBinding FontFamily}"
FontWeight="SemiLight"
Expand Down
32 changes: 27 additions & 5 deletions dev/TabView/TabViewListView.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ winrt::DependencyObject TabViewListView::GetContainerForItemOverride()
bool TabViewListView::IsItemItsOwnContainerOverride(winrt::IInspectable const& args)
{
bool isItemItsOwnContainer = false;
if (auto item = args.try_as<winrt::TabViewItem>())
if (const auto item = args.try_as<winrt::TabViewItem>())
{
isItemItsOwnContainer = static_cast<bool>(item);
}
Expand All @@ -40,18 +40,40 @@ void TabViewListView::OnItemsChanged(winrt::IInspectable const& item)
{
__super::OnItemsChanged(item);

if (auto tabView = SharedHelpers::GetAncestorOfType<winrt::TabView>(winrt::VisualTreeHelper::GetParent(*this)))
if (const auto tabView = SharedHelpers::GetAncestorOfType<winrt::TabView>(winrt::VisualTreeHelper::GetParent(*this)))
{
auto internalTabView = winrt::get_self<TabView>(tabView);
const auto internalTabView = winrt::get_self<TabView>(tabView);
internalTabView->OnItemsChanged(item);
}
}

void TabViewListView::PrepareContainerForItemOverride(const winrt::DependencyObject& element, const winrt::IInspectable& item)
{
const auto itemContainer = element.as<winrt::TabViewItem>();
const auto tvi = winrt::get_self<TabViewItem>(itemContainer);

// Due to virtualization, a TabViewItem might be recycled to display a different tab data item.
// In that case, there is no need to set the TabWidthMode of the TabViewItem or its parent TabView
// as they are already set correctly here.
//
// We know we are currently looking at a TabViewItem being recycled if its parent TabView has already been set.
if (!tvi->GetParentTabView())
{
if (const auto tabView = SharedHelpers::GetAncestorOfType<winrt::TabView>(winrt::VisualTreeHelper::GetParent(*this)))
{
tvi->OnTabViewWidthModeChanged(tabView.TabWidthMode());
tvi->SetParentTabView(tabView);
}
}

__super::PrepareContainerForItemOverride(element, item);
}

void TabViewListView::OnContainerContentChanging(const winrt::IInspectable& sender, const winrt::Windows::UI::Xaml::Controls::ContainerContentChangingEventArgs& args)
{
if (auto tabView = SharedHelpers::GetAncestorOfType<winrt::TabView>(winrt::VisualTreeHelper::GetParent(*this)))
if (const auto tabView = SharedHelpers::GetAncestorOfType<winrt::TabView>(winrt::VisualTreeHelper::GetParent(*this)))
{
auto internalTabView = winrt::get_self<TabView>(tabView);
const auto internalTabView = winrt::get_self<TabView>(tabView);
internalTabView->UpdateTabContent();
}
}
1 change: 1 addition & 0 deletions dev/TabView/TabViewListView.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class TabViewListView :
winrt::DependencyObject GetContainerForItemOverride();
bool IsItemItsOwnContainerOverride(winrt::IInspectable const& item);
void OnItemsChanged(winrt::IInspectable const& item);
void PrepareContainerForItemOverride(const winrt::DependencyObject& element, const winrt::IInspectable& item);

private:
void OnContainerContentChanging(const winrt::IInspectable& sender, const winrt::ContainerContentChangingEventArgs& args);
Expand Down

0 comments on commit 309c88f

Please sign in to comment.