Implement popup windows for macOS#182371
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for popup windows on both macOS and Windows platforms. It includes the native implementation for creating and managing these windows, the corresponding Dart FFI bindings, and the framework-level PopupWindowController. The text input plugin on macOS has also been updated to correctly function with non-key popup windows. Additionally, the example application has been extended to demonstrate the new popup window functionality. While this is a substantial and valuable feature addition, the review has identified a critical use-after-free bug in the Windows implementation, a high-severity issue where methods were incorrectly placed in the Dart code for Windows, and a few minor copy-paste errors in comments and assertions that should be addressed.
|
@knopp Is this ready for review or should it be a draft? I see the "WIP" in the title. |
It's WIP until I get the CI passing. |
1a728ea to
b304f03
Compare
|
@justinmc, this should be ready for review. |
mattkae
left a comment
There was a problem hiding this comment.
I have a single nit, but looks reasonable to me. Perhaps @justinmc or @loic-sharma may have more to say than me about the API changes
|
|
||
| NSWindow* parent = nil; | ||
|
|
||
| if (request->parent_view_id != 0) { |
There was a problem hiding this comment.
A popup window must always have a parent, right? So this check may be unnecessary
justinmc
left a comment
There was a problem hiding this comment.
I tried running the example on my mac but I got an error when I clicked on "Show Popup". Maybe it's just because I haven't compiled the engine on this branch, I'll try that.
Error message
══╡ EXCEPTION CAUGHT BY GESTURE ╞═══════════════════════════════════════════════════════════════════
The following ArgumentError was thrown while handling a gesture:
Invalid argument(s): Couldn't resolve native function
'InternalFlutter_WindowController_CreatePopupWindow' in
'package:flutter/src/widgets/_window_macos.dart' : No asset with id
'package:flutter/src/widgets/_window_macos.dart' found. No available native assets. Attempted to
fallback to process lookup. dlsym(RTLD_DEFAULT, InternalFlutter_WindowController_CreatePopupWindow):
symbol not found.
| void onWindowDestroyed() { | ||
| onDestroyed(); |
There was a problem hiding this comment.
Nit: It might be too late to change this, but see here about naming this kind of stuff: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#naming-rules-for-typedefs-and-function-variables
I think that means onWindowDestroyed should have been handleWindowDestroyed.
There was a problem hiding this comment.
I don't think we care much about not breaking things right now. I think we can rename it. @mattkae ?
There was a problem hiding this comment.
Ah TIL about that naming convention! These APIs are still experimental, we can break things willy nilly. Feel free to do this in a follow-up pull request though.
There was a problem hiding this comment.
Can you test the new changes to this example in examples/multiple_windows/test?
| /// registered once, so we use a singleton to ensure that. | ||
| class _ElementPositionTrackerManager { | ||
| _ElementPositionTrackerManager._() { | ||
| WidgetsBinding.instance.addPersistentFrameCallback((_) { |
There was a problem hiding this comment.
So every frame, you look up the location of the popup window and report it to macos? Is that in order to keep the popup menu positioned on the correct place on the screen relative to the window that it came from?
There was a problem hiding this comment.
I think this would be easier to understand if I could play around with the example, but I'm getting an error (see main review text).
There was a problem hiding this comment.
The crash - You need to run it with local engine.
The tracker manager - I think this is actually just copied from the tooltip_button.dart, now that I'm looking into that. This should be shared.
The idea is that the Controller API itself only gets an anchor rect, it shouldn't concern itself with position tracking. That's a completely orthogonal problem. The tracker in example is something I built quickly to get the example running and it stuck there. It's by no means the best way to solve the problem.
There was a problem hiding this comment.
So every frame, you look up the location of the popup window and report it to macos? Is that in order to keep the popup menu positioned on the correct place on the screen relative to the window that it came from?
Yes, on every frame the position gets updated. This is to make sure the popup tracks the anchor even if it moves (i.e. during window resizing).
|
|
||
| /// Manages all [_ElementPositionTracker] instances. | ||
| /// | ||
| /// This class is a singleton because it needs to hook into the global frame |
There was a problem hiding this comment.
I'm always wary of global singletons, are you sure this one is necessary or is there another way to do this?
There was a problem hiding this comment.
Sometimes a way around using a singleton is to use an InheritedWidget at the root of the tree instead. I'm not sure if that would be practical here or not.
There was a problem hiding this comment.
The _ElementPositionTrackerManager is now just an implementation detail of ElementPositionTracker. It is a singleton because I don't want to register more than one persistent frame callback and there's no way to unregister them.
I don't think I'd want to pass this down widget hierarchy - the user shouldn't be concerned with it at all.
There was a problem hiding this comment.
My initial impression is that this is quite a lot of boilerplate for a user to set up in order to use a popup window. Do we have any plans in the longer term to put some of this code into Flutter for users to use?
There was a problem hiding this comment.
I moved the ElementPositionTracker to separate file so that it's not copied for each button and simplified the API. The singleton (_ElementPositionTrackerManager) is private and the reason for existence is to only register one persistent frame callback for all trackers, because you can't unregister persistent callbacks.
I don't think the tracker itself is too verbose (89 lines). At this point I'm not sure that's the best way is to track the anchor position, that's why the tracker is part of the example instead of framework.
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
flutter/flutter@3d69471...0f401ee 2026-04-02 matej.knopp@gmail.com Remove isSizedToContent from _window_linux.dart. (flutter/flutter#184506) 2026-04-02 matej.knopp@gmail.com Windows: Get graphics adapter from engine instead of view (flutter/flutter#184479) 2026-04-02 robert.ancell@canonical.com Reduce number of mallocs in FFI call (flutter/flutter#184166) 2026-04-02 robert.ancell@canonical.com Handle events without a device (flutter/flutter#184163) 2026-04-02 robert.ancell@canonical.com Implement tooltip windows on Linux (flutter/flutter#182348) 2026-04-02 engine-flutter-autoroll@skia.org Roll Skia from bdeebacf23c8 to bb9fd8653739 (4 revisions) (flutter/flutter#184494) 2026-04-02 matej.knopp@gmail.com Implement popup windows for macOS (flutter/flutter#182371) 2026-04-02 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from fV-JIWUt4FQGeDtEe... to BFLjk6Uwd0gs_Hkdk... (flutter/flutter#184492) 2026-04-02 victorsanniay@gmail.com Add await or ignore to future-returning methods defined in Dart SDK (flutter/flutter#184229) 2026-04-02 engine-flutter-autoroll@skia.org Roll Dart SDK from 043a2bfd56ff to d84bdfeb45eb (2 revisions) (flutter/flutter#184487) 2026-04-01 engine-flutter-autoroll@skia.org Roll Skia from c2363c39c283 to bdeebacf23c8 (9 revisions) (flutter/flutter#184480) 2026-04-01 ksanaullah383.khan@gmail.com Remove sliver_test_utils cross-import from sliver_app_bar_test (flutter/flutter#184193) 2026-04-01 116356835+AbdeMohlbi@users.noreply.github.com Improve error message when `dart-define` content are not `base64 encoded` and add more test cases (flutter/flutter#184219) 2026-04-01 116356835+AbdeMohlbi@users.noreply.github.com Replace usages of `MediaQuery.of(context).property` with `MediaQuery.propertyOf(context)` (flutter/flutter#184211) 2026-04-01 techonlabs@gmail.com [ios] Add opt-in inline prediction text input support (flutter/flutter#183650) 2026-04-01 jesswon@google.com [fix-forward] fix build_android_host_app_with_module_source integration test (flutter/flutter#184466) 2026-04-01 katelovett@google.com Update style guide (flutter/flutter#184478) 2026-04-01 engine-flutter-autoroll@skia.org Roll Packages from b04f3e5 to b3fcf14 (3 revisions) (flutter/flutter#184474) 2026-04-01 15619084+vashworth@users.noreply.github.com Warn about slow SwiftPM downloads and centralize SwiftPM cache (flutter/flutter#183747) 2026-04-01 15619084+vashworth@users.noreply.github.com Inject FlutterFramework dependency in iOS Add2App swift packages (flutter/flutter#184365) 2026-04-01 1063596+reidbaker@users.noreply.github.com Prepare for skills adoption (flutter/flutter#184129) 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 louisehsu@google.com,stuartmorgan@google.com 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
<!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## 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. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.