From 33aa726f925150afc49c72e4ce56e14e7d8648d5 Mon Sep 17 00:00:00 2001 From: Omar Mashal Date: Sun, 23 Aug 2026 20:18:43 +0300 Subject: [PATCH 1/2] Fix context menu crash after split pane submenu --- .../LocalTests_TerminalApp/TabTests.cpp | 48 +++++++++++ .../Resources/en-US/Resources.resw | 4 + src/cascadia/TerminalApp/TerminalPage.cpp | 84 +++++++++++-------- 3 files changed, 99 insertions(+), 37 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 3bf302108b2..d71b39d0ca7 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -84,6 +84,7 @@ namespace TerminalAppLocalTests TEST_METHOD(CloseZoomedPane); TEST_METHOD(SwapPanes); + TEST_METHOD(ContextMenuUsesMenuFlyoutsForSplitAndSwapPane); TEST_METHOD(NextMRUTab); TEST_METHOD(VerifyCommandPaletteTabSwitcherOrder); @@ -1045,6 +1046,53 @@ namespace TerminalAppLocalTests }); } + void TabTests::ContextMenuUsesMenuFlyoutsForSplitAndSwapPane() + { + auto page = _commonSetup(); + + TestOnUIThread([&page]() { + Log::Comment(L"Create a second pane so Swap pane is present."); + page->_SplitPane(nullptr, SplitDirection::Right, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr)); + + const auto activeControl{ page->_GetActiveControl() }; + VERIFY_IS_NOT_NULL(activeControl); + + winrt::Microsoft::UI::Xaml::Controls::CommandBarFlyout contextMenu{}; + page->_PopulateContextMenu(activeControl, contextMenu, false); + + bool foundSplitPane = false; + bool foundSwapPane = false; + + for (const auto& command : contextMenu.SecondaryCommands()) + { + if (const auto button{ command.try_as() }) + { + if (button.Label() == RS_(L"SplitPaneText")) + { + foundSplitPane = true; + const auto flyout{ button.Flyout() }; + const auto menuFlyout{ flyout.try_as() }; + VERIFY_IS_NOT_NULL(menuFlyout); + VERIFY_IS_GREATER_THAN(menuFlyout.Items().Size(), 0u); + VERIFY_IS_NOT_NULL(menuFlyout.Items().GetAt(0).try_as()); + } + else if (button.Label() == RS_(L"SwapPaneText")) + { + foundSwapPane = true; + const auto flyout{ button.Flyout() }; + const auto menuFlyout{ flyout.try_as() }; + VERIFY_IS_NOT_NULL(menuFlyout); + VERIFY_IS_GREATER_THAN(menuFlyout.Items().Size(), 0u); + VERIFY_IS_NOT_NULL(menuFlyout.Items().GetAt(0).try_as()); + } + } + } + + VERIFY_IS_TRUE(foundSplitPane); + VERIFY_IS_TRUE(foundSwapPane); + }); + } + void TabTests::NextMRUTab() { // This is a test for GH#8025 - we want to make sure that MRU tab diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index a178ae252d3..22578b85979 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -195,6 +195,10 @@ Right click for split directions - right/down/up/left + + Automatic + An option in the Split pane context menu that automatically chooses the split direction. + Split pane down diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index aaaed4c926b..ca906639e98 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -5584,33 +5584,28 @@ namespace winrt::TerminalApp::implementation 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) { - AppBarButton button{}; + auto makeMenuFlyoutItem = [&makeCallback](const winrt::hstring& label, + const winrt::hstring& icon, + const auto& action, + auto& targetMenu) { + WUX::Controls::MenuFlyoutItem item{}; if (!icon.empty()) { auto iconElement = UI::IconPathConverter::IconWUX(icon); Automation::AutomationProperties::SetAccessibilityView(iconElement, Automation::Peers::AccessibilityView::Raw); - button.Icon(iconElement); + item.Icon(iconElement); } - button.Label(label); - button.Click(makeCallback(action)); - WUX::Controls::ToolTipService::SetToolTip(button, box_value(tooltip)); - button.ContextFlyout(subMenu); - targetMenu.SecondaryCommands().Append(button); + item.Text(label); + item.Click(makeCallback(action)); + targetMenu.Items().Append(item); }; const auto focusedProfile = _GetFocusedTabImpl()->GetFocusedProfile(); - auto separatorItem = AppBarSeparator{}; auto activeProfiles = _settings.ActiveProfiles(); auto activeProfileCount = gsl::narrow_cast(activeProfiles.Size()); - MUX::Controls::CommandBarFlyout splitPaneMenu{}; + WUX::Controls::MenuFlyout splitPaneMenu{}; // Wire up each item to the action that should be performed. By actually // connecting these to actions, we ensure the implementation is @@ -5627,20 +5622,41 @@ namespace winrt::TerminalApp::implementation const auto splitPaneDownText = RS_(L"SplitPaneDownText"); const auto splitPaneUpText = RS_(L"SplitPaneUpText"); const auto splitPaneLeftText = RS_(L"SplitPaneLeftText"); - const auto splitPaneToolTipText = RS_(L"SplitPaneToolTipText"); + const auto splitPaneAutomaticText = RS_(L"SplitPaneAutomaticText"); - MUX::Controls::CommandBarFlyout splitPaneContextMenu{}; - makeItem(splitPaneRightText, focusedProfileIcon, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Duplicate, SplitDirection::Right, .5, nullptr } }, splitPaneContextMenu); - makeItem(splitPaneDownText, focusedProfileIcon, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Duplicate, SplitDirection::Down, .5, nullptr } }, splitPaneContextMenu); - makeItem(splitPaneUpText, focusedProfileIcon, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Duplicate, SplitDirection::Up, .5, nullptr } }, splitPaneContextMenu); - makeItem(splitPaneLeftText, focusedProfileIcon, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Duplicate, SplitDirection::Left, .5, nullptr } }, splitPaneContextMenu); + auto makeSplitSubMenu = [&](const winrt::hstring& label, + const winrt::hstring& icon, + const SplitType splitType, + const NewTerminalArgs& args) { + WUX::Controls::MenuFlyoutSubItem subMenu{}; - makeContextItem(splitPaneDuplicateText, focusedProfileIcon, splitPaneToolTipText, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Duplicate, SplitDirection::Automatic, .5, nullptr } }, splitPaneContextMenu, splitPaneMenu); + if (!icon.empty()) + { + auto iconElement = UI::IconPathConverter::IconWUX(icon); + Automation::AutomationProperties::SetAccessibilityView(iconElement, Automation::Peers::AccessibilityView::Raw); + subMenu.Icon(iconElement); + } - // add menu separator - const auto separatorAutoItem = AppBarSeparator{}; + subMenu.Text(label); + + WUX::Controls::MenuFlyoutItem autoItem{}; + autoItem.Text(splitPaneAutomaticText); + autoItem.Click(makeCallback(ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ splitType, SplitDirection::Automatic, .5, args } })); + subMenu.Items().Append(autoItem); - splitPaneMenu.SecondaryCommands().Append(separatorAutoItem); + subMenu.Items().Append(WUX::Controls::MenuFlyoutSeparator{}); + + makeMenuFlyoutItem(splitPaneRightText, L"", ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ splitType, SplitDirection::Right, .5, args } }, subMenu); + makeMenuFlyoutItem(splitPaneDownText, L"", ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ splitType, SplitDirection::Down, .5, args } }, subMenu); + makeMenuFlyoutItem(splitPaneUpText, L"", ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ splitType, SplitDirection::Up, .5, args } }, subMenu); + makeMenuFlyoutItem(splitPaneLeftText, L"", ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ splitType, SplitDirection::Left, .5, args } }, subMenu); + + splitPaneMenu.Items().Append(subMenu); + }; + + makeSplitSubMenu(splitPaneDuplicateText, focusedProfileIcon, SplitType::Duplicate, nullptr); + + splitPaneMenu.Items().Append(WUX::Controls::MenuFlyoutSeparator{}); for (auto profileIndex = 0; profileIndex < activeProfileCount; profileIndex++) { @@ -5651,13 +5667,7 @@ namespace winrt::TerminalApp::implementation NewTerminalArgs args{}; args.Profile(profileName); - MUX::Controls::CommandBarFlyout splitPaneContextMenu{}; - makeItem(splitPaneRightText, profileIcon, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Manual, SplitDirection::Right, .5, args } }, splitPaneContextMenu); - makeItem(splitPaneDownText, profileIcon, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Manual, SplitDirection::Down, .5, args } }, splitPaneContextMenu); - makeItem(splitPaneUpText, profileIcon, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Manual, SplitDirection::Up, .5, args } }, splitPaneContextMenu); - makeItem(splitPaneLeftText, profileIcon, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Manual, SplitDirection::Left, .5, args } }, splitPaneContextMenu); - - makeContextItem(profileName, profileIcon, splitPaneToolTipText, ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ SplitType::Manual, SplitDirection::Automatic, .5, args } }, splitPaneContextMenu, splitPaneMenu); + makeSplitSubMenu(profileName, profileIcon, SplitType::Manual, args); } makeMenuItem(RS_(L"SplitPaneText"), L"\xF246", splitPaneMenu, menu); @@ -5665,7 +5675,7 @@ namespace winrt::TerminalApp::implementation // Only wire up "Close Pane" if there's multiple panes. if (_GetFocusedTabImpl()->GetLeafPaneCount() > 1) { - MUX::Controls::CommandBarFlyout swapPaneMenu{}; + WUX::Controls::MenuFlyout swapPaneMenu{}; const auto rootPane = _GetFocusedTabImpl()->GetRootPane(); const auto mruPanes = _GetFocusedTabImpl()->GetMruPanes(); auto activePane = _GetFocusedTabImpl()->GetActivePane(); @@ -5681,22 +5691,22 @@ namespace winrt::TerminalApp::implementation if (auto neighbor = rootPane->NavigateDirection(activePane, FocusDirection::Down, mruPanes)) { - makeItem(RS_(L"SwapPaneDownText"), neighbor->GetProfile().Icon().Resolved(), ActionAndArgs{ ShortcutAction::SwapPane, SwapPaneArgs{ FocusDirection::Down } }, swapPaneMenu); + makeMenuFlyoutItem(RS_(L"SwapPaneDownText"), neighbor->GetProfile().Icon().Resolved(), ActionAndArgs{ ShortcutAction::SwapPane, SwapPaneArgs{ FocusDirection::Down } }, swapPaneMenu); } if (auto neighbor = rootPane->NavigateDirection(activePane, FocusDirection::Right, mruPanes)) { - makeItem(RS_(L"SwapPaneRightText"), neighbor->GetProfile().Icon().Resolved(), ActionAndArgs{ ShortcutAction::SwapPane, SwapPaneArgs{ FocusDirection::Right } }, swapPaneMenu); + makeMenuFlyoutItem(RS_(L"SwapPaneRightText"), neighbor->GetProfile().Icon().Resolved(), ActionAndArgs{ ShortcutAction::SwapPane, SwapPaneArgs{ FocusDirection::Right } }, swapPaneMenu); } if (auto neighbor = rootPane->NavigateDirection(activePane, FocusDirection::Up, mruPanes)) { - makeItem(RS_(L"SwapPaneUpText"), neighbor->GetProfile().Icon().Resolved(), ActionAndArgs{ ShortcutAction::SwapPane, SwapPaneArgs{ FocusDirection::Up } }, swapPaneMenu); + makeMenuFlyoutItem(RS_(L"SwapPaneUpText"), neighbor->GetProfile().Icon().Resolved(), ActionAndArgs{ ShortcutAction::SwapPane, SwapPaneArgs{ FocusDirection::Up } }, swapPaneMenu); } if (auto neighbor = rootPane->NavigateDirection(activePane, FocusDirection::Left, mruPanes)) { - makeItem(RS_(L"SwapPaneLeftText"), neighbor->GetProfile().Icon().Resolved(), ActionAndArgs{ ShortcutAction::SwapPane, SwapPaneArgs{ FocusDirection::Left } }, swapPaneMenu); + makeMenuFlyoutItem(RS_(L"SwapPaneLeftText"), neighbor->GetProfile().Icon().Resolved(), ActionAndArgs{ ShortcutAction::SwapPane, SwapPaneArgs{ FocusDirection::Left } }, swapPaneMenu); } makeMenuItem(RS_(L"SwapPaneText"), L"\xF1CB", swapPaneMenu, menu); From 96db85f03694cd48cadfbc01e748f9ad603a1da2 Mon Sep 17 00:00:00 2001 From: Omar Mashal Date: Sun, 6 Sep 2026 16:48:58 +0300 Subject: [PATCH 2/2] Fix context menu crash after split pane submenu --- .../LocalTests_TerminalApp/TabTests.cpp | 48 ------------------- src/cascadia/TerminalApp/TerminalPage.cpp | 2 +- 2 files changed, 1 insertion(+), 49 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index d71b39d0ca7..3bf302108b2 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -84,7 +84,6 @@ namespace TerminalAppLocalTests TEST_METHOD(CloseZoomedPane); TEST_METHOD(SwapPanes); - TEST_METHOD(ContextMenuUsesMenuFlyoutsForSplitAndSwapPane); TEST_METHOD(NextMRUTab); TEST_METHOD(VerifyCommandPaletteTabSwitcherOrder); @@ -1046,53 +1045,6 @@ namespace TerminalAppLocalTests }); } - void TabTests::ContextMenuUsesMenuFlyoutsForSplitAndSwapPane() - { - auto page = _commonSetup(); - - TestOnUIThread([&page]() { - Log::Comment(L"Create a second pane so Swap pane is present."); - page->_SplitPane(nullptr, SplitDirection::Right, 0.5f, page->_MakePane(nullptr, page->_GetFocusedTab(), nullptr)); - - const auto activeControl{ page->_GetActiveControl() }; - VERIFY_IS_NOT_NULL(activeControl); - - winrt::Microsoft::UI::Xaml::Controls::CommandBarFlyout contextMenu{}; - page->_PopulateContextMenu(activeControl, contextMenu, false); - - bool foundSplitPane = false; - bool foundSwapPane = false; - - for (const auto& command : contextMenu.SecondaryCommands()) - { - if (const auto button{ command.try_as() }) - { - if (button.Label() == RS_(L"SplitPaneText")) - { - foundSplitPane = true; - const auto flyout{ button.Flyout() }; - const auto menuFlyout{ flyout.try_as() }; - VERIFY_IS_NOT_NULL(menuFlyout); - VERIFY_IS_GREATER_THAN(menuFlyout.Items().Size(), 0u); - VERIFY_IS_NOT_NULL(menuFlyout.Items().GetAt(0).try_as()); - } - else if (button.Label() == RS_(L"SwapPaneText")) - { - foundSwapPane = true; - const auto flyout{ button.Flyout() }; - const auto menuFlyout{ flyout.try_as() }; - VERIFY_IS_NOT_NULL(menuFlyout); - VERIFY_IS_GREATER_THAN(menuFlyout.Items().Size(), 0u); - VERIFY_IS_NOT_NULL(menuFlyout.Items().GetAt(0).try_as()); - } - } - } - - VERIFY_IS_TRUE(foundSplitPane); - VERIFY_IS_TRUE(foundSwapPane); - }); - } - void TabTests::NextMRUTab() { // This is a test for GH#8025 - we want to make sure that MRU tab diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index ca906639e98..755d204e453 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -5639,11 +5639,11 @@ namespace winrt::TerminalApp::implementation subMenu.Text(label); + // A MenuFlyoutSubItem can't be clicked, so the automatic split is an explicit entry. WUX::Controls::MenuFlyoutItem autoItem{}; autoItem.Text(splitPaneAutomaticText); autoItem.Click(makeCallback(ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ splitType, SplitDirection::Automatic, .5, args } })); subMenu.Items().Append(autoItem); - subMenu.Items().Append(WUX::Controls::MenuFlyoutSeparator{}); makeMenuFlyoutItem(splitPaneRightText, L"", ActionAndArgs{ ShortcutAction::SplitPane, SplitPaneArgs{ splitType, SplitDirection::Right, .5, args } }, subMenu);