Skip to content

Fix context menu crash after split pane submenu - #20612

Open
OmarMashal (OmarMashal0) wants to merge 2 commits into
microsoft:mainfrom
OmarMashal0:fix/20570-context-menu-crash
Open

Fix context menu crash after split pane submenu#20612
OmarMashal (OmarMashal0) wants to merge 2 commits into
microsoft:mainfrom
OmarMashal0:fix/20570-context-menu-crash

Conversation

@OmarMashal0

Copy link
Copy Markdown

Summary of the Pull Request

Fixes the context menu crash reported in #20570 after interacting with the Split pane submenu and collapsing/re-expanding the root menu.

This replaces the nested CommandBarFlyout child menus with standard MenuFlyout controls.

References and Relevant Issues

Closes #20570

Related: #20332

The repro from #20332 also no longer occurs on the patched build.

Detailed Description of the Pull Request / Additional comments

  • Replaces the nested Split pane and Swap pane CommandBarFlyout menus with MenuFlyout.
  • Uses MenuFlyoutSubItem for duplicate/profile split entries, with an explicit Automatic option followed by the directional split options.
  • Keeps the top-level context menu as a CommandBarFlyout.
  • Adds the localized Automatic label.
  • Adds a structural regression test verifying the Split and Swap child flyout types.

Validation Steps Performed

Video

issues_20570_20332.mp4

PR Checklist

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

const auto& subMenu,
auto& targetMenu) {
AppBarButton button{};
auto makeMenuFlyoutItem = [&makeCallback](const winrt::hstring& label,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your AI wrote this PR, right? Can you ask it to minimize the diff please? It's a bug fix after all. 🙂

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, I agree the diff was a bit large for a bug fix
I had an idea about what was causing the crash and used AI to help me implement it, then reviewed and tested the changes carefully

I tried a few ways to reduce the production changes, but the nested CommandBarFlyouts need to become MenuFlyouts, so some of the code has to change because the APIs are different. I mainly reduced the diff by removing the extra structural test
If you see a smaller or cleaner approach, I’d really appreciate your guidance. I’d be happy to update it again if you have any suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The show/hide toggle button from the context menu freezes and crashes the whole terminal (all tabs, all windows)

2 participants