Update MenuBarItem behavior to skip disabled items in keyboard naviga…

…tion (#3555) * Add failing test * Fix bug, update test * Update comments * Add check for non tabstop items * Switch to better implementation by Austin * Update naming --------- Co-authored-by: Barbara Kudiess <bakudies@microsoft.com>
microsoft · Aug 21, 2023 · 3c1ddbb · 3c1ddbb
1 parent c7894a5
commit 3c1ddbb
Show file tree

Hide file tree

Showing 4 changed files with 115 additions and 3 deletions.
diff --git a/dev/MenuBar/MenuBarItem.cpp b/dev/MenuBar/MenuBarItem.cpp
@@ -293,13 +293,41 @@ void MenuBarItem::OpenFlyoutFrom(FlyoutLocation location)
         CloseMenuFlyout();
         if (location == FlyoutLocation::Left)
         {
-            winrt::get_self<MenuBarItem>(menuBar.Items().GetAt(((index - 1) + menuBar.Items().Size()) % menuBar.Items().Size()))->ShowMenuFlyout();
+            if (const auto item = FocusAndReturnNextFocusableItem(index, -1).try_as<MenuBarItem>())
+            {
+                item->ShowMenuFlyout();
+            }
         }
         else
         {
-            winrt::get_self<MenuBarItem>(menuBar.Items().GetAt((index + 1) % menuBar.Items().Size()))->ShowMenuFlyout();
+            if (const auto item = FocusAndReturnNextFocusableItem(index, +1).try_as<MenuBarItem>())
+            {
+                item->ShowMenuFlyout();
+            }
+        }
+    }
+}
+
+winrt::MenuBarItem MenuBarItem::FocusAndReturnNextFocusableItem(int index, int direction)
+{
+    if (const auto menuBar = m_menuBar.get())
+    {
+        const int itemsCount = menuBar.Items().Size();
+        // index + direction might be negative so account for that by adding itemsCount.
+        int itemIndex = (index + direction + itemsCount) % itemsCount;
+        while (!menuBar.Items().GetAt(itemIndex).Focus(winrt::FocusState::Programmatic))
+        {
+            // index + direction might be negative so account for that by adding itemsCount.
+            itemIndex = (itemIndex + direction + itemsCount) % itemsCount;
+            if (itemIndex == index)
+            {
+                // We are where we started, return no new item.
+                return nullptr;
+            }
         }
+        return menuBar.Items().GetAt(itemIndex);
     }
+    return nullptr;
 }
 
 void MenuBarItem::MoveFocusTo(FlyoutLocation location)

diff --git a/dev/MenuBar/MenuBarItem.h b/dev/MenuBar/MenuBarItem.h
@@ -41,6 +41,8 @@ class MenuBarItem :
     void OpenFlyoutFrom(FlyoutLocation location);
     void MoveFocusTo(FlyoutLocation location);
 
+    winrt::MenuBarItem FocusAndReturnNextFocusableItem(int index, int direction);
+
     void OnVisualPropertyChanged(const winrt::DependencyObject& sender, const winrt::DependencyProperty& args);
     void UpdateVisualStates();
 

diff --git a/dev/MenuBar/MenuBar_InteractionTests/MenuBarTests.cs b/dev/MenuBar/MenuBar_InteractionTests/MenuBarTests.cs
@@ -186,7 +186,50 @@ public void KeyboardNavigationWithArrowKeysTest()
                 }
             }
         }
-
+
+        [TestMethod]
+        public void KeyboardNavigationWithArrowKeysWithDisabledItemTest()
+        {
+            using (var setup = new TestSetupHelper("MenuBar Tests"))
+            {
+                var editButton = FindElement.ById<Button>("MenuBarPartiallyEnabledOne");
+                editButton.Invoke();
+                VerifyElement.Found("PartiallyEnabledFlyoutOne", FindBy.Name);
+
+                KeyboardHelper.PressKey(Key.Right);
+                VerifyElement.Found("PartiallyEnabledFlyoutThree", FindBy.Name);
+
+                KeyboardHelper.PressKey(Key.Left);
+                VerifyElement.Found("PartiallyEnabledFlyoutOne", FindBy.Name);
+
+                KeyboardHelper.PressKey(Key.Left);
+                VerifyElement.Found("PartiallyEnabledFlyoutThree", FindBy.Name);
+
+                KeyboardHelper.PressKey(Key.Left);
+                VerifyElement.Found("PartiallyEnabledFlyoutOne", FindBy.Name);
+
+                KeyboardHelper.PressKey(Key.Right);
+                VerifyElement.Found("PartiallyEnabledFlyoutThree", FindBy.Name);
+
+                KeyboardHelper.PressKey(Key.Right);
+                VerifyElement.Found("PartiallyEnabledFlyoutOne", FindBy.Name);
+            }
+        }
+
+        [TestMethod]
+        public void KeyboardNavigationWithArrowKeysWithOnlyOneItemWorks()
+        {
+            using (var setup = new TestSetupHelper("MenuBar Tests"))
+            {
+                var editButton = FindElement.ById<Button>("LoopTestBarOne");
+                editButton.Invoke();
+                VerifyElement.Found("LoopTestItemOne", FindBy.Name);
+
+                KeyboardHelper.PressKey(Key.Right);
+                VerifyElement.NotFound("LoopTestItemOne", FindBy.Name);
+            }
+        }
+
         [TestMethod]
         [TestProperty("Ignore", "True")]
         // Disabled due to: MenuBarTests.KeyboardNavigationWithAccessKeysTest unreliable #135

diff --git a/dev/MenuBar/MenuBar_TestUI/MenuBarPage.xaml b/dev/MenuBar/MenuBar_TestUI/MenuBarPage.xaml
@@ -70,6 +70,45 @@
 
             <TextBlock Text="Initially empty menubar" Margin="0,6,0,0"/>
             <muxc:MenuBar x:Name="EmptyMenuBar"></muxc:MenuBar>
+
+            <TextBlock Text="Disabled items test"/>
+            <StackPanel Orientation="Horizontal">
+                <muxc:MenuBar>
+                    <muxc:MenuBarItem x:Name="MenuBarPartiallyEnabledOne" AutomationProperties.Name="MenuBarPartiallyEnabledOne" Title="MenuBarPartiallyEnabledOne">
+                        <MenuFlyoutItem AutomationProperties.Name="PartiallyEnabledFlyoutOne" Text="PartiallyEnabledFlyoutOne"/>
+                        <MenuFlyoutItem Text="Open..."/>
+                        <MenuFlyoutItem Text="Save"/>
+                        <MenuFlyoutItem Text="Exit"/>
+                    </muxc:MenuBarItem>
+
+                    <muxc:MenuBarItem Title="Edit" IsEnabled="false">
+                        <MenuFlyoutItem AutomationProperties.Name="PartiallyEnabledFlyoutTwo" Text="PartiallyEnabledFlyoutTwo"/>
+                        <MenuFlyoutItem Text="Cut"/>
+                        <MenuFlyoutItem Text="Copy"/>
+                        <MenuFlyoutItem Text="Paste"/>
+                    </muxc:MenuBarItem>
+
+                    <muxc:MenuBarItem Title="Help">
+                        <MenuFlyoutItem AutomationProperties.Name="PartiallyEnabledFlyoutThree" Text="PartiallyEnabledFlyoutThree"/>
+                        <MenuFlyoutItem Text="About"/>
+                    </muxc:MenuBarItem>
+                </muxc:MenuBar>
+                <muxc:MenuBar>
+                    <muxc:MenuBarItem x:Name="LoopTestBarOne" AutomationProperties.Name="LoopTestBarOne" Title="LoopTestBarOne">
+                        <MenuFlyoutItem AutomationProperties.Name="LoopTestItemOne" Text="PartiallyEnabledFlyoutOne"/>
+                        <MenuFlyoutItem Text="Open..."/>
+                        <MenuFlyoutItem Text="Save"/>
+                        <MenuFlyoutItem Text="Exit"/>
+                    </muxc:MenuBarItem>
+
+                    <muxc:MenuBarItem Title="Edit" IsEnabled="false">
+                        <MenuFlyoutItem AutomationProperties.Name="PartiallyEnabledFlyoutTwo" Text="PartiallyEnabledFlyoutTwo"/>
+                        <MenuFlyoutItem Text="Cut"/>
+                        <MenuFlyoutItem Text="Copy"/>
+                        <MenuFlyoutItem Text="Paste"/>
+                    </muxc:MenuBarItem>
+                </muxc:MenuBar>
+            </StackPanel>
         </StackPanel>
         <TextBlock Grid.Row="1" Text="Options" Style="{ThemeResource StandardGroupHeader}"/>
         <StackPanel Margin="0,20,0,20" VerticalAlignment="Top"  Grid.Row="2">