Adding accessible context menus to TabView pages #1664
Conversation
|
@llongley FYI, looks like a conflict popped up |
llongley
force-pushed
the
user/llongley/TabViewAccessibility
branch
from
c340efc to
d3dd80d
Compare
|
/azp run |
|
/azp run |
1 similar comment
|
/azp run |
|
@llongley FYI I fixed the conflicts and enabled Mica on the tabs demo |
|
/azp run |
|
Closing this PR for now since there is quite a lot of work needed to get this mergeable but this PR can of course be reopened if you wish to continue working on this. |
|
Hi @marcelwgn . Currently we are working on this PR and fixing the issues addressed. |
@sabareesanms FYI - since the opening of this PR there were many changes in terms of project/solution structure and where files now live. That's why there are so many conflicts. We are in a much better state now and we are not anticipating any other big changes like that at this point! |
sabareesanms
force-pushed
the
user/llongley/TabViewAccessibility
branch
from
5ae4656 to
89305ac
Compare
sabareesanms
dismissed
marcelwgn’s stale review
Merge with main changed file path
|
/azp run |
|
@marcelwgn we have already added checks for not allowing drag when there is only one tab exists. Can you give more details if you still see the issue? |
|
Can we have screenshots/videos of before and after fix for visualization? |
| { | ||
| if (tabView.TabItemsSource is IList itemsSourceList) | ||
| { | ||
| var item = itemsSourceList[index]; |
There was a problem hiding this comment.
Will index get copied over to lambda as it will be cleaned up from this method before the Click event actually happen?
|
|
||
| while (current != null) | ||
| { | ||
| if (current is T parent) |
There was a problem hiding this comment.
What's behavior of this function that we want (not current behavior) when current DO is of type T and it has a parent of type T?
There was a problem hiding this comment.
I think it is reasonable to return the first result, otherwise you would need to scan up to the root to find the latest T
|
|
||
| public static readonly DependencyProperty HeaderProperty = DependencyProperty.Register("Header", typeof(string), typeof(TabItemData), new PropertyMetadata("")); | ||
| public static readonly DependencyProperty ContentProperty = DependencyProperty.Register("Content", typeof(string), typeof(TabItemData), new PropertyMetadata("")); | ||
| public static readonly DependencyProperty IsInProgressProperty = DependencyProperty.Register("Content", typeof(bool), typeof(TabItemData), new PropertyMetadata(false)); |
| @@ -13,7 +13,7 @@ | |||
| <TextBlock Text="{Binding}" Style="{ThemeResource TitleTextBlockStyle}" /> | |||
| <TextBlock Text="Drag the Tab outside of the window to spawn a new window." Style="{ThemeResource SubtitleTextBlockStyle}" /> | |||
| <TextBlock Text="Notice that the state of the Tab is maintained in the new window. For example, if you toggle the ToggleSwitch ON, it will remain ON in the new window." Style="{ThemeResource BodyTextBlockStyle}" /> | |||
| <ToggleSwitch x:Name="ControlToggle" Header="Turn on ProgressRing" Margin="0,8" /> | |||
| <ProgressRing IsActive="{x:Bind ControlToggle.IsOn, Mode=OneWay}" HorizontalAlignment="Left" /> | |||
| <ToggleSwitch x:Name="ControlToggle" Header="Turn on ProgressRing" Margin="0,8" IsOn="{x:Bind IsInProgress, Mode=TwoWay}" /> | |||
There was a problem hiding this comment.
I am unclear on why this change is being made & net impact on UX. Can you explain or add screenshot in PR Description.
|
|
||
| currentWindow.Closed += (s, args) => | ||
| { | ||
| windowList.Remove(currentWindow); |
There was a problem hiding this comment.
Don't we have to unregister the Closed event as well from currentWindow?
| return tabView; | ||
| } | ||
| source.Remove((TabItemData)tabItemData); | ||
| destination.Insert(index, (TabItemData)tabItemData); |
There was a problem hiding this comment.
@sabareesanms This is where the app is crashing for me, trying to insert at index 1 while destination is empty. The repro is to move the last tab of a window out and you should be able to this the crash.
| TabTearOutRequested="Tabs_TabTearOutRequested" | ||
| TabTearOutWindowRequested="Tabs_TabTearOutWindowRequested"> | ||
| <TabView | ||
| x:Name="Tabs" |
There was a problem hiding this comment.
Can you fix indentation and why order of event subscription being changed for all the items (only diff seems addition of TabItemsSource)?
| private void TabViewContextMenu_Opening(object sender, object e) | ||
| { | ||
| // The contents of the context menu depends on the state of the application, so we'll build it dynamically. | ||
| MenuFlyout contextMenu = (MenuFlyout)sender; |
There was a problem hiding this comment.
Is it safe to assume the sender is always a MenuFlyout?
There was a problem hiding this comment.
Even if it is unlikely not to be, I would rather use as and return if it is in fact not a MenuFlyout instead of having the potential to crash here.
| { | ||
| contextMenu.Items.Clear(); | ||
|
|
||
| var item = (TabViewItem)contextMenu.Target; |
There was a problem hiding this comment.
Should we check type first before typecasting?
In order for TabView pages to be sufficiently accessible, there needs to be a way for users to rearrange tabs using a single pointer input without the need for precise manipulation. To achieve this, I've added context menus for each TabView that allows users to move tabs left and right, as well as moving tabs to other windows in the case of the tab tear-out example.
In working on this change, I found that there's a bug in WinUI 3 where moving a TabViewItem from one TabView in one window to another in another window retains the previous XamlRoot value, which throws an exception when we try to open a context menu on that TabViewItem, since it doesn't know what its correct XamlRoot is. I've filed a bug on that issue, but for now, to work around that, I've switched from using explicitly defined TabViewItems to using a data item source, which causes each TabView to generate its own TabViewItems.
Finally, I also found a bug in Win32WindowHelper - newWndProc and oldWndProc should not be static, because the class itself is instantiated on a per-window basis. Having these be static can result in an EngineExecutionException as a result of these delegates being garbage collected while there's still a native reference to them. Changing them to be instance fields fixes this issue.