Skip to content

Implement dialog windows for the win32 platform #176309

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

Jump to bottom
Merged
mattkae merged 9 commits into from
Oct 16, 2025
Merged

Implement dialog windows for the win32 platform #176309

mattkae merged 9 commits into from
Oct 16, 2025

Conversation

@mattkae
Copy link
Contributor

@mattkae mattkae commented Sep 30, 2025

What's new?

  • Implemented DialogWindowControllerWin32 for win32 dialogs 🚀
  • Refactored and updated the Win32 embedder to support dialogs
  • Updated the multiple_windows example to demonstrate both modal and modeless dialogs
  • Added integration tests for the dialog windows
  • Added tests for dialogs in the embedder

How to test?

  1. Run the multiple_windows example application with a local engine built from this pull request
  2. Click the Modeless Dialog creation button on the main window
  3. Open a regular window and click the Modal Dialog creation button
  4. Note the behavior of modal and modeless dialogs

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine related. See also e: labels. platform-windows Building on or for Windows specifically d: examples Sample code and demos a: desktop Running on desktop labels Sep 30, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Sep 30, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for dialog windows on the Win32 platform by refactoring HostWindow into a base class with HostWindowRegular and HostWindowDialog subclasses. This is a solid architectural change that also adds modal and modeless dialog capabilities, complete with updates to the multi-window example and new integration tests. My review focuses on a few areas for improvement, including focus management logic, adherence to platform conventions for dialogs, documentation, and a minor typo in the example code.

justinmc
justinmc reviewed Oct 1, 2025
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

Looks as expected to me! Loic should probably review the engine code when he's back.

Can you add simple unit tests for the changes in _window_win32.dart? Assuming you can avoid bumping up against the FFI calls. I think you should be able to test that createDialogWindowController now returns a controller instead of throwing at least, and hopefully test the methods on DialogWindowControllerWin32 itself.

I'm going to try to get the example running on my Windows machine tonight, will post back.

@mattkae
Copy link
Contributor Author

mattkae commented Oct 2, 2025

Can you add simple unit tests for the changes in _window_win32.dart? Assuming you can avoid bumping up against the FFI calls. I think you should be able to test that createDialogWindowController now returns a controller instead of throwing at least, and hopefully test the methods on DialogWindowControllerWin32 itself.

I had tried this previously when I did the regular windows bits, but to no avail. There are too many things that we'd have to mock to have a meaningful unit test there. The integration tests should serve as the test of this API instead, as they are truer to reality in this regard.

@mattkae mattkae requested a review from justinmc October 2, 2025 13:27
loic-sharma
loic-sharma reviewed Oct 6, 2025
View reviewed changes
engine/src/flutter/shell/platform/windows/host_window.cc
FML_LOG(ERROR) << "Failed to create view";
return nullptr;
}
FML_CHECK(view != nullptr);
Copy link
Member

@loic-sharma loic-sharma Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I slightly prefer the previous model where we would try to not crash if the window could not be created (I assume this resulted in an exception on the Dart side). Should we stick to that approach? (Not a blocker for landing this PR).

Copy link
Contributor Author

@mattkae mattkae Oct 7, 2025

Choose a reason for hiding this comment

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

@knopp and I spoke about this earlier, but a failure to make a view is most likely a fatal error and a precondition to the engine working properly. We decided that it should be asserted rather than handled, since it would be breaking your app anyway

loic-sharma
loic-sharma reviewed Oct 6, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 6, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 6, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 6, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 6, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 7, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 7, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 7, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 7, 2025
View reviewed changes
@mattkae mattkae requested a review from loic-sharma October 7, 2025 19:15
loic-sharma
loic-sharma reviewed Oct 8, 2025
View reviewed changes
loic-sharma
loic-sharma reviewed Oct 8, 2025
View reviewed changes
@justinmc
Copy link
Contributor

justinmc commented Oct 15, 2025

I failed to get this running again. Last time et build was working for me but I couldn't get it to run. This time I tried to rebuild the engine and even et build wasn't working (something about a file in the perfetto directory was missing). I need to spend more time on it but don't let me hold up this PR.

@mattkae
Copy link
Contributor Author

mattkae commented Oct 15, 2025

I failed to get this running again. Last time et build was working for me but I couldn't get it to run. This time I tried to rebuild the engine and even et build wasn't working (something about a file in the perfetto directory was missing). I need to spend more time on it but don't let me hold up this PR.

I'm currently busy getting CI to stop failing, so you may have time to try 😆 But I will land beforehand regardless. We can make changes to the example app after if need be

@mattkae mattkae added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Oct 15, 2025
robert-ancell
robert-ancell approved these changes Oct 15, 2025
View reviewed changes
Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

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

I think a fix needed to stop the test failing on Linux, but otherwise just pedantic stuff.

auto-merge was automatically disabled October 16, 2025 06:49

Head branch was pushed to by a user without write access

@mattkae mattkae enabled auto-merge October 16, 2025 14:40
@mattkae mattkae added this pull request to the merge queue Oct 16, 2025
Merged via the queue into flutter:master with commit fe409cf Oct 16, 2025
184 checks passed
@mattkae mattkae deleted the dialogs-win32 branch October 16, 2025 16:05
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 16, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
@engine-flutter-autoroll
fe409cfca Implement dialog windows for the win32 platform (flutter/fl…
d46116a 
…utter#176309)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 16, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
@engine-flutter-autoroll
fe409cfca Implement dialog windows for the win32 platform (flutter/fl…
3b616ad 
…utter#176309)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 16, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
@engine-flutter-autoroll
fe409cfca Implement dialog windows for the win32 platform (flutter/fl…
c19102b 
…utter#176309)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 16, 2025
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
@engine-flutter-autoroll
fe409cfca Implement dialog windows for the win32 platform (flutter/fl…
6c18596 
…utter#176309)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 16, 2025
@engine-flutter-autoroll
Manual roll Flutter from 7cd821c21093 to a873a27309b0 (22 revisions) (#…
3747006 
…10244)

Manual roll requested by [email protected]

flutter/flutter@7cd821c...a873a27

2025-10-16 [email protected] [tool] makes listing a shader also as an asset a build failure (flutter/flutter#176866)
2025-10-16 [email protected] Roll Packages from d062181 to 835dccb (7 revisions) (flutter/flutter#177100)
2025-10-16 [email protected] Handle the new location of Perfetto in create_updated_flutter_deps.py (flutter/flutter#177099)
2025-10-16 [email protected] Implement dialog windows for the win32 platform (flutter/flutter#176309)
2025-10-16 [email protected] `SelectableRegion` should not show flutter rendered context menu when web context menu is enabled (flutter/flutter#176855)
2025-10-16 [email protected] Manual roll Skia to 2d9df7c70b6f (flutter/flutter#177074)
2025-10-16 [email protected] feat: add `OptionsViewOpenDirection.mostSpace` to `RawAutocomplete` (flutter/flutter#172997)
2025-10-16 [email protected] [Android] Refactor `ImageReaderSurfaceProducer` restoration after app resumes (flutter/flutter#175937)
2025-10-15 [email protected] Refactor: migrate fade upwards page transition builder to widgets (flutter/flutter#175560)
2025-10-15 [email protected] Marks Windows windowing_test to be unflaky (flutter/flutter#176701)
2025-10-15 [email protected] fix: 🐛 Add equality and hashCode implementations to ScrollAwareImageProvider (flutter/flutter#175038)
2025-10-15 [email protected] Updates `sliver_tree.1.dart‎` to use `MediaQuery.widthOf(context)`  (flutter/flutter#176888)
2025-10-15 [email protected] [web] Fix focus issues in newer versions of Chrome (flutter/flutter#176938)
2025-10-15 [email protected] [Android 16] Update `android_engine_vulkan_tests` to Test Against SDK 36 Emulator (flutter/flutter#176985)
2025-10-15 [email protected] Fix key events interception by RadioGroup when no Radio is focused. (flutter/flutter#176335)
2025-10-15 [email protected] Update cherry-pick instructions to include instructions for pre-release CPs (flutter/flutter#177020)
2025-10-15 [email protected] Manual roll Skia to c501c727a007 (flutter/flutter#177015)
2025-10-15 [email protected] Update examples to latest Linux runner style (flutter/flutter#177033)
2025-10-15 [email protected] [material/menu_anchor.dart] Create internal menu controller if external controller is changed to null. (flutter/flutter#176375)
2025-10-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix - TalkBack does not announce list information (#174374)" (flutter/flutter#177062)
2025-10-14 [email protected] Implement Regular Windows for Linux (flutter/flutter#176187)
2025-10-14 [email protected] Fix - TalkBack does not announce list information (flutter/flutter#174374)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@jtmcdole
Copy link
Member

jtmcdole commented Oct 17, 2025

This change looks to be super breaking the tree. DialogCanNeverBeFullscreen is the last message I see in several of the failing runs.

Can you please take over #177172 and find out why this test or the underlying code is having issues?

@mattkae
Copy link
Contributor Author

mattkae commented Oct 17, 2025

This change looks to be super breaking the tree. DialogCanNeverBeFullscreen is the last message I see in several of the failing runs.

Can you please take over #177172 and find out why this test or the underlying code is having issues?

Yup! I'll look today

bufffun pushed a commit to bufffun/flutter that referenced this pull request Nov 26, 2025
@mattkae @bufffun
Implement dialog windows for the win32 platform (flutter#176309)
bd56c82 
## What's new?
- Implemented `DialogWindowControllerWin32` for win32 dialogs 🚀
- Refactored and updated the Win32 embedder to support dialogs
- Updated the `multiple_windows` example to demonstrate both modal and
modeless dialogs
- Added integration tests for the dialog windows
- Added tests for dialogs in the embedder

## How to test?
1. Run the `multiple_windows` example application with a local engine
built from this pull request
2. Click the `Modeless Dialog` creation button on the main window
3. Open a regular window and click the `Modal Dialog` creation button
4. Note the behavior of modal and modeless dialogs

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.
# Conflicts:
#	engine/src/flutter/shell/platform/windows/host_window.cc
reidbaker pushed a commit to AbdeMohlbi/flutter that referenced this pull request Dec 10, 2025
@mattkae @reidbaker
Implement dialog windows for the win32 platform (flutter#176309)
49b9a2a 
## What's new?
- Implemented `DialogWindowControllerWin32` for win32 dialogs 🚀
- Refactored and updated the Win32 embedder to support dialogs
- Updated the `multiple_windows` example to demonstrate both modal and
modeless dialogs
- Added integration tests for the dialog windows
- Added tests for dialogs in the embedder

## How to test?
1. Run the `multiple_windows` example application with a local engine
built from this pull request
2. Click the `Modeless Dialog` creation button on the main window
3. Open a regular window and click the `Modal Dialog` creation button
4. Note the behavior of modal and modeless dialogs

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robert-ancell robert-ancell robert-ancell approved these changes

@justinmc justinmc justinmc approved these changes

@loic-sharma loic-sharma loic-sharma approved these changes

@knopp knopp Awaiting requested review from knopp

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: desktop Running on desktop d: examples Sample code and demos engine flutter/engine related. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. platform-windows Building on or for Windows specifically

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mattkae @loic-sharma @scnufan @kjxbyz @justinmc @jtmcdole @robert-ancell