Skip to content

Return keyboard focus to terminal when pane context menu closes #20652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Mango Yen (mangokingTW) wants to merge 1 commit into
base: main
Choose a base branch
from
+246 −12
Open

Return keyboard focus to terminal when pane context menu closes #20652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 61 additions & 10 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<WUX::DependencyObject>())
{
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())
Expand All @@ -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())
Expand All @@ -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);
};

Expand Down
56 changes: 54 additions & 2 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "pch.h"
#include "TermControl.h"
#include <Utils.h>

#include <inputpaneinterop.h>

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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:
// - <none>
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<DependencyObject>())
{
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)
{
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
130 changes: 130 additions & 0 deletions src/cascadia/WinRTUtils/inc/Utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(<winrt/Microsoft.UI.Xaml.Controls.h>)
#include <winrt/Microsoft.UI.Xaml.Controls.h>

// 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<winrt::Windows::UI::Xaml::Controls::ICommandBarElement>())
{
matchedCommand = cmd;
break;
}
if (const auto fe = cur.try_as<winrt::Windows::UI::Xaml::FrameworkElement>(); fe && fe.Name() == moreButtonPartName)
{
isInsideMoreButton = true;
}
if (auto cb = cur.try_as<winrt::Windows::UI::Xaml::Controls::CommandBar>())
{
if (isInsideMoreButton)
{
focusedBar = cb;
}
break;
}
if (cur.try_as<winrt::Windows::UI::Xaml::Controls::Primitives::Popup>())
{
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<winrt::Windows::UI::Xaml::DependencyObject>())
{
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<winrt::Windows::UI::Xaml::Controls::CommandBar>())
{
checkedBarForThisList = true;
break;
}
if (p.try_as<winrt::Windows::UI::Xaml::Controls::Primitives::Popup>())
{
checkedBarForThisList = true;
break;
}
}
}
}
if (const auto btn = element.try_as<winrt::Windows::UI::Xaml::Controls::AppBarButton>())
{
for (const auto& child : { btn.Flyout(), btn.ContextFlyout() })
{
if (const auto childFlyout = child.try_as<winrt::Microsoft::UI::Xaml::Controls::CommandBarFlyout>())
{
if (Check(childFlyout, targetCommand, targetBar, depth + 1))
{
return true;
}
}
}
}
}
}
}
return false;
}
};

return MenuChecker::Check(flyout, matchedCommand, focusedBar, 0);
}
#endif
#endif