Add TabView SelectionPattern #2856
Conversation
|
Implemented SelectionItem pattern on the TabViewItem now, added new API test for reading the IsSelected property and changing selection through the peer. Tested manually that changing selection also get's correctly announced in narrator. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Looks like last test run failed due to heap limit. Merged with master to get latest fix for that. |
Could you please sync again? Keith made another fix. |
Done, |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Seems that CI failed due to visual tree verification files: https://dev.azure.com/ms/microsoft-ui-xaml/_build/results?buildId=93164&view=logs&j=f1f05f7d-9051-5031-1c4e-31dcdce6d081&t=dc1e8999-6150-522b-4887-86903abea868 Is this because of the recent renaming of the folder? Or is it more of a "general" issue? Also why are there new AppBarButton files? @kaiguo FYI |
|
|
||
| void TabViewItem::SetParentTabView(winrt::TabView const& tabView) | ||
| { | ||
| m_parentTabView = winrt::make_weak(tabView); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
TabViewItem does have an IsSelected property which is inherited from SelectorItem
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Since IsSelected works here, I removed the "GetParentTabView" pattern/code.
Edit: Readded it.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
|
|
||
| winrt::IRawElementProviderSimple TabViewItemAutomationPeer::SelectionContainer() | ||
| { | ||
| return nullptr; |
There was a problem hiding this comment.
You will need to return the TabView's automation peer here.
There was a problem hiding this comment.
So we need a reference to TabView here anyway?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@YuliKl is there someone we can ask about how Narrator uses this?
There was a problem hiding this comment.
Narrator uses SelectionContainer for a few purposes:
- Listening to UIA events for the entire container.
- 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. - 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?
There was a problem hiding this comment.
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 !
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
This PR adds the SelectionPattern to the TabViewAutomationPeer.
I am not using the internals ListView pattern since we plan to switch that out at some point, and then we would need to do all of that ourselves anyway.
Motivation and Context
Fixes #2847
How Has This Been Tested?
Add API test
Screenshots (if appropriate):
Open questions
Given that the TabView now has the SelectionPattern, should the TabViewItems also have the SelectionItemPattern?
There is also a way to run Accessibility Insights as a test through the Axe package. Maybe we could leverage that to prevent those errors in the future? Given that the MUXControlsTestApp is a bit special compared to a regular app, maybe this could be done with the XCG, after all it contains a lot of samples for the controls.
@stmoy FYI