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

Add TabView SelectionPattern #2856

Merged
merged 9 commits into from
Jul 20, 2020
Merged

Add TabView SelectionPattern #2856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d90f159
Add TabView SelectionPattern
marcelwgn Jul 7, 2020
9762b08
Add logic for accessing parent tabview
marcelwgn Jul 7, 2020
342e76f
Add test
marcelwgn Jul 7, 2020
643f397
Implement SelectionItem pattern and provider
marcelwgn Jul 7, 2020
2363226
Merge branch 'master' into tabview-selection-pattern
marcelwgn Jul 8, 2020
5da4a61
Merge branch 'master' into tabview-selection-pattern
marcelwgn Jul 8, 2020
a2d6120
Remove parent tabview getter
marcelwgn Jul 12, 2020
d730b1f
Fix missing SelectionContainer
marcelwgn Jul 13, 2020
a852497
Fix compile time error and add nullcheck for SelectionContainer
marcelwgn Jul 17, 2020
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
85 changes: 84 additions & 1 deletion dev/TabView/APITests/TabViewTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

using MUXControlsTestApp.Utilities;
Expand All @@ -10,6 +10,9 @@
using Common;
using Microsoft.UI.Xaml.Controls;
using System.Collections.Generic;
using Windows.UI.Xaml.Automation.Peers;
using Windows.UI.Xaml.Automation;
using Windows.UI.Xaml.Automation.Provider;

#if USING_TAEF
using WEX.TestExecution;
Expand Down Expand Up @@ -84,6 +87,86 @@ public void VerifyCompactTabWidthVisualStates()
});
}

[TestMethod]
public void VerifyTabViewUIABehavior()
{
RunOnUIThread.Execute(() =>
{
TabView tabView = new TabView();
Content = tabView;

tabView.TabItems.Add(CreateTabViewItem("Item 0", Symbol.Add));
tabView.TabItems.Add(CreateTabViewItem("Item 1", Symbol.AddFriend));
tabView.TabItems.Add(CreateTabViewItem("Item 2"));

Content.UpdateLayout();

var tabViewPeer = FrameworkElementAutomationPeer.CreatePeerForElement(tabView);
Verify.IsNotNull(tabViewPeer);
var tabViewSelectionPattern = tabViewPeer.GetPattern(PatternInterface.Selection);
Verify.IsNotNull(tabViewSelectionPattern);
var selectionProvider = tabViewSelectionPattern as ISelectionProvider;
// Tab controls must require selection
Verify.IsTrue(selectionProvider.IsSelectionRequired);
});
}

[TestMethod]
public void VerifyTabViewItemUIABehavior()
{
TabView tabView = null;

TabViewItem tvi0 = null;
TabViewItem tvi1 = null;
TabViewItem tvi2 = null;
RunOnUIThread.Execute(() =>
{
tabView = new TabView();
Content = tabView;

tvi0 = CreateTabViewItem("Item 0", Symbol.Add);
tvi1 = CreateTabViewItem("Item 1", Symbol.AddFriend);
tvi2 = CreateTabViewItem("Item 2");

tabView.TabItems.Add(tvi0);
tabView.TabItems.Add(tvi1);
tabView.TabItems.Add(tvi2);

tabView.SelectedIndex = 0;
tabView.SelectedItem = tvi0;
Content.UpdateLayout();
});

IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
var selectionItemProvider = GetProviderFromTVI(tvi0);
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");

Verify.IsNotNull(selectionItemProvider.SelectionContainer);
});

static ISelectionItemProvider GetProviderFromTVI(TabViewItem item)
{
var peer = FrameworkElementAutomationPeer.CreatePeerForElement(item);
var provider = peer.GetPattern(PatternInterface.SelectionItem)
as ISelectionItemProvider;
Verify.IsNotNull(provider);
return provider;
}
}

private static void VerifyTabWidthVisualStates(IList<object> items, bool isCompact)
{
foreach (var item in items)
Expand Down
3 changes: 2 additions & 1 deletion dev/TabView/TabView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

#include "pch.h"
Expand Down Expand Up @@ -638,6 +638,7 @@ void TabView::OnItemsChanged(winrt::IInspectable const& item)
if (const auto newItem = TabItems().GetAt(args.Index()).try_as<TabViewItem>())
{
newItem->OnTabViewWidthModeChanged(TabWidthMode());
newItem->SetParentTabView(*this);
}
UpdateTabWidths();
}
Expand Down
28 changes: 28 additions & 0 deletions dev/TabView/TabViewAutomationPeer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ TabViewAutomationPeer::TabViewAutomationPeer(winrt::TabView const& owner)
// IAutomationPeerOverrides
winrt::IInspectable TabViewAutomationPeer::GetPatternCore(winrt::PatternInterface const& patternInterface)
{
if (patternInterface == winrt::PatternInterface::Selection)
{
return *this;
}
return __super::GetPatternCore(patternInterface);
}

Expand All @@ -31,3 +35,27 @@ winrt::AutomationControlType TabViewAutomationPeer::GetAutomationControlTypeCore
return winrt::AutomationControlType::Tab;
}

bool TabViewAutomationPeer::CanSelectMultiple()
{
return false;
}

bool TabViewAutomationPeer::IsSelectionRequired()
{
return true;
}

winrt::com_array<winrt::Windows::UI::Xaml::Automation::Provider::IRawElementProviderSimple> TabViewAutomationPeer::GetSelection()
{
if (auto tabView = Owner().try_as<TabView>())
{
if (auto tabViewItem = tabView->ContainerFromIndex(tabView->SelectedIndex()).try_as<winrt::TabViewItem>())
{
if (auto peer = winrt::FrameworkElementAutomationPeer::CreatePeerForElement(tabViewItem))
{
return { ProviderFromPeer(peer) };
}
}
}
return {};
}
9 changes: 8 additions & 1 deletion dev/TabView/TabViewAutomationPeer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
#include "TabViewAutomationPeer.g.h"

class TabViewAutomationPeer :
public ReferenceTracker<TabViewAutomationPeer, winrt::implementation::TabViewAutomationPeerT>
public ReferenceTracker<TabViewAutomationPeer,
winrt::implementation::TabViewAutomationPeerT,
winrt::ISelectionProvider>
{

public:
Expand All @@ -17,4 +19,9 @@ class TabViewAutomationPeer :
winrt::IInspectable GetPatternCore(winrt::PatternInterface const& patternInterface);
hstring GetClassNameCore();
winrt::AutomationControlType GetAutomationControlTypeCore();

// ISelectionProvider
bool CanSelectMultiple();
bool IsSelectionRequired();
winrt::com_array<winrt::Windows::UI::Xaml::Automation::Provider::IRawElementProviderSimple> GetSelection();
};
15 changes: 15 additions & 0 deletions dev/TabView/TabViewItem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ void TabViewItem::OnApplyTemplate()

void TabViewItem::OnIsSelectedPropertyChanged(const winrt::DependencyObject& sender, const winrt::DependencyProperty& args)
{
if (const auto peer = winrt::FrameworkElementAutomationPeer::CreatePeerForElement(*this))
{
peer.RaiseAutomationEvent(winrt::AutomationEvents::SelectionItemPatternOnElementSelected);
}

if (IsSelected())
{
SetValue(winrt::Canvas::ZIndexProperty(),box_value(20));
Expand Down Expand Up @@ -146,6 +151,16 @@ void TabViewItem::OnCloseButtonOverlayModeChanged(winrt::TabViewCloseButtonOverl
UpdateCloseButton();
}

winrt::TabView TabViewItem::GetParentTabView()
{
return m_parentTabView.get();
}

void TabViewItem::SetParentTabView(winrt::TabView const& tabView)
{
m_parentTabView = winrt::make_weak(tabView);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the fact that this is needed indicates that the components are architectured incorrectly. The TabViewItem shouldn't need to know about the Parent TabView. Instead I think that it might have been better to thave selection status held on the TabViewItem, which can raise Selection changed events that the TabView can use to manage the single selection. That being said I don't think we should change this now... maybe.

Copy link
Collaborator Author

@marcelwgn marcelwgn Jul 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That's a really good point. The question that comes to my mind now, is why the NavigationViewItem has an IsSelected property, while the TabViewItem does not. Both act more or less like a tab item, in both cases only a single item can be selected at a time. Was/is there a specific reasoning why those two diverge here?

Maybe @stmoy has an idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

TabViewItem does have an IsSelected property which is inherited from SelectorItem

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh missed that. The "Is selected" property doesn't appear in the official TabViewItem documentation. Also the TabViewItem/TabView do not care about that property at all.

Was this by design? I think @StephenLPeters is right here, that is something we should update. Currently only TabView knows about selection in contrast to NavigationView, where the items also know their selection status.

Copy link
Contributor

@Felix-Dev Felix-Dev Jul 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh missed that. The "Is selected" property doesn't appear in the official TabViewItem documentation. Also the TabViewItem/TabView do not care about that property at all.

@chingucoding The TabViewItem.IsSelected property does work though. See this XAML:

<muxc:TabView>
    <muxc:TabView.TabItems>
        <muxc:TabViewItem Header="Tab 1"/>
        <muxc:TabViewItem Header="Tab 2"/>
        <muxc:TabViewItem Header="Tab 3" IsSelected="True"/>
        <muxc:TabViewItem Header="Tab 4"/>
    </muxc:TabView.TabItems>
</muxc:TabView>

This gives me the expected result:
image

Copy link
Collaborator Author

@marcelwgn marcelwgn Jul 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Since IsSelected works here, I removed the "GetParentTabView" pattern/code.

Edit: Readded it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chigy and @YuliKl for the documentation question

Copy link
Contributor

Choose a reason for hiding this comment

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

@chingucoding would it makes sense to move the pointer to the parent tab view onto the TabViewItemAutomationPeer since the TabViewItem itself doesn't need it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But how would we set that property then? After all, we don't know when the AutomationPeer for a TabViewItem will be created, so TVI needs to "save" it until it will be needed. And since AutomationPeer are more or less "fire and forget" anyway (meaning they get created and can be used only once and get recreated for the next usage), the TVI AutomationPeer would always have to ask a TVI for it's parent TabView.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

}

void TabViewItem::OnTabViewWidthModeChanged(winrt::TabViewWidthMode const& mode)
{
m_tabViewWidthMode = mode;
Expand Down
5 changes: 5 additions & 0 deletions dev/TabView/TabViewItem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ class TabViewItem :
void OnTabViewWidthModeChanged(winrt::TabViewWidthMode const& mode);
void OnCloseButtonOverlayModeChanged(winrt::TabViewCloseButtonOverlayMode const& mode);

winrt::TabView GetParentTabView();
void SetParentTabView(winrt::TabView const& tabView);

private:
tracker_ref<winrt::Button> m_closeButton{ this };
tracker_ref<winrt::ToolTip> m_toolTip{ this };
Expand Down Expand Up @@ -72,4 +75,6 @@ class TabViewItem :

void UpdateShadow();
winrt::IInspectable m_shadow{ nullptr };

winrt::weak_ref<winrt::TabView> m_parentTabView{ nullptr };
};
62 changes: 61 additions & 1 deletion dev/TabView/TabViewItemAutomationPeer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@ TabViewItemAutomationPeer::TabViewItemAutomationPeer(winrt::TabViewItem const& o
}

// IAutomationPeerOverrides
winrt::IInspectable TabViewItemAutomationPeer::GetPatternCore(winrt::PatternInterface const& patternInterface)
{
if (patternInterface == winrt::PatternInterface::SelectionItem)
{
return *this;
}
return __super::GetPatternCore(patternInterface);
}

hstring TabViewItemAutomationPeer::GetClassNameCore()
{
return winrt::hstring_name_of<winrt::TabViewItem>();
Expand All @@ -27,7 +36,6 @@ winrt::AutomationControlType TabViewItemAutomationPeer::GetAutomationControlType
return winrt::AutomationControlType::TabItem;
}


winrt::hstring TabViewItemAutomationPeer::GetNameCore()
{
winrt::hstring returnHString = __super::GetNameCore();
Expand All @@ -43,3 +51,55 @@ winrt::hstring TabViewItemAutomationPeer::GetNameCore()

return returnHString;
}


bool TabViewItemAutomationPeer::IsSelected()
{
if (auto tvi = Owner().try_as<TabViewItem>())
{
return tvi->IsSelected();
}
return false;
}

winrt::IRawElementProviderSimple TabViewItemAutomationPeer::SelectionContainer()
{
if (const auto parent = GetParentTabView())
{
if (const auto peer = winrt::FrameworkElementAutomationPeer::CreatePeerForElement(parent))
{
return ProviderFromPeer(peer);
}
}
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to return the TabView's automation peer here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we need a reference to TabView here anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. We need to either pass it down to the item or look up the tree from the item in this case. That said, I'm not exactly sure how Narrator uses this method though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added the logic back, I added earlier in this PR. Walking up the tree seems a bit expensive here, though I am not a fan of having this relation between TabViewItem and TabView just for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@YuliKl is there someone we can ask about how Narrator uses this?

Copy link

@LukaszMakar LukaszMakar Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Narrator uses SelectionContainer for a few purposes:

  1. Listening to UIA events for the entire container.
  2. Understanding how selection behaves in the container and therefore which actions should be presented to the user. For instance, if the container is single selection + selection required, Narrator will not expose the action of removing an item from selection.
  3. Making announcements that inform the user about selection state of individual items. For instance, Narrator's "(non-)selected" for list items depends on whether the selection container supports multi-selection. Not quite your case but it does not mean it cannot become one.

Does that help?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think point 2 is a good reason here to not provide a nullptr for the SelectionContainer. After all TabView does not allow deselecting an item, and without the SelectionContainer being provided, Narrator wouldn't be able to hide that option from users.

Thank you for the information @LukaszMakar !

}

void TabViewItemAutomationPeer::AddToSelection()
{
Select();
}

void TabViewItemAutomationPeer::RemoveFromSelection()
{
// Can't unselect in a TabView without knowing next selection
}

void TabViewItemAutomationPeer::Select()
{
if (auto owner = Owner().try_as<TabViewItem>().get())
{
owner->IsSelected(true);
}
}

winrt::TabView TabViewItemAutomationPeer::GetParentTabView()
{
winrt::TabView parentTabView{ nullptr };

winrt::TabViewItem tabViewItem = Owner().try_as<winrt::TabViewItem>();
if (tabViewItem)
{
parentTabView = winrt::get_self<TabViewItem>(tabViewItem)->GetParentTabView();
}
return parentTabView;
}
15 changes: 14 additions & 1 deletion dev/TabView/TabViewItemAutomationPeer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,27 @@
#include "TabViewItemAutomationPeer.g.h"

class TabViewItemAutomationPeer :
public ReferenceTracker<TabViewItemAutomationPeer, winrt::implementation::TabViewItemAutomationPeerT>
public ReferenceTracker < TabViewItemAutomationPeer,
winrt::implementation::TabViewItemAutomationPeerT,
winrt::ISelectionItemProvider >
{

public:
TabViewItemAutomationPeer(winrt::TabViewItem const& owner);

// IAutomationPeerOverrides
winrt::IInspectable GetPatternCore(winrt::PatternInterface const& patternInterface);
winrt::hstring GetNameCore();
hstring GetClassNameCore();
winrt::AutomationControlType GetAutomationControlTypeCore();

// ISelectionItemProvider
bool IsSelected();
winrt::IRawElementProviderSimple SelectionContainer();
void AddToSelection();
void RemoveFromSelection();
void Select();

private:
winrt::TabView GetParentTabView();
};