From e135153c45cda52b0b65b931f3c86fe154f7725b Mon Sep 17 00:00:00 2001
From: Arjun Balgovind <32061677+arjunbalgovind@users.noreply.github.com>
Date: Fri, 18 Sep 2020 17:12:37 -0700
Subject: [PATCH] [Keyboard Manager] Fix focusable elements should have
 different names accessibility issue (#6672)

* Add listview

* Added row index to accessible names

* Cleanup rowIndex

* Fixed accessibility issue with ComboBox

* Updated comments
---
 .../keyboardmanager/dll/Resources.resx        |  7 ++--
 .../keyboardmanager/ui/KeyDropDownControl.cpp | 36 ++++++++++++-------
 .../keyboardmanager/ui/KeyDropDownControl.h   |  3 ++
 .../keyboardmanager/ui/ShortcutControl.cpp    | 34 ++++++++++++++----
 .../keyboardmanager/ui/ShortcutControl.h      |  6 +++-
 .../ui/SingleKeyRemapControl.cpp              | 24 +++++++++++--
 .../ui/SingleKeyRemapControl.h                |  4 +++
 7 files changed, 87 insertions(+), 27 deletions(-)

diff --git a/src/modules/keyboardmanager/dll/Resources.resx b/src/modules/keyboardmanager/dll/Resources.resx
index 42076f3092..2113b8b34e 100644
--- a/src/modules/keyboardmanager/dll/Resources.resx
+++ b/src/modules/keyboardmanager/dll/Resources.resx
@@ -276,10 +276,7 @@
   <data name="Delete_Remapping_Button" xml:space="preserve">
     <value>Delete Remapping</value>
   </data>
-  <data name="Remapped_To" xml:space="preserve">
-    <value>Remapped To</value>
-  </data>
-  <data name="Target_Application" xml:space="preserve">
-    <value>For Target Application </value>
+  <data name="AutomationProperties_Row" xml:space="preserve">
+    <value>Row </value>
   </data>
 </root>
\ No newline at end of file
diff --git a/src/modules/keyboardmanager/ui/KeyDropDownControl.cpp b/src/modules/keyboardmanager/ui/KeyDropDownControl.cpp
index 1a061169e4..9bd6568783 100644
--- a/src/modules/keyboardmanager/ui/KeyDropDownControl.cpp
+++ b/src/modules/keyboardmanager/ui/KeyDropDownControl.cpp
@@ -36,8 +36,15 @@ void KeyDropDownControl::SetDefaultProperties(bool isShortcut)
     // Attach flyout to the drop down
     warningFlyout.as<Flyout>().Content(warningMessage.as<TextBlock>());
     dropDown.as<ComboBox>().ContextFlyout().SetAttachedFlyout((FrameworkElement)dropDown.as<ComboBox>(), warningFlyout.as<Flyout>());
-    // To set the accessible name of the combo-box
-    dropDown.as<ComboBox>().SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_KEY_DROPDOWN_COMBOBOX)));
+    // To set the accessible name of the combo-box (by default index 1)
+    SetAccessibleNameForComboBox(dropDown.as<ComboBox>(), 1);
+}
+
+// Function to set accessible name for combobox
+void KeyDropDownControl::SetAccessibleNameForComboBox(ComboBox dropDown, int index)
+{
+    // Display name with drop down index (where this indexing will start from 1) - Used by narrator
+    dropDown.SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_KEY_DROPDOWN_COMBOBOX) + L" " + std::to_wstring(index)));
 }
 
 // Function to check if the layout has changed and accordingly update the drop down list
@@ -67,7 +74,8 @@ void KeyDropDownControl::SetSelectionHandler(Grid& table, StackPanel singleKeyCo
         bool indexFound = table.Children().IndexOf(singleKeyControl, controlIndex);
         if (indexFound)
         {
-            int rowIndex = (controlIndex - KeyboardManagerConstants::RemapTableHeaderCount) / KeyboardManagerConstants::RemapTableColCount;
+            // GetRow will give the row index including the table header
+            int rowIndex = table.GetRow(singleKeyControl) - 1;
 
             // Validate current remap selection
             KeyboardManagerHelper::ErrorType errorType = BufferValidationHelpers::ValidateAndUpdateKeyBufferElement(rowIndex, colIndex, selectedKeyIndex, keyCodeList, singleKeyRemapBuffer);
@@ -109,14 +117,8 @@ std::pair<KeyboardManagerHelper::ErrorType, int> KeyDropDownControl::ValidateSho
 
     if (controlIindexFound)
     {
-        if (isSingleKeyWindow)
-        {
-            rowIndex = (controlIndex - KeyboardManagerConstants::RemapTableHeaderCount) / KeyboardManagerConstants::RemapTableColCount;
-        }
-        else
-        {
-            rowIndex = (controlIndex - KeyboardManagerConstants::ShortcutTableHeaderCount) / KeyboardManagerConstants::ShortcutTableColCount;
-        }
+        // GetRow will give the row index including the table header
+        rowIndex = table.GetRow(shortcutControl) - 1;
 
         std::vector<int32_t> selectedIndices = GetSelectedIndicesFromStackPanel(parent);
 
@@ -136,7 +138,7 @@ std::pair<KeyboardManagerHelper::ErrorType, int> KeyDropDownControl::ValidateSho
         }
         else if (validationResult.second == BufferValidationHelpers::DropDownAction::ClearUnusedDropDowns)
         {
-            // remove all the drop downs after the current index
+            // remove all the drop downs after the current index (accessible names do not have to be updated since drop downs at the end of the list are getting removed)
             int elementsToBeRemoved = parent.Children().Size() - dropDownIndex - 1;
             for (int i = 0; i < elementsToBeRemoved; i++)
             {
@@ -154,6 +156,13 @@ std::pair<KeyboardManagerHelper::ErrorType, int> KeyDropDownControl::ValidateSho
         // Handle None case if there are no other errors
         else if (validationResult.second == BufferValidationHelpers::DropDownAction::DeleteDropDown)
         {
+            // Update accessible names for drop downs appearing after the deleted one
+            for (uint32_t i = dropDownIndex + 1; i < keyDropDownControlObjects.size(); i++)
+            {
+                // Update accessible name (row index will become i-1 for this element, so the display name would be i (display name indexing from 1)
+                SetAccessibleNameForComboBox(keyDropDownControlObjects[i]->GetComboBox(), i);
+            }
+
             parent.Children().RemoveAt(dropDownIndex);
             // delete drop down control object from the vector so that it can be destructed
             keyDropDownControlObjects.erase(keyDropDownControlObjects.begin() + dropDownIndex);
@@ -265,6 +274,9 @@ void KeyDropDownControl::AddDropDown(Grid table, StackPanel shortcutControl, Sta
     parent.Children().Append(keyDropDownControlObjects[keyDropDownControlObjects.size() - 1]->GetComboBox());
     keyDropDownControlObjects[keyDropDownControlObjects.size() - 1]->SetSelectionHandler(table, shortcutControl, parent, colIndex, shortcutRemapBuffer, keyDropDownControlObjects, targetApp, isHybridControl, isSingleKeyWindow);
     parent.UpdateLayout();
+
+    // Update accessible name
+    SetAccessibleNameForComboBox(keyDropDownControlObjects[keyDropDownControlObjects.size() - 1]->GetComboBox(), (int)keyDropDownControlObjects.size());
 }
 
 // Function to get the list of key codes from the shortcut combo box stack panel
diff --git a/src/modules/keyboardmanager/ui/KeyDropDownControl.h b/src/modules/keyboardmanager/ui/KeyDropDownControl.h
index bc182ed18e..3d3c13961b 100644
--- a/src/modules/keyboardmanager/ui/KeyDropDownControl.h
+++ b/src/modules/keyboardmanager/ui/KeyDropDownControl.h
@@ -45,6 +45,9 @@ private:
     // Function to check if the layout has changed and accordingly update the drop down list
     void CheckAndUpdateKeyboardLayout(ComboBox currentDropDown, bool isShortcut);
 
+    // Function to set accessible name for combobox
+    static void SetAccessibleNameForComboBox(ComboBox dropDown, int index);
+
 public:
     // Pointer to the keyboard manager state
     static KeyboardManagerState* keyboardManagerState;
diff --git a/src/modules/keyboardmanager/ui/ShortcutControl.cpp b/src/modules/keyboardmanager/ui/ShortcutControl.cpp
index f2e0c480e0..1ed92ead8d 100644
--- a/src/modules/keyboardmanager/ui/ShortcutControl.cpp
+++ b/src/modules/keyboardmanager/ui/ShortcutControl.cpp
@@ -43,10 +43,10 @@ ShortcutControl::ShortcutControl(Grid table, const int colIndex, TextBox targetA
 }
 
 // Function to set the accessible name of the target App text box
-void ShortcutControl::SetAccessibleNameForTextBox(TextBox targetAppTextBox)
+void ShortcutControl::SetAccessibleNameForTextBox(TextBox targetAppTextBox, int rowIndex)
 {
     // To set the accessible name of the target App text box by adding the string `All Apps` if the text box is empty, if not the application name is read by narrator.
-    std::wstring targetAppTextBoxAccessibleName = GET_RESOURCE_STRING(IDS_TARGET_APPLICATION);
+    std::wstring targetAppTextBoxAccessibleName = GET_RESOURCE_STRING(IDS_AUTOMATIONPROPERTIES_ROW) + std::to_wstring(rowIndex) + L", " + GET_RESOURCE_STRING(IDS_EDITSHORTCUTS_TARGETAPPHEADER);
     if (targetAppTextBox.Text() == L"")
     {
         targetAppTextBoxAccessibleName += GET_RESOURCE_STRING(IDS_EDITSHORTCUTS_ALLAPPS);
@@ -54,6 +54,15 @@ void ShortcutControl::SetAccessibleNameForTextBox(TextBox targetAppTextBox)
     targetAppTextBox.SetValue(Automation::AutomationProperties::NameProperty(), box_value(targetAppTextBoxAccessibleName));
 }
 
+// Function to set the accessible names for all the controls in a row
+void ShortcutControl::UpdateAccessibleNames(StackPanel sourceColumn, StackPanel mappedToColumn, TextBox targetAppTextBox, Button deleteButton, int rowIndex)
+{
+    sourceColumn.SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_AUTOMATIONPROPERTIES_ROW) + std::to_wstring(rowIndex) + L", " + GET_RESOURCE_STRING(IDS_EDITSHORTCUTS_SOURCEHEADER)));
+    mappedToColumn.SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_AUTOMATIONPROPERTIES_ROW) + std::to_wstring(rowIndex) + L", " + GET_RESOURCE_STRING(IDS_EDITSHORTCUTS_TARGETHEADER)));
+    ShortcutControl::SetAccessibleNameForTextBox(targetAppTextBox, rowIndex);
+    deleteButton.SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_AUTOMATIONPROPERTIES_ROW) + std::to_wstring(rowIndex) + L", " + GET_RESOURCE_STRING(IDS_DELETE_REMAPPING_BUTTON)));
+}
+
 // Function to add a new row to the shortcut table. If the originalKeys and newKeys args are provided, then the displayed shortcuts are set to those values.
 void ShortcutControl::AddNewShortcutControlRow(Grid& parent, std::vector<std::vector<std::unique_ptr<ShortcutControl>>>& keyboardRemapControlObjects, const Shortcut& originalKeys, const std::variant<DWORD, Shortcut>& newKeys, const std::wstring& targetAppName)
 {
@@ -85,8 +94,6 @@ void ShortcutControl::AddNewShortcutControlRow(Grid& parent, std::vector<std::ve
     parent.SetRow(arrowIcon, parent.RowDefinitions().Size() - 1);
     parent.Children().Append(arrowIcon);
 
-    // To set the accessible name of the arrow icon by setting the accessible name of the remapped shortcut
-    keyboardRemapControlObjects[keyboardRemapControlObjects.size() - 1][1]->getShortcutControl().SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_REMAPPED_TO)));
     // ShortcutControl for the new shortcut
     parent.Children().Append(keyboardRemapControlObjects[keyboardRemapControlObjects.size() - 1][1]->getShortcutControl());
 
@@ -96,8 +103,6 @@ void ShortcutControl::AddNewShortcutControlRow(Grid& parent, std::vector<std::ve
     targetAppTextBox.HorizontalAlignment(HorizontalAlignment::Center);
     targetAppTextBox.PlaceholderText(KeyboardManagerConstants::DefaultAppName);
     targetAppTextBox.Text(targetAppName);
-    // Initialize the accessible name of the target app text box
-    ShortcutControl::SetAccessibleNameForTextBox(targetAppTextBox);
 
     // LostFocus handler will be called whenever text is updated by a user and then they click something else or tab to another control. Does not get called if Text is updated while the TextBox isn't in focus (i.e. from code)
     targetAppTextBox.LostFocus([&keyboardRemapControlObjects, parent, targetAppTextBox](auto const& sender, auto const& e) {
@@ -157,7 +162,7 @@ void ShortcutControl::AddNewShortcutControlRow(Grid& parent, std::vector<std::ve
         }
 
         // To set the accessibile name of the target app text box when focus is lost
-        ShortcutControl::SetAccessibleNameForTextBox(targetAppTextBox);
+        ShortcutControl::SetAccessibleNameForTextBox(targetAppTextBox, rowIndex + 1);
     });
 
     parent.SetColumn(targetAppTextBox, KeyboardManagerConstants::ShortcutTableTargetAppColIndex);
@@ -193,6 +198,18 @@ void ShortcutControl::AddNewShortcutControlRow(Grid& parent, std::vector<std::ve
             parent.SetRow(children.GetAt(i).as<FrameworkElement>(), elementRowIndex - 1);
         }
 
+        // Update accessible names for each row after the deleted row
+        for (uint32_t i = lastIndexInRow + 1; i < children.Size(); i += KeyboardManagerConstants::ShortcutTableColCount)
+        {
+            // Get row index from grid
+            int32_t elementRowIndex = parent.GetRow(children.GetAt(i).as<FrameworkElement>());
+            StackPanel sourceCol = children.GetAt(i + KeyboardManagerConstants::ShortcutTableOriginalColIndex).as<StackPanel>();
+            StackPanel targetCol = children.GetAt(i + KeyboardManagerConstants::ShortcutTableNewColIndex).as<StackPanel>();
+            TextBox targetApp = children.GetAt(i + KeyboardManagerConstants::ShortcutTableTargetAppColIndex).as<TextBox>();
+            Button delButton = children.GetAt(i + KeyboardManagerConstants::ShortcutTableRemoveColIndex).as<Button>();
+            UpdateAccessibleNames(sourceCol, targetCol, targetApp, delButton, elementRowIndex);
+        }
+
         for (int i = 0; i < KeyboardManagerConstants::ShortcutTableColCount; i++)
         {
             parent.Children().RemoveAt(lastIndexInRow - i);
@@ -214,6 +231,9 @@ void ShortcutControl::AddNewShortcutControlRow(Grid& parent, std::vector<std::ve
     parent.Children().Append(deleteShortcut);
     parent.UpdateLayout();
 
+    // Set accessible names
+    UpdateAccessibleNames(keyboardRemapControlObjects[keyboardRemapControlObjects.size() - 1][0]->getShortcutControl(), keyboardRemapControlObjects[keyboardRemapControlObjects.size() - 1][1]->getShortcutControl(), targetAppTextBox, deleteShortcut, parent.RowDefinitions().Size() - 1);
+
     // Set the shortcut text if the two vectors are not empty (i.e. default args)
     if (originalKeys.IsValidShortcut() && !(newKeys.index() == 0 && std::get<DWORD>(newKeys) == NULL) && !(newKeys.index() == 1 && !std::get<Shortcut>(newKeys).IsValidShortcut()))
     {
diff --git a/src/modules/keyboardmanager/ui/ShortcutControl.h b/src/modules/keyboardmanager/ui/ShortcutControl.h
index ff4bdb2eda..6730fe8ecf 100644
--- a/src/modules/keyboardmanager/ui/ShortcutControl.h
+++ b/src/modules/keyboardmanager/ui/ShortcutControl.h
@@ -12,6 +12,7 @@ namespace winrt::Windows::UI::Xaml
         struct StackPanel;
         struct Grid;
         struct TextBox;
+        struct Button;
     }
 }
 
@@ -28,7 +29,10 @@ private:
     winrt::Windows::Foundation::IInspectable shortcutControlLayout;
 
     // Function to set the accessible name of the target app text box
-    static void SetAccessibleNameForTextBox(TextBox targetAppTextBox);
+    static void SetAccessibleNameForTextBox(TextBox targetAppTextBox, int rowIndex);
+
+    // Function to set the accessible names for all the controls in a row
+    static void UpdateAccessibleNames(StackPanel sourceColumn, StackPanel mappedToColumn, TextBox targetAppTextBox, Button deleteButton, int rowIndex);
 
 public:
     // Handle to the current Edit Shortcuts Window
diff --git a/src/modules/keyboardmanager/ui/SingleKeyRemapControl.cpp b/src/modules/keyboardmanager/ui/SingleKeyRemapControl.cpp
index a643149107..d464deba8f 100644
--- a/src/modules/keyboardmanager/ui/SingleKeyRemapControl.cpp
+++ b/src/modules/keyboardmanager/ui/SingleKeyRemapControl.cpp
@@ -62,6 +62,14 @@ SingleKeyRemapControl::SingleKeyRemapControl(Grid table, const int colIndex)
     singleKeyRemapControlLayout.as<StackPanel>().UpdateLayout();
 }
 
+// Function to set the accessible names for all the controls in a row
+void SingleKeyRemapControl::UpdateAccessibleNames(StackPanel sourceColumn, StackPanel mappedToColumn, Button deleteButton, int rowIndex)
+{
+    sourceColumn.SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_AUTOMATIONPROPERTIES_ROW) + std::to_wstring(rowIndex) + L", " + GET_RESOURCE_STRING(IDS_EDITKEYBOARD_SOURCEHEADER)));
+    mappedToColumn.SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_AUTOMATIONPROPERTIES_ROW) + std::to_wstring(rowIndex) + L", " + GET_RESOURCE_STRING(IDS_EDITKEYBOARD_TARGETHEADER)));
+    deleteButton.SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_AUTOMATIONPROPERTIES_ROW) + std::to_wstring(rowIndex) + L", " + GET_RESOURCE_STRING(IDS_DELETE_REMAPPING_BUTTON)));
+}
+
 // Function to add a new row to the remap keys table. If the originalKey and newKey args are provided, then the displayed remap keys are set to those values.
 void SingleKeyRemapControl::AddNewControlKeyRemapRow(Grid& parent, std::vector<std::vector<std::unique_ptr<SingleKeyRemapControl>>>& keyboardRemapControlObjects, const DWORD originalKey, const std::variant<DWORD, Shortcut> newKey)
 {
@@ -90,8 +98,6 @@ void SingleKeyRemapControl::AddNewControlKeyRemapRow(Grid& parent, std::vector<s
     parent.SetRow(arrowIcon, parent.RowDefinitions().Size() - 1);
     parent.Children().Append(arrowIcon);
 
-    // To set the accessible name of the arrow icon by setting the accessible name of the remapped key
-    keyboardRemapControlObjects[keyboardRemapControlObjects.size() - 1][1]->getSingleKeyRemapControl().SetValue(Automation::AutomationProperties::NameProperty(), box_value(GET_RESOURCE_STRING(IDS_REMAPPED_TO)));
     // SingleKeyRemapControl for the new remap key
     parent.Children().Append(keyboardRemapControlObjects[keyboardRemapControlObjects.size() - 1][1]->getSingleKeyRemapControl());
 
@@ -154,6 +160,17 @@ void SingleKeyRemapControl::AddNewControlKeyRemapRow(Grid& parent, std::vector<s
             parent.SetRow(children.GetAt(i).as<FrameworkElement>(), elementRowIndex - 1);
         }
 
+        // Update accessible names for each row after the deleted row
+        for (uint32_t i = lastIndexInRow + 1; i < children.Size(); i += KeyboardManagerConstants::RemapTableColCount)
+        {
+            // Get row index from grid
+            int32_t elementRowIndex = parent.GetRow(children.GetAt(i).as<FrameworkElement>());
+            StackPanel sourceCol = children.GetAt(i + KeyboardManagerConstants::RemapTableOriginalColIndex).as<StackPanel>();
+            StackPanel targetCol = children.GetAt(i + KeyboardManagerConstants::RemapTableNewColIndex).as<StackPanel>();
+            Button delButton = children.GetAt(i + KeyboardManagerConstants::RemapTableRemoveColIndex).as<Button>();
+            UpdateAccessibleNames(sourceCol, targetCol, delButton, elementRowIndex);
+        }
+
         for (int i = 0; i < KeyboardManagerConstants::RemapTableColCount; i++)
         {
             parent.Children().RemoveAt(lastIndexInRow - i);
@@ -174,6 +191,9 @@ void SingleKeyRemapControl::AddNewControlKeyRemapRow(Grid& parent, std::vector<s
     parent.SetRow(deleteRemapKeys, parent.RowDefinitions().Size() - 1);
     parent.Children().Append(deleteRemapKeys);
     parent.UpdateLayout();
+
+    // Set accessible names
+    UpdateAccessibleNames(keyboardRemapControlObjects[keyboardRemapControlObjects.size() - 1][0]->getSingleKeyRemapControl(), keyboardRemapControlObjects[keyboardRemapControlObjects.size() - 1][1]->getSingleKeyRemapControl(), deleteRemapKeys, parent.RowDefinitions().Size() - 1);
 }
 
 // Function to return the stack panel element of the SingleKeyRemapControl. This is the externally visible UI element which can be used to add it to other layouts
diff --git a/src/modules/keyboardmanager/ui/SingleKeyRemapControl.h b/src/modules/keyboardmanager/ui/SingleKeyRemapControl.h
index 3ba7164165..9e70f28a23 100644
--- a/src/modules/keyboardmanager/ui/SingleKeyRemapControl.h
+++ b/src/modules/keyboardmanager/ui/SingleKeyRemapControl.h
@@ -10,6 +10,7 @@ namespace winrt::Windows::UI::Xaml
     {
         struct StackPanel;
         struct Grid;
+        struct Button;
     }
 }
 
@@ -25,6 +26,9 @@ private:
     // Stack panel for the drop downs to display the selected shortcut for the hybrid case
     winrt::Windows::Foundation::IInspectable hybridDropDownStackPanel;
 
+    // Function to set the accessible names for all the controls in a row
+    static void UpdateAccessibleNames(StackPanel sourceColumn, StackPanel mappedToColumn, Button deleteButton, int rowIndex);
+
 public:
     // Vector to store dynamically allocated KeyDropDownControl objects to avoid early destruction
     std::vector<std::unique_ptr<KeyDropDownControl>> keyDropDownControlObjects;