Add tooltip windows to the windowing API alongside the window positioning logic #177404
Conversation
github-actions
bot
added
the
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces tooltip windows to the windowing API, along with the logic for positioning them. A significant amount of new code has been added, including the TooltipWindowController, TooltipWindow widget, and the WindowPositioner logic. A comprehensive suite of tests for the new window positioning logic has also been included.
My review focuses on improving code clarity, consistency, and maintainability. I've pointed out a few documentation typos and copy-paste errors, including a misleading error message in the Win32 implementation. I've also suggested a minor refactoring in the WindowPositioner to improve code readability and reduce redundancy. Overall, this is a solid addition with good test coverage.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Overall looks good. This kind of positioning stuff is fiddly but I think your way of doing things keeps it pretty clean.
My main concern is with the use of GlobalKeys in the example and how that should be done in the ideal situation, because I want to make sure we set users up to be able to get this right on the happy path.
I see that the framework is positioning the tooltip based on constraints from the platform. Just to make sure, do all platforms provide this kind of constraint size information?
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
I think the only blocker for me is double checking that we really don't want to use Alignment, otherwise this looks good!
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // TODO(mattkae): remove invalid_use_of_internal_member ignore comment when this API is stable. |
There was a problem hiding this comment.
Nit: Maybe link to a GitHub issue.
There was a problem hiding this comment.
I will link to the pre-launch checklist: #177586
| /// satisfy. If the constraints disagree with the current size of the window, | ||
| /// the platform might resize the window to satisfy the new constraints. | ||
| /// | ||
| /// The platform is free to ignore this request. |
There was a problem hiding this comment.
Out of curiosity, is it possible to get the size of the tooltip window after calling this method to see what the size ended up being?
There was a problem hiding this comment.
You can use WindowScope.contentSizeOf
| /// to inject platform-specific logic. | ||
| /// | ||
| /// {@macro flutter.widgets.windowing.experimental} | ||
| @internal |
There was a problem hiding this comment.
Should this be protected?
There was a problem hiding this comment.
No, because we want to enable implementers of the TooltipWindowController interface to call this method. It is public by design
@justinmc After talking with Saviq about this, we've settled that having anchor enums (both child and parent) are important enough to justify the new constructs. When we apply the constraints for off-window tooltips/popups, it will make more sense to do so with predefined anchors. A general For the "teardrop" example that we were discussing earlier, one could very easily use the anchors + offset to position the tooltip on the end of the teardrop. Another reason is that we can't currently express the So our vote is leaving the enums for now, unless we use the API and see a good reason why we should switch to Alignment :) Let me know what you think! We can always come back to it if we decide it's not working for us |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
I'm on board with the WindowPositionerAnchor enum instead of Alignment, especially given that it more closely maps to Waylands implementation. Thanks for all of the changes.
…ow positioning logic (flutter/flutter#177404)
…ow positioning logic (flutter/flutter#177404)
…ow positioning logic (flutter/flutter#177404)
…ow positioning logic (flutter/flutter#177404)
…ow positioning logic (flutter/flutter#177404)
…ow positioning logic (flutter/flutter#177404)
…ning logic (flutter#177404) ## What's new? - Added tooltips to the windowing API, but they have no implementation for the time being - Added the `WindowPostiioner` logic to the windowing API - Wrote a lot of tests 🧪 ## 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: # packages/flutter/lib/src/widgets/_window_win32.dart
flutter/flutter@3f553f6...7b98d50 2025-11-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix for win32 embedder failing to send all alt key downs to the flutter app (#179097)" (flutter/flutter#179136) 2025-11-26 [email protected] Fix for win32 embedder failing to send all alt key downs to the flutter app (flutter/flutter#179097) 2025-11-26 [email protected] Modernize framework lints (flutter/flutter#179089) 2025-11-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add framework-side hitTestBehavior support to Semantics (#178817)" (flutter/flutter#179100) 2025-11-25 [email protected] Add framework-side hitTestBehavior support to Semantics (flutter/flutter#178817) 2025-11-25 [email protected] Roll Packages from e019cf9 to cc3dca6 (1 revision) (flutter/flutter#179081) 2025-11-25 [email protected] Add tooltip windows to the windowing API alongside the window positioning logic (flutter/flutter#177404) 2025-11-25 [email protected] FlutterWindowsView::SendWindowMetrics now reliably sends the display_id (flutter/flutter#179053) 2025-11-25 [email protected] Remove semantics geometry shortcircuit (flutter/flutter#178680) 2025-11-25 [email protected] Add an assert message when OverlayEntry.remove is called twice (flutter/flutter#178163) 2025-11-25 [email protected] Roll Fuchsia Linux SDK from pOO9Jl9HTLsEmks6y... to nzuAxCJGeJbkZCTkr... (flutter/flutter#179066) 2025-11-25 [email protected] Dynamically set MinimumOSVersion in App.framework (flutter/flutter#178253) 2025-11-25 [email protected] Roll Skia from d83c30b090f4 to 925c311f4b37 (2 revisions) (flutter/flutter#179060) 2025-11-25 [email protected] Marks Linux build_android_host_app_with_module_aar to be unflaky (flutter/flutter#174864) 2025-11-25 [email protected] Marks Linux_mokey complex_layout__start_up to be unflaky (flutter/flutter#174865) 2025-11-25 [email protected] Manual Dart SDK roll to 3.11.0-169.0.dev (flutter/flutter#179054) 2025-11-25 [email protected] Bump Dart to 3.9 (flutter/flutter#179041) 2025-11-25 [email protected] Roll Skia from e298c2f93ebf to d83c30b090f4 (2 revisions) (flutter/flutter#179058) 2025-11-24 [email protected] updated licenses_cpp readme (flutter/flutter#178874) 2025-11-24 [email protected] Roll Skia from 43d2020be565 to e298c2f93ebf (5 revisions) (flutter/flutter#179046) 2025-11-24 [email protected] Refactor `_isLabel` method in `stepper.dart` to use `any` for better readablity (flutter/flutter#178909) 2025-11-24 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 5 to 6 in the all-github-actions group (flutter/flutter#179049) 2025-11-24 [email protected] Disposes test restoration manager when accessed by bindings (flutter/flutter#176519) 2025-11-24 [email protected] [ Widget Preview ] Always generate scaffold under `$TMP` (flutter/flutter#179039) 2025-11-24 [email protected] Roll Packages from e67b6be to e019cf9 (9 revisions) (flutter/flutter#179035) 2025-11-24 [email protected] Update CHANGELOG.md for Flutter 3.38.3 (flutter/flutter#178935) 2025-11-24 [email protected] Remove unnecessary `String.valueOf` in `SettingsChannel.java` (flutter/flutter#178590) 2025-11-24 [email protected] Roll pub manually, pick up flutter_lints in examples/api (flutter/flutter#179030) 2025-11-24 [email protected] Roll Dart SDK from 24cc9a740bd3 to afca43095efa (1 revision) (flutter/flutter#179019) 2025-11-24 [email protected] Pass EXCLUDED_ARCHS from Xcode project to xcodebuild for macOS builds (flutter/flutter#176948) 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
…ning logic (flutter#177404) ## What's new? - Added tooltips to the windowing API, but they have no implementation for the time being - Added the `WindowPostiioner` logic to the windowing API - Wrote a lot of tests 🧪 ## 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.
…ning logic (flutter#177404) ## What's new? - Added tooltips to the windowing API, but they have no implementation for the time being - Added the `WindowPostiioner` logic to the windowing API - Wrote a lot of tests 🧪 ## 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.
6 participants
What's new?
WindowPostiionerlogic to the windowing APIPre-launch Checklist
///).