diff --git a/controls/dev/CommandBarFlyout/CommandBarFlyout.cpp b/controls/dev/CommandBarFlyout/CommandBarFlyout.cpp index fa3bb42982..68a4a26a21 100644 --- a/controls/dev/CommandBarFlyout/CommandBarFlyout.cpp +++ b/controls/dev/CommandBarFlyout/CommandBarFlyout.cpp @@ -7,9 +7,14 @@ #include "CommandBarFlyoutCommandBar.h" #include "Vector.h" #include "RuntimeProfiler.h" +#include "velocity.h" +#include #include "CommandBarFlyout.properties.cpp" +// Bug 46607274: [1.4 servicing] Microsoft.UI.Xaml.Controls.dll!CommandBarFlyoutCommandBar::EnsureLocalizedControlTypes impacting context menu performance +#define WINAPPSDK_CHANGEID_46607274 46607274 + // Change to 'true' to turn on debugging outputs in Output window bool CommandBarFlyoutTrace::s_IsDebugOutputEnabled{ false }; bool CommandBarFlyoutTrace::s_IsVerboseDebugOutputEnabled{ false }; @@ -307,6 +312,20 @@ winrt::Control CommandBarFlyout::CreatePresenter() { auto commandBar = winrt::make_self(); + if (WinAppSdk::Containment::IsChangeEnabled()) + { + // Localized string resource lookup is more expensive on MRTCore. Do the lookup ahead of time and reuse it for all + // the CommandBarFlyoutCommandBar::EnsureLocalizedControlTypes calls in response to PrimaryCommands/SecondCommands + // changed events. + commandBar->CacheLocalizedStringResources(); + } + auto const scopeGuard = WinAppSdk::Containment::IsChangeEnabled() + ? wil::scope_exit(std::function([commandBar]() + { + commandBar->ClearLocalizedStringResourceCache(); + })) + : wil::scope_exit(std::function([](){})); + SharedHelpers::CopyVector(m_primaryCommands, commandBar->PrimaryCommands()); SharedHelpers::CopyVector(m_secondaryCommands, commandBar->SecondaryCommands()); diff --git a/controls/dev/CommandBarFlyout/CommandBarFlyoutCommandBar.cpp b/controls/dev/CommandBarFlyout/CommandBarFlyoutCommandBar.cpp index 0a2a925fe3..b96d96c097 100644 --- a/controls/dev/CommandBarFlyout/CommandBarFlyoutCommandBar.cpp +++ b/controls/dev/CommandBarFlyout/CommandBarFlyoutCommandBar.cpp @@ -895,17 +895,47 @@ void CommandBarFlyoutCommandBar::EnsureLocalizedControlTypes() } } +void CommandBarFlyoutCommandBar::CacheLocalizedStringResources() +{ + m_localizedCommandBarFlyoutAppBarButtonControlType = ResourceAccessor::GetLocalizedStringResource(SR_CommandBarFlyoutAppBarButtonLocalizedControlType); + m_localizedCommandBarFlyoutAppBarToggleButtonControlType = ResourceAccessor::GetLocalizedStringResource(SR_CommandBarFlyoutAppBarToggleButtonLocalizedControlType); + m_areLocalizedStringResourcesCached = true; +} + +void CommandBarFlyoutCommandBar::ClearLocalizedStringResourceCache() +{ + m_areLocalizedStringResourcesCached = false; + m_localizedCommandBarFlyoutAppBarButtonControlType.clear(); + m_localizedCommandBarFlyoutAppBarToggleButtonControlType.clear(); +} + void CommandBarFlyoutCommandBar::SetKnownCommandLocalizedControlTypes(winrt::ICommandBarElement const& command) { COMMANDBARFLYOUT_TRACE_VERBOSE(*this, TRACE_MSG_METH, METH_NAME, this); if (auto const& appBarButton = command.try_as()) { - winrt::AutomationProperties::SetLocalizedControlType(appBarButton, ResourceAccessor::GetLocalizedStringResource(SR_CommandBarFlyoutAppBarButtonLocalizedControlType)); + if (m_areLocalizedStringResourcesCached) + { + MUX_ASSERT(!m_localizedCommandBarFlyoutAppBarButtonControlType.empty()); + winrt::AutomationProperties::SetLocalizedControlType(appBarButton, m_localizedCommandBarFlyoutAppBarButtonControlType); + } + else + { + winrt::AutomationProperties::SetLocalizedControlType(appBarButton, ResourceAccessor::GetLocalizedStringResource(SR_CommandBarFlyoutAppBarButtonLocalizedControlType)); + } } else if (auto const& appBarToggleButton = command.try_as()) { - winrt::AutomationProperties::SetLocalizedControlType(appBarToggleButton, ResourceAccessor::GetLocalizedStringResource(SR_CommandBarFlyoutAppBarToggleButtonLocalizedControlType)); + if (m_areLocalizedStringResourcesCached) + { + MUX_ASSERT(!m_localizedCommandBarFlyoutAppBarToggleButtonControlType.empty()); + winrt::AutomationProperties::SetLocalizedControlType(appBarToggleButton, m_localizedCommandBarFlyoutAppBarToggleButtonControlType); + } + else + { + winrt::AutomationProperties::SetLocalizedControlType(appBarToggleButton, ResourceAccessor::GetLocalizedStringResource(SR_CommandBarFlyoutAppBarToggleButtonLocalizedControlType)); + } } } diff --git a/controls/dev/CommandBarFlyout/CommandBarFlyoutCommandBar.h b/controls/dev/CommandBarFlyout/CommandBarFlyoutCommandBar.h index 0ce35e5e84..097d14b4a7 100644 --- a/controls/dev/CommandBarFlyout/CommandBarFlyoutCommandBar.h +++ b/controls/dev/CommandBarFlyout/CommandBarFlyoutCommandBar.h @@ -46,6 +46,9 @@ class CommandBarFlyoutCommandBar : void OnPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args); + void CacheLocalizedStringResources(); + void ClearLocalizedStringResourceCache(); + private: void AttachEventHandlers(); void DetachEventHandlers(); @@ -127,4 +130,11 @@ class CommandBarFlyoutCommandBar : // The one built into Popup is too high up in the Visual tree to be animated by a custom animation. winrt::ContentExternalBackdropLink m_backdropLink{ nullptr }; winrt::ContentExternalBackdropLink m_overflowPopupBackdropLink{ nullptr }; + + // Localized string caches. Looking these up from MRTCore is expensive, so we don't want to put the lookups in a + // loop. Instead, look them up once, cache them, use the cached values, then clear the cache. The values in these + // caches are only valid after CacheLocalizedStringResources and before ClearLocalizedStringResourceCache. + bool m_areLocalizedStringResourcesCached{ false }; + winrt::hstring m_localizedCommandBarFlyoutAppBarButtonControlType{}; + winrt::hstring m_localizedCommandBarFlyoutAppBarToggleButtonControlType{}; }; diff --git a/controls/dev/NavigationView/NavigationView.cpp b/controls/dev/NavigationView/NavigationView.cpp index 0ffb250ce8..ce5e6e7b34 100644 --- a/controls/dev/NavigationView/NavigationView.cpp +++ b/controls/dev/NavigationView/NavigationView.cpp @@ -28,6 +28,11 @@ #include "NavigationViewItemExpandingEventArgs.h" #include "NavigationViewItemCollapsedEventArgs.h" #include "InspectingDataSource.h" +#include "velocity.h" +#include + +// Bug 46697123: [1.4 servicing] - Tab/Shift+Tab Navigation is very very confusing with List Views +#define WINAPPSDK_CHANGEID_46697123 46697123 // General items static constexpr auto c_togglePaneButtonName = L"TogglePaneButton"sv; @@ -2885,6 +2890,53 @@ void NavigationView::OnRepeaterGettingFocus(const winrt::IInspectable& sender, c args.Handled(true); } } + else if (WinAppSdk::Containment::IsChangeEnabled() && + !isFocusOutsideCurrentRootRepeater) + { + // Tab or Shift-Tab from within the same root repeater should move focus outside it + auto navigationViewItemBase = args.NewFocusedElement().try_as(); + if (navigationViewItemBase && + (args.Direction() == winrt::FocusNavigationDirection::Previous || + args.Direction() == winrt::FocusNavigationDirection::Next)) + { + // We need to find the next tab stop outside the root repeater. + // winrt::FocusManager::FindNextElement will at first give us NavigationViewItems + // within the root repeater. This is why we need to set IsTabStop = false for + // the found the elements within the root repeater so that FindNextElement eventually + // finds an element outside the root repeater. + std::vector containersWithTabFocusOff{}; + + const winrt::FindNextElementOptions options; + options.SearchRoot(this->XamlRoot().Content()); + + auto nextElement = winrt::FocusManager::FindNextElement(args.Direction(), options); + while (auto nvib = nextElement.try_as()) + { + // Verify the item is in the root repeater we are trying to leave + if (newRootItemsRepeater == GetParentRootItemsRepeaterForContainer(nvib)) + { + nvib.IsTabStop(false); + // Keep track of the container so that we can re-enable tab stop once focus leaves + containersWithTabFocusOff.push_back(nvib); + nextElement = winrt::FocusManager::FindNextElement(args.Direction(), options); + } + else + { + break; + } + } + + for (auto nvib : containersWithTabFocusOff) + { + nvib.IsTabStop(true); + } + + if (args.TrySetNewFocusedElement(nextElement)) + { + args.Handled(true); + } + } + } } } } diff --git a/controls/dev/NavigationView/NavigationView.xaml b/controls/dev/NavigationView/NavigationView.xaml index 16a8362130..afbfd038ec 100644 --- a/controls/dev/NavigationView/NavigationView.xaml +++ b/controls/dev/NavigationView/NavigationView.xaml @@ -361,7 +361,10 @@ - + diff --git a/controls/dev/NavigationView/NavigationView_InteractionTests/FocusBehaviorTests.cs b/controls/dev/NavigationView/NavigationView_InteractionTests/FocusBehaviorTests.cs index f55e39bed9..463668df11 100644 --- a/controls/dev/NavigationView/NavigationView_InteractionTests/FocusBehaviorTests.cs +++ b/controls/dev/NavigationView/NavigationView_InteractionTests/FocusBehaviorTests.cs @@ -291,6 +291,160 @@ string GetSelectedItem() } } + [TestMethod] + public void TabNavigationHierarchicalTest() + { + var testScenarios = RegressionTestScenario.BuildLeftNavRegressionTestScenarios(); + foreach (var testScenario in testScenarios) + { + using (var setup = new TestSetupHelper(new[] { "NavigationView Tests", "HierarchicalNavigationView Markup Test" })) + { + Button togglePaneButton = new Button(FindElement.ById("TogglePaneButton")); + Button getSelectItemButton = new Button(FindElement.ByName("GetSelectedItemLabelButton")); + + VerifyTabNavigationWithoutMenuItemSelected(); + VerifyTabNavigationWithMenuItemSelected(); + VerifyTabNavigationWithSettingsItemVisible(); + + void VerifyTabNavigationWithoutMenuItemSelected() + { + Log.Comment("Verify Tab navigation without a selected menu item"); + + getSelectItemButton.Invoke(); + Wait.ForIdle(); + + Verify.AreEqual("No Item Selected", GetSelectedItem()); + + UIObject menuItem1 = FindElement.ByName("Menu Item 1"); + UIObject menuItem29 = FindElement.ByName("Menu Item 29 (Selectable)"); + + // Set focus on the pane's toggle button. + togglePaneButton.SetFocus(); + Wait.ForIdle(); + + Log.Comment("Verify that pressing tab while TogglePaneButton has focus moves to Menu Item 1."); + KeyboardHelper.PressKey(Key.Tab); + Wait.ForIdle(); + Verify.IsTrue(menuItem1.HasKeyboardFocus); + + Log.Comment("Verify that pressing tab while Menu Item 1 has focus moves to 'Get Selected Item Label' Button item"); + KeyboardHelper.PressKey(Key.Tab); + Wait.ForIdle(); + Verify.IsTrue(getSelectItemButton.HasKeyboardFocus); + + Log.Comment("Verify that pressing SHIFT+tab will move focus to Menu Item 29"); + KeyboardHelper.PressKey(Key.Tab, ModifierKey.Shift, 1); + Wait.ForIdle(); + Verify.IsTrue(menuItem29.HasKeyboardFocus); + + Log.Comment("Verify that pressing SHIFT+tab will move focus to the TogglePaneButton"); + KeyboardHelper.PressKey(Key.Tab, ModifierKey.Shift, 1); + Wait.ForIdle(); + Verify.IsTrue(togglePaneButton.HasKeyboardFocus); + } + + void VerifyTabNavigationWithMenuItemSelected() + { + Log.Comment("Verify Tab navigation with a selected menu item"); + + Log.Comment("Expand Menu Item 15."); + UIObject menuItem15 = FindElement.ByName("Menu Item 15"); + InputHelper.LeftClick(menuItem15); + + Log.Comment("Select Menu Item 16."); + UIObject menuItem16 = FindElement.ByName("Menu Item 16"); + InputHelper.LeftClick(menuItem16); + + getSelectItemButton.Invoke(); + Wait.ForIdle(); + + Verify.AreEqual("Menu Item 16", GetSelectedItem()); + + // Set focus on the pane's toggle button. + togglePaneButton.SetFocus(); + Wait.ForIdle(); + + Log.Comment("Verify that pressing tab while TogglePaneButton has focus moves to Menu Item 16."); + KeyboardHelper.PressKey(Key.Tab); + Wait.ForIdle(); + Verify.IsTrue(menuItem16.HasKeyboardFocus); + + Log.Comment("Verify that pressing tab while Menu Item 16 has focus moves to 'Get Selected Item Label' Button item"); + KeyboardHelper.PressKey(Key.Tab); + Wait.ForIdle(); + Verify.IsTrue(getSelectItemButton.HasKeyboardFocus); + + Log.Comment("Verify that pressing SHIFT+tab will move focus to Menu Item 16"); + KeyboardHelper.PressKey(Key.Tab, ModifierKey.Shift, 1); + Wait.ForIdle(); + Verify.IsTrue(menuItem16.HasKeyboardFocus); + + Log.Comment("Verify that pressing SHIFT+tab will move focus to the TogglePaneButton"); + KeyboardHelper.PressKey(Key.Tab, ModifierKey.Shift, 1); + Wait.ForIdle(); + Verify.IsTrue(togglePaneButton.HasKeyboardFocus); + + Log.Comment("Verify that pressing tab from parent of item will move focus to 'Get Selected Item Label' Button item"); + KeyboardHelper.PressKey(Key.Down, ModifierKey.None, 3); + Wait.ForIdle(); + Verify.IsTrue(menuItem15.HasKeyboardFocus); + + KeyboardHelper.PressKey(Key.Tab); + Wait.ForIdle(); + Verify.IsTrue(getSelectItemButton.HasKeyboardFocus); + } + + void VerifyTabNavigationWithSettingsItemVisible() + { + Log.Comment("Verify tab navigation with settings item visible."); + + Log.Comment("Check IsSettingsVisible"); + CheckBox checkBoxIsSettingsVisible = FindElement.ByName("Settings Item Visibility"); + checkBoxIsSettingsVisible.Check(); + Wait.ForIdle(); + + UIObject settingsItem = FindElement.ByName("Settings"); + + getSelectItemButton.Invoke(); + Wait.ForIdle(); + + Verify.AreEqual("Menu Item 16", GetSelectedItem()); + + UIObject menuItem16 = FindElement.ByName("Menu Item 16"); + + // Set focus on the pane's toggle button. + togglePaneButton.SetFocus(); + Wait.ForIdle(); + + Log.Comment("Verify that pressing tab while TogglePaneButton has focus moves to Menu Item 16."); + KeyboardHelper.PressKey(Key.Tab); + Wait.ForIdle(); + Verify.IsTrue(menuItem16.HasKeyboardFocus); + + Log.Comment("Verify that pressing tab while Menu Item 16 has focus moves to Settings item"); + KeyboardHelper.PressKey(Key.Tab); + Wait.ForIdle(); + Verify.IsTrue(settingsItem.HasKeyboardFocus); + + Log.Comment("Verify that pressing SHIFT+tab will move focus to Menu Item 16"); + KeyboardHelper.PressKey(Key.Tab, ModifierKey.Shift, 1); + Wait.ForIdle(); + Verify.IsTrue(menuItem16.HasKeyboardFocus); + + Log.Comment("Verify that pressing SHIFT+tab will move focus to the TogglePaneButton"); + KeyboardHelper.PressKey(Key.Tab, ModifierKey.Shift, 1); + Wait.ForIdle(); + Verify.IsTrue(togglePaneButton.HasKeyboardFocus); + } + } + + string GetSelectedItem() + { + return new TextBlock(FindElement.ByName("SelectedItemLabel")).DocumentText; + } + } + } + [TestMethod] public void CanDoSelectionChangedOfItemTemplate() { diff --git a/controls/dev/NavigationView/TestUI/Hierarchical/HierarchicalNavigationViewDataBinding.xaml b/controls/dev/NavigationView/TestUI/Hierarchical/HierarchicalNavigationViewDataBinding.xaml index adffb807ce..fee03a5844 100644 --- a/controls/dev/NavigationView/TestUI/Hierarchical/HierarchicalNavigationViewDataBinding.xaml +++ b/controls/dev/NavigationView/TestUI/Hierarchical/HierarchicalNavigationViewDataBinding.xaml @@ -17,8 +17,15 @@ - +