Skip to content

Conversation

@FF69B3
Copy link
Contributor

@FF69B3 FF69B3 commented Jan 3, 2026

Note

Adds a window-hosted dialog stack enabling richer, modal dialogs separate from the main window.

  • New WindowedContentDialog control exposes ShowAsync, modal parenting/centering, header image, theming, and button text/styles with keyboard accelerators
  • New ContentDialogWindow (Window) hosts dialog UI and manages sizing, title bar, backdrop, focus, async click handling, and right-tap routing
  • New ContentDialogContent control + XAML template with visual states for button combinations and default button focus; custom measure to size to buttons/resources
  • Underlay support via UnderlayMode, UnderlaySystemBackdropOptions, and UnderlaySmokeLayerOptions (popup-based smoke layer or system backdrop), with resize handling
  • New async/event plumbing: ContentDialogWindowButtonClickEventArgs, WinUIAsyncEventArgs with deferrals; keyboard accelerators wiring
  • Refactors modal behavior into ModalWindowBase, updates ModalWindow to consume it; adds ContentDialogUtils helpers
  • Project files updated to include new headers/IDL/XAML and resources

Written by Cursor Bugbot for commit fef8cba. This will update automatically on new commits. Configure here.

@HO-COOH
Copy link
Owner

HO-COOH commented Jan 4, 2026

1. First thing first, because of the existence of this bug, we cannot directly inherit ModalWindow in the WinUI3Package project. But we can refactor out the "Modal" part to ModalWindowBase.hpp. And use it in the ContentDialogWindow.

@HO-COOH
Copy link
Owner

HO-COOH commented Jan 5, 2026

  1. Move all style related setter into ContentDialogContent_Resource.xaml

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

This PR is being reviewed by Cursor Bugbot

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

else
{
winrt::Microsoft::UI::Xaml::VisualStateManager::GoToState(*this, L"SecondaryVisible", false);
return L"SecondaryAndCloseVisible";
Copy link

Choose a reason for hiding this comment

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

Wrong return value in button visibility state determination

In DetermineButtonsVisibilityState(), when only the secondary button is visible (secondary text is non-empty but close text is empty), the code correctly applies the visual state L"SecondaryVisible" but incorrectly returns L"SecondaryAndCloseVisible". This mismatch between the actual applied state and the returned value stored in buttonsVisibilityState will cause incorrect behavior when other code relies on this state string.

Fix in Cursor Fix in Web


SetModal(parent);
m_parent.Closed(m_OnParentClosed);
m_OnParentClosed = m_parent.Closed({ this, &ContentDialogWindow::OnParentClosed });;
Copy link

Choose a reason for hiding this comment

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

Event unsubscription targets wrong parent window

In SetParent(), SetModal(parent) sets m_parent to the new parent before the event unsubscription on line 495. This means m_parent.Closed(m_OnParentClosed) attempts to unsubscribe the old event token from the new parent window instead of the old parent. The old parent's Closed event handler will never be properly unsubscribed, causing a handler leak and potential unexpected callbacks.

Fix in Cursor Fix in Web


PrimaryButton.Click(m_OnPrimaryButtonClick);
SecondaryButton.Click(m_OnSecondaryButtonClick);
CloseButton.Click(m_OnCloseButtonClick);
Copy link

Choose a reason for hiding this comment

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

Destructor accesses potentially null button objects

The destructor calls .Click() on PrimaryButton, SecondaryButton, and CloseButton to unsubscribe event handlers. However, these members are initialized to nullptr and only assigned in OnApplyTemplate(). If the control is destroyed before the template is applied (e.g., before being loaded into the visual tree), calling .Click() on null objects will cause a crash.

Fix in Cursor Fix in Web

m_parent = parent;
m_closedRevoker = getSelf().Closed(winrt::auto_revoke, [this](auto&&...) {
m_parent.Activate();
});
Copy link

Choose a reason for hiding this comment

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

Null parent causes crash in modal window close handler

The ModalWindowBase::SetModal function registers a Closed handler on the dialog window that calls m_parent.Activate(). However, ContentDialogWindow::OnParentClosed sets m_parent = nullptr when the parent window closes first. If the parent closes before the dialog, and then the dialog closes, the Closed handler will call Activate() on a null m_parent, causing a crash.

Additional Locations (1)

Fix in Cursor Fix in Web

winrt::Windows::Foundation::Collections::IVector<Microsoft::UI::Xaml::Input::KeyboardAccelerator> m_CloseButtonKeyboardAccelerators{ nullptr };

// IsHeaderImage property field
bool _IsHeaderImage;
Copy link

Choose a reason for hiding this comment

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

Uninitialized boolean causes undefined visual state behavior

The member variable _IsHeaderImage is declared but never initialized in the constructor. Since it's a plain bool without an initializer, its value is indeterminate. This variable is read in IsHeaderImage() which is called in OnApplyTemplate(), the Loaded handler, AfterGotFocus(), and AfterLostFocus() to determine visual states. The undefined value could cause random visual state transitions like incorrectly expanding the command space.

Fix in Cursor Fix in Web

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.

2 participants