From 3c1ddbbad64935200d42628612b33e617a4a662b Mon Sep 17 00:00:00 2001
From: Marcel Wagner <marcel.alex.wagner@outlook.com>
Date: Mon, 21 Aug 2023 20:48:05 +0200
Subject: [PATCH] Update MenuBarItem behavior to skip disabled items in
 keyboard navigation (#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>
---
 dev/MenuBar/MenuBarItem.cpp                   | 32 ++++++++++++-
 dev/MenuBar/MenuBarItem.h                     |  2 +
 .../MenuBar_InteractionTests/MenuBarTests.cs  | 45 ++++++++++++++++++-
 dev/MenuBar/MenuBar_TestUI/MenuBarPage.xaml   | 39 ++++++++++++++++
 4 files changed, 115 insertions(+), 3 deletions(-)

diff --git a/dev/MenuBar/MenuBarItem.cpp b/dev/MenuBar/MenuBarItem.cpp
index 762ab3abfc..184a52187b 100755
--- 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
index 7dd3108306..886a43bba6 100755
--- 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
index 5c88dea610..a4864f2e5f 100644
--- 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
index 2cb9359908..26974c2c55 100644
--- 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">