Add tooltip windows to the windowing API alongside the window positioning logic #177404
Conversation
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
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
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
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.
… positioning logic (flutter/flutter#177404)
… positioning logic (flutter/flutter#177404)
… positioning logic (flutter/flutter#177404)
… positioning logic (flutter/flutter#177404)
… positioning logic (flutter/flutter#177404)
… 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 matt.kosarek@canonical.com Fix for win32 embedder failing to send all alt key downs to the flutter app (flutter/flutter#179097) 2025-11-26 katelovett@google.com 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 zhongliu88889@gmail.com Add framework-side hitTestBehavior support to Semantics (flutter/flutter#178817) 2025-11-25 engine-flutter-autoroll@skia.org Roll Packages from e019cf9 to cc3dca6 (1 revision) (flutter/flutter#179081) 2025-11-25 matt.kosarek@canonical.com Add tooltip windows to the windowing API alongside the window positioning logic (flutter/flutter#177404) 2025-11-25 matt.kosarek@canonical.com FlutterWindowsView::SendWindowMetrics now reliably sends the display_id (flutter/flutter#179053) 2025-11-25 31859944+LongCatIsLooong@users.noreply.github.com Remove semantics geometry shortcircuit (flutter/flutter#178680) 2025-11-25 bruno.leroux@gmail.com Add an assert message when OverlayEntry.remove is called twice (flutter/flutter#178163) 2025-11-25 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from pOO9Jl9HTLsEmks6y... to nzuAxCJGeJbkZCTkr... (flutter/flutter#179066) 2025-11-25 okorohelijah@google.com Dynamically set MinimumOSVersion in App.framework (flutter/flutter#178253) 2025-11-25 engine-flutter-autoroll@skia.org Roll Skia from d83c30b090f4 to 925c311f4b37 (2 revisions) (flutter/flutter#179060) 2025-11-25 fluttergithubbot@gmail.com Marks Linux build_android_host_app_with_module_aar to be unflaky (flutter/flutter#174864) 2025-11-25 fluttergithubbot@gmail.com Marks Linux_mokey complex_layout__start_up to be unflaky (flutter/flutter#174865) 2025-11-25 vegorov@google.com Manual Dart SDK roll to 3.11.0-169.0.dev (flutter/flutter#179054) 2025-11-25 katelovett@google.com Bump Dart to 3.9 (flutter/flutter#179041) 2025-11-25 engine-flutter-autoroll@skia.org Roll Skia from e298c2f93ebf to d83c30b090f4 (2 revisions) (flutter/flutter#179058) 2025-11-24 30870216+gaaclarke@users.noreply.github.com updated licenses_cpp readme (flutter/flutter#178874) 2025-11-24 engine-flutter-autoroll@skia.org Roll Skia from 43d2020be565 to e298c2f93ebf (5 revisions) (flutter/flutter#179046) 2025-11-24 116356835+AbdeMohlbi@users.noreply.github.com 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 38378650+hgraceb@users.noreply.github.com Disposes test restoration manager when accessed by bindings (flutter/flutter#176519) 2025-11-24 bkonyi@google.com [ Widget Preview ] Always generate scaffold under `$TMP` (flutter/flutter#179039) 2025-11-24 engine-flutter-autoroll@skia.org Roll Packages from e67b6be to e019cf9 (9 revisions) (flutter/flutter#179035) 2025-11-24 1063596+reidbaker@users.noreply.github.com Update CHANGELOG.md for Flutter 3.38.3 (flutter/flutter#178935) 2025-11-24 116356835+AbdeMohlbi@users.noreply.github.com Remove unnecessary `String.valueOf` in `SettingsChannel.java` (flutter/flutter#178590) 2025-11-24 katelovett@google.com Roll pub manually, pick up flutter_lints in examples/api (flutter/flutter#179030) 2025-11-24 engine-flutter-autoroll@skia.org Roll Dart SDK from 24cc9a740bd3 to afca43095efa (1 revision) (flutter/flutter#179019) 2025-11-24 zezohassam@gmail.com 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 bmparr@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
…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.
What's new?
WindowPostiionerlogic to the windowing APIPre-launch Checklist
///).