From a8d7e68c5820fdc2c459d8dc6990e03ea3f79cca Mon Sep 17 00:00:00 2001 From: Mango Yen Date: Mon, 7 Sep 2026 19:04:22 +0800 Subject: [PATCH] Return keyboard focus to terminal when pane context menu closes When dismissing the pane context menu (or submenus) with Esc, XAML leaves keyboard focus on the now-hidden AppBarButton or MoreButton. Subsequent Enter invokes the hidden item instead of reaching the shell. Fix: - TermControl: Restore focus on context menu Closed if the focused element belongs to the closing flyout; respect active search box. - TerminalPage: Hand focus back to parent button when a sub-flyout closes. - Utils: Add bounded VisualTree helper IsElementInCommandBarFlyout to verify flyout membership and prevent cross-pane focus stealing. Closes #20593 --- src/cascadia/TerminalApp/TerminalPage.cpp | 71 ++++++++-- src/cascadia/TerminalControl/TermControl.cpp | 56 +++++++- src/cascadia/TerminalControl/TermControl.h | 1 + src/cascadia/WinRTUtils/inc/Utils.h | 130 +++++++++++++++++++ 4 files changed, 246 insertions(+), 12 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 6a65c342e48..58500f22372 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -5491,10 +5491,59 @@ namespace winrt::TerminalApp::implementation targetMenu.SecondaryCommands().Append(button); }; - auto makeMenuItem = [](const winrt::hstring& label, - const winrt::hstring& icon, - const auto& subMenu, - auto& targetMenu) { + // GH#20593: dismissing a nested flyout with Esc leaves keyboard focus on + // the button inside it that had it. That button is no longer on screen + // but still alive, so Enter invokes it. When a sub-flyout closes with + // focus still on one of its own buttons, hand it back to the button that + // opened it. (The top-level flyout does the same for the control itself + // in TermControl.) + auto handBackFocusOnClose = [weakControl = winrt::make_weak(control)](const MUX::Controls::CommandBarFlyout& subMenu, const AppBarButton& owner) { + subMenu.Closed([weakMenu = winrt::make_weak(subMenu), weakOwner = winrt::make_weak(owner), weakControl](auto&&, auto&&) { + const auto menu{ weakMenu.get() }; + const auto owner{ weakOwner.get() }; + if (!menu || !owner) + { + return; + } + auto root{ owner.XamlRoot() }; + if (!root) + { + if (const auto control{ weakControl.get() }) + { + root = control.XamlRoot(); + } + } + if (!root) + { + return; + } + const auto focused{ WUX::Input::FocusManager::GetFocusedElement(root) }; + if (const auto focusedDo = focused.try_as()) + { + if (IsElementInCommandBarFlyout(focusedDo, menu)) + { + if (WUX::Media::VisualTreeHelper::GetParent(owner)) + { + if (!owner.Focus(FocusState::Keyboard) && !owner.Focus(FocusState::Programmatic)) + { + if (const auto control{ weakControl.get() }) + { + control.Focus(FocusState::Programmatic); + } + } + } + // If owner is detached (e.g. during light-dismiss of the entire flyout hierarchy), + // do not force focus to the terminal here: the top-level menu's Closed handler in + // TermControl will handle focus restoration and respect open UI states like the Find search box. + } + } + }); + }; + + auto makeMenuItem = [&handBackFocusOnClose](const winrt::hstring& label, + const winrt::hstring& icon, + const auto& subMenu, + auto& targetMenu) { AppBarButton button{}; if (!icon.empty()) @@ -5506,15 +5555,16 @@ namespace winrt::TerminalApp::implementation button.Label(label); button.Flyout(subMenu); + handBackFocusOnClose(subMenu, button); targetMenu.SecondaryCommands().Append(button); }; - auto makeContextItem = [&makeCallback](const winrt::hstring& label, - const winrt::hstring& icon, - const winrt::hstring& tooltip, - const auto& action, - const auto& subMenu, - auto& targetMenu) { + auto makeContextItem = [&makeCallback, &handBackFocusOnClose](const winrt::hstring& label, + const winrt::hstring& icon, + const winrt::hstring& tooltip, + const auto& action, + const auto& subMenu, + auto& targetMenu) { AppBarButton button{}; if (!icon.empty()) @@ -5528,6 +5578,7 @@ namespace winrt::TerminalApp::implementation button.Click(makeCallback(action)); WUX::Controls::ToolTipService::SetToolTip(button, box_value(tooltip)); button.ContextFlyout(subMenu); + handBackFocusOnClose(subMenu, button); targetMenu.SecondaryCommands().Append(button); }; diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index fcb21cb0d54..231ce210b9f 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -3,6 +3,7 @@ #include "pch.h" #include "TermControl.h" +#include #include @@ -429,7 +430,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation ContextMenu().Closed([weakThis = get_weak()](auto&&, auto&&) { if (auto control{ weakThis.get() }; control && !control->_IsClosing()) { - const auto& menu{ control->ContextMenu() }; + const auto menu{ control->ContextMenu() }; + control->_takeFocusBackFromContextMenu(menu); menu.PrimaryCommands().Clear(); menu.SecondaryCommands().Clear(); for (const auto& e : control->_originalPrimaryElements) @@ -445,7 +447,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation SelectionContextMenu().Closed([weakThis = get_weak()](auto&&, auto&&) { if (auto control{ weakThis.get() }; control && !control->_IsClosing()) { - const auto& menu{ control->SelectionContextMenu() }; + const auto menu{ control->SelectionContextMenu() }; + control->_takeFocusBackFromContextMenu(menu); menu.PrimaryCommands().Clear(); menu.SecondaryCommands().Clear(); for (const auto& e : control->_originalSelectedPrimaryElements) @@ -3931,6 +3934,55 @@ namespace winrt::Microsoft::Terminal::Control::implementation }; } + // Method Description: + // - GH#20593: when the pane context menu is dismissed with Esc, XAML leaves + // keyboard focus on the AppBarButton that had it, even though the flyout + // is gone. The button is off screen but alive, so Enter would invoke it. + // If the flyout closed with focus still on one of its own commands, nothing + // else took focus, so hand it back to the control. We verify membership against + // the closing menu specifically to avoid cross-pane focus stealing in + // multi-pane layouts. + // Arguments: + // - menu: the CommandBarFlyout that was closed. + // Return Value: + // - + void TermControl::_takeFocusBackFromContextMenu(const winrt::Microsoft::UI::Xaml::Controls::CommandBarFlyout& menu) + { + if (!menu) + { + return; + } + + const auto root = XamlRoot(); + if (!root) + { + return; + } + + const auto focused = FocusManager::GetFocusedElement(root); + if (!focused) + { + return; + } + + if (const auto focusedDo = focused.try_as()) + { + if (IsElementInCommandBarFlyout(focusedDo, menu)) + { + // GH#10112: if the search box is active/open, return focus to it; + // otherwise restore focus to the terminal control. + if (_searchBox && _searchBox->IsOpen()) + { + _searchBox->SetFocusOnTextbox(); + } + else + { + Focus(FocusState::Programmatic); + } + } + } + } + void TermControl::_contextMenuHandler(IInspectable /*sender*/, Control::ContextMenuRequestedEventArgs args) { diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 4011c76fc22..6548ebe2279 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -433,6 +433,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _pasteTextWithBroadcast(const winrt::hstring& text); void _contextMenuHandler(IInspectable sender, Control::ContextMenuRequestedEventArgs args); + void _takeFocusBackFromContextMenu(const winrt::Microsoft::UI::Xaml::Controls::CommandBarFlyout& menu); void _showContextMenuAt(const winrt::Windows::Foundation::Point& controlRelativePos); void _bubbleSearchMissingCommand(const IInspectable& sender, const Control::SearchMissingCommandEventArgs& args); diff --git a/src/cascadia/WinRTUtils/inc/Utils.h b/src/cascadia/WinRTUtils/inc/Utils.h index 036050a21bd..0d3bf6b2392 100644 --- a/src/cascadia/WinRTUtils/inc/Utils.h +++ b/src/cascadia/WinRTUtils/inc/Utils.h @@ -117,4 +117,134 @@ winrt::Windows::Foundation::IInspectable ThemeLookup(const auto& res, // We didn't find it in the requested dict, fall back to the default dictionary. return res.Lookup(key); }; + +#if __has_include() +#include + +// Function Description: +// - GH#20593: Determines whether a focused dependency object (or any of its visual ancestors) +// belongs to the specified CommandBarFlyout or any of its nested child flyouts. +// Handles internal parts such as MoreButton (and its child elements) while strictly +// bounding upward traversal to CommandBar/Popup to avoid cross-pane focus stealing. +// Arguments: +// - focused: The currently focused DependencyObject. +// - flyout: The CommandBarFlyout being checked. +// Return value: +// - true if the focused element is within the flyout hierarchy; false otherwise. +inline bool IsElementInCommandBarFlyout( + const winrt::Windows::UI::Xaml::DependencyObject& focused, + const winrt::Microsoft::UI::Xaml::Controls::CommandBarFlyout& flyout) +{ + if (!focused || !flyout) + { + return false; + } + + static constexpr std::wstring_view moreButtonPartName{ L"MoreButton" }; + + winrt::Windows::UI::Xaml::Controls::ICommandBarElement matchedCommand{ nullptr }; + winrt::Windows::UI::Xaml::Controls::CommandBar focusedBar{ nullptr }; + bool isInsideMoreButton = false; + + for (auto cur = focused; cur; cur = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(cur)) + { + if (auto cmd = cur.try_as()) + { + matchedCommand = cmd; + break; + } + if (const auto fe = cur.try_as(); fe && fe.Name() == moreButtonPartName) + { + isInsideMoreButton = true; + } + if (auto cb = cur.try_as()) + { + if (isInsideMoreButton) + { + focusedBar = cb; + } + break; + } + if (cur.try_as()) + { + break; + } + } + + if (!matchedCommand && !focusedBar) + { + return false; + } + + struct MenuChecker + { + static bool Check( + const winrt::Microsoft::UI::Xaml::Controls::CommandBarFlyout& currentMenu, + const winrt::Windows::UI::Xaml::Controls::ICommandBarElement& targetCommand, + const winrt::Windows::UI::Xaml::Controls::CommandBar& targetBar, + const size_t depth) + { + if (depth > 8 || !currentMenu) + { + return false; + } + + for (const auto& commands : { currentMenu.PrimaryCommands(), currentMenu.SecondaryCommands() }) + { + if (commands) + { + bool checkedBarForThisList = false; + for (const auto& element : commands) + { + if (targetCommand && element == targetCommand) + { + return true; + } + if (targetBar && !checkedBarForThisList) + { + if (const auto cmdDo = element.try_as()) + { + for (auto p = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(cmdDo); p; p = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(p)) + { + if (p == targetBar) + { + return true; + } + if (p.try_as()) + { + checkedBarForThisList = true; + break; + } + if (p.try_as()) + { + checkedBarForThisList = true; + break; + } + } + } + } + if (const auto btn = element.try_as()) + { + for (const auto& child : { btn.Flyout(), btn.ContextFlyout() }) + { + if (const auto childFlyout = child.try_as()) + { + if (Check(childFlyout, targetCommand, targetBar, depth + 1)) + { + return true; + } + } + } + } + } + } + } + return false; + } + }; + + return MenuChecker::Check(flyout, matchedCommand, focusedBar, 0); +} #endif +#endif +