[iOS] Refactor keyboard inset logic into FlutterKeyboardInsetManager#185535
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. |
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
949eae2 to
0378ece
Compare
0378ece to
dffe14b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors keyboard inset and animation logic from FlutterViewController into a new FlutterKeyboardInsetManager class, updating the view controller to delegate these responsibilities and adjusting unit tests. Feedback identifies opportunities to improve coordinate space conversions by using bounds instead of frame and to prevent potential retain cycles by ensuring consistent weak/strong capture patterns in animation and completion blocks.
dffe14b to
c8a7d47
Compare
7e583c3 to
3a575e9
Compare
3a575e9 to
48776e6
Compare
48776e6 to
a72d01a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors iOS keyboard inset management by extracting the logic from FlutterViewController into a new FlutterKeyboardInsetManager class. This manager handles keyboard notifications, calculates insets for various modes (docked, floating, multitasking), and synchronizes animations with the system keyboard using a hidden view and VSyncClient. Review feedback identifies opportunities to improve header inclusion hygiene in the implementation file, enhance type safety when casting the delegate to a UIViewController, and ensure the internal animation view is correctly re-parented if the delegate's view is reloaded during the animation lifecycle.
a72d01a to
6ed79d7
Compare
6ed79d7 to
a3e1d0d
Compare
a3e1d0d to
f11610a
Compare
|
Alright this is good for a re-review. Just the review suggestions and ran the formatter. |
from `FlutterViewController` into a dedicated internal keyboard inset manager class. This is one in a series of refactorings that aim to slim down `FlutterViewController`. The goal is to improve maintainability/readability, make it easier to reason about the code, and allow for deterministic testing of keyboard geometry without a full view controller lifecycle. Background for future archaeologists: iOS doesn't provide us with a frame-by-frame callback for keyboard transitions, but we need to animate our views smoothly to account for keyboard size/position changes. To ensure Flutter's layout animates in perfect sync with the system keyboard, we use a hidden view synchronisation trick: * When a keyboard notification (e.g., `UIKeyboardWillShow`) is received, the manager animates a hidden UIView's frame using the native iOS duration and curve. * A `VSyncClient` tracks the `presentationLayer` of this hidden view on every vsync. * The intermediate positions are then translated into physical pixel insets and sent to the Flutter engine until the animation completes. During testing, I found the rare flake in the old code. I added a `layoutIfNeeded` call to the view animation block to ensure that the `CAAnimation` is instantiated immediately, allowing it to be captured by the VSyncClient and unit tests and prevent these race conditions. Manually verified behaviour on an iPad simulator to ensure that Split View keyboards in split-view scenarios don't trigger Flutter layout shifts. I've also added more detailed doc comments to the code. This change does *not* migrate the class to Swift because it currently relies on C++ types for the vsync client: `fml::RefPtr<fml::TaskRunner>`, and `std::unique_ptr<flutter::FrameTimingsRecorder>`. I'll follow up with a small patch to VSyncClient to make a Swift migration possible. Regardless, extraction and Swift migration are two orthogonal changes, so doing in two or three patches is a good idea. Because we need a valid `uiTaskRunner` during `VSyncCLient` initialisation, I've migrated several tests to use `FlutterEnginePartialMock`. Using a real `FlutterEngine`, involves pretty heavy/potentially flaky shell initialisation just so we can get at the task runners. Instead, the partial mock provides a lightweight runner on the current thread. I'm not at all happy about adding more usage of OCMock, but once we've got it separated out, I can work on generating the fakes we need to move off it. Issue: flutter#157140
f11610a to
eae0d41
Compare
…11595) Manual roll Flutter from 31001886cbe8 to 61fca76dd523 (53 revisions) Manual roll requested by tarrinneal@google.com flutter/flutter@3100188...61fca76 2026-04-27 30870216+gaaclarke@users.noreply.github.com Adds integration test for the FLTEnableSDFs flag for iOS (flutter/flutter#185637) 2026-04-27 97480502+b-luk@users.noreply.github.com Fix sdfs being enabled for MacOS regardless of FLTEnableSDFs value (flutter/flutter#185565) 2026-04-27 97480502+b-luk@users.noreply.github.com Don't use UberSDF for paint with incompatible blend modes (flutter/flutter#184889) 2026-04-27 engine-flutter-autoroll@skia.org Roll Dart SDK from de495e3de9a0 to 941ca325cfc9 (2 revisions) (flutter/flutter#185653) 2026-04-27 jhy03261997@gmail.com [a11y] Add CONTENT_CHANGE_TYPE_EXPANDED support on android. (flutter/flutter#185305) 2026-04-27 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#185641) 2026-04-27 1063596+reidbaker@users.noreply.github.com Modify analyze.dart to have flags to run only or exclude each verification step. (flutter/flutter#185618) 2026-04-27 meylis@divine.video Fix debugNeedsPaint/Layout/CompositedLayerUpdate crashing in release mode (flutter/flutter#184627) 2026-04-27 kallentu@google.com Enable `var_with_no_type_annotation` lint. (flutter/flutter#185215) 2026-04-27 53523825+JhonaCodes@users.noreply.github.com Fix SelectionArea handles overlapping context menu on Android (flutter/flutter#182663) 2026-04-27 30870216+gaaclarke@users.noreply.github.com Adds debugging information to compiled metal shaders (flutter/flutter#185629) 2026-04-27 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#185638) 2026-04-27 84978733+alejandro-all-win-software@users.noreply.github.com Use null-aware elements in dev/devicelab/lib/framework/browser.dart (flutter/flutter#184778) 2026-04-27 154381524+flutteractionsbot@users.noreply.github.com Sync CHANGELOG.md from stable (flutter/flutter#185633) 2026-04-27 31591868+zawhtetnaing10@users.noreply.github.com Added useOriginalColors flag which allows ImageIcon to bypass IconTheme colorization and use the original colors (flutter/flutter#180491) 2026-04-27 engine-flutter-autoroll@skia.org Roll Packages from 8400f71 to 23280da (2 revisions) (flutter/flutter#185619) 2026-04-27 engine-flutter-autoroll@skia.org Roll Skia from f1238e0f1022 to ce82d32b3e03 (1 revision) (flutter/flutter#185616) 2026-04-27 15619084+vashworth@users.noreply.github.com [SwiftPM] Enable package resolution on xcodebuild commands (flutter/flutter#185208) 2026-04-27 chris@bracken.jp [iOS] Refactor keyboard inset logic into FlutterKeyboardInsetManager (flutter/flutter#185535) 2026-04-27 matej.knopp@gmail.com [Win32] FlutterDesktopEngineGetGraphicsAdapter should use out parameter (flutter/flutter#185590) 2026-04-27 engine-flutter-autoroll@skia.org Roll Skia from d77e3356d526 to f1238e0f1022 (4 revisions) (flutter/flutter#185604) 2026-04-27 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from flsn8jC4LkTn6WECf... to i6d0NoDueUiXpePfX... (flutter/flutter#185601) 2026-04-26 engine-flutter-autoroll@skia.org Roll Dart SDK from a108dfe2d227 to de495e3de9a0 (1 revision) (flutter/flutter#185599) 2026-04-26 engine-flutter-autoroll@skia.org Roll Skia from ce9aa2231292 to d77e3356d526 (1 revision) (flutter/flutter#185596) 2026-04-26 engine-flutter-autoroll@skia.org Roll Skia from 622fff4c24d2 to ce9aa2231292 (1 revision) (flutter/flutter#185588) 2026-04-25 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 9fPnyEo9PaNdXtasl... to flsn8jC4LkTn6WECf... (flutter/flutter#185585) 2026-04-25 engine-flutter-autoroll@skia.org Roll Dart SDK from 605a8faf0dda to a108dfe2d227 (1 revision) (flutter/flutter#185584) 2026-04-25 engine-flutter-autoroll@skia.org Roll Dart SDK from 6b8c39765f17 to 605a8faf0dda (1 revision) (flutter/flutter#185578) 2026-04-25 engine-flutter-autoroll@skia.org Roll Skia from 185f6b57d64f to 622fff4c24d2 (1 revision) (flutter/flutter#185573) 2026-04-25 evanwall@buffalo.edu Implement square-like round superellipses with circular corners in the SDF uber shader (flutter/flutter#185370) 2026-04-25 victorsanniay@gmail.com Add @awaitNotRequired annotation to flutter sdk (flutter/flutter#181513) 2026-04-25 engine-flutter-autoroll@skia.org Roll Dart SDK from 01228cb7af42 to 6b8c39765f17 (2 revisions) (flutter/flutter#185569) 2026-04-25 flar@google.com Adapt the DisplayList benchmarks into a primitive rendering benchmark suite (flutter/flutter#185270) 2026-04-25 okorohelijah@google.com Enable SPM for GoogleMobileAds (flutter/flutter#185548) 2026-04-25 engine-flutter-autoroll@skia.org Roll Skia from 3f467a581942 to 185f6b57d64f (1 revision) (flutter/flutter#185564) 2026-04-25 victorsanniay@gmail.com Fix Table crash when a cell child paints below the row bottom (flutter/flutter#185323) 2026-04-24 ahmedsameha1@gmail.com Make sure that an Image doesn't crash in 0x0 environment (flutter/flutter#181154) 2026-04-24 engine-flutter-autoroll@skia.org Roll Skia from 300d432048b0 to 3f467a581942 (3 revisions) (flutter/flutter#185558) 2026-04-24 engine-flutter-autoroll@skia.org Roll Dart SDK from c26627715892 to 01228cb7af42 (4 revisions) (flutter/flutter#185559) 2026-04-24 jacksongardner@google.com Reland "[web] Fix LateInitializationError in CkSurface and SkwasmSurface (#185116)" (flutter/flutter#185553) 2026-04-24 41930132+hellohuanlin@users.noreply.github.com [github]fix git ls-file glob pattern in labeler.yml instruction (flutter/flutter#185495) 2026-04-24 41930132+hellohuanlin@users.noreply.github.com [ios]update ios-reviewers tags to include more files (flutter/flutter#185490) 2026-04-24 tomac@google.com Add initial support for Cross-Origin Storage (flutter/flutter#184149) 2026-04-24 30870216+gaaclarke@users.noreply.github.com tool: Skip cached engine artifacts with local engine (flutter/flutter#185546) ...
…lutter#11595) Manual roll Flutter from 31001886cbe8 to 61fca76dd523 (53 revisions) Manual roll requested by tarrinneal@google.com flutter/flutter@3100188...61fca76 2026-04-27 30870216+gaaclarke@users.noreply.github.com Adds integration test for the FLTEnableSDFs flag for iOS (flutter/flutter#185637) 2026-04-27 97480502+b-luk@users.noreply.github.com Fix sdfs being enabled for MacOS regardless of FLTEnableSDFs value (flutter/flutter#185565) 2026-04-27 97480502+b-luk@users.noreply.github.com Don't use UberSDF for paint with incompatible blend modes (flutter/flutter#184889) 2026-04-27 engine-flutter-autoroll@skia.org Roll Dart SDK from de495e3de9a0 to 941ca325cfc9 (2 revisions) (flutter/flutter#185653) 2026-04-27 jhy03261997@gmail.com [a11y] Add CONTENT_CHANGE_TYPE_EXPANDED support on android. (flutter/flutter#185305) 2026-04-27 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#185641) 2026-04-27 1063596+reidbaker@users.noreply.github.com Modify analyze.dart to have flags to run only or exclude each verification step. (flutter/flutter#185618) 2026-04-27 meylis@divine.video Fix debugNeedsPaint/Layout/CompositedLayerUpdate crashing in release mode (flutter/flutter#184627) 2026-04-27 kallentu@google.com Enable `var_with_no_type_annotation` lint. (flutter/flutter#185215) 2026-04-27 53523825+JhonaCodes@users.noreply.github.com Fix SelectionArea handles overlapping context menu on Android (flutter/flutter#182663) 2026-04-27 30870216+gaaclarke@users.noreply.github.com Adds debugging information to compiled metal shaders (flutter/flutter#185629) 2026-04-27 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#185638) 2026-04-27 84978733+alejandro-all-win-software@users.noreply.github.com Use null-aware elements in dev/devicelab/lib/framework/browser.dart (flutter/flutter#184778) 2026-04-27 154381524+flutteractionsbot@users.noreply.github.com Sync CHANGELOG.md from stable (flutter/flutter#185633) 2026-04-27 31591868+zawhtetnaing10@users.noreply.github.com Added useOriginalColors flag which allows ImageIcon to bypass IconTheme colorization and use the original colors (flutter/flutter#180491) 2026-04-27 engine-flutter-autoroll@skia.org Roll Packages from 8400f71 to 23280da (2 revisions) (flutter/flutter#185619) 2026-04-27 engine-flutter-autoroll@skia.org Roll Skia from f1238e0f1022 to ce82d32b3e03 (1 revision) (flutter/flutter#185616) 2026-04-27 15619084+vashworth@users.noreply.github.com [SwiftPM] Enable package resolution on xcodebuild commands (flutter/flutter#185208) 2026-04-27 chris@bracken.jp [iOS] Refactor keyboard inset logic into FlutterKeyboardInsetManager (flutter/flutter#185535) 2026-04-27 matej.knopp@gmail.com [Win32] FlutterDesktopEngineGetGraphicsAdapter should use out parameter (flutter/flutter#185590) 2026-04-27 engine-flutter-autoroll@skia.org Roll Skia from d77e3356d526 to f1238e0f1022 (4 revisions) (flutter/flutter#185604) 2026-04-27 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from flsn8jC4LkTn6WECf... to i6d0NoDueUiXpePfX... (flutter/flutter#185601) 2026-04-26 engine-flutter-autoroll@skia.org Roll Dart SDK from a108dfe2d227 to de495e3de9a0 (1 revision) (flutter/flutter#185599) 2026-04-26 engine-flutter-autoroll@skia.org Roll Skia from ce9aa2231292 to d77e3356d526 (1 revision) (flutter/flutter#185596) 2026-04-26 engine-flutter-autoroll@skia.org Roll Skia from 622fff4c24d2 to ce9aa2231292 (1 revision) (flutter/flutter#185588) 2026-04-25 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 9fPnyEo9PaNdXtasl... to flsn8jC4LkTn6WECf... (flutter/flutter#185585) 2026-04-25 engine-flutter-autoroll@skia.org Roll Dart SDK from 605a8faf0dda to a108dfe2d227 (1 revision) (flutter/flutter#185584) 2026-04-25 engine-flutter-autoroll@skia.org Roll Dart SDK from 6b8c39765f17 to 605a8faf0dda (1 revision) (flutter/flutter#185578) 2026-04-25 engine-flutter-autoroll@skia.org Roll Skia from 185f6b57d64f to 622fff4c24d2 (1 revision) (flutter/flutter#185573) 2026-04-25 evanwall@buffalo.edu Implement square-like round superellipses with circular corners in the SDF uber shader (flutter/flutter#185370) 2026-04-25 victorsanniay@gmail.com Add @awaitNotRequired annotation to flutter sdk (flutter/flutter#181513) 2026-04-25 engine-flutter-autoroll@skia.org Roll Dart SDK from 01228cb7af42 to 6b8c39765f17 (2 revisions) (flutter/flutter#185569) 2026-04-25 flar@google.com Adapt the DisplayList benchmarks into a primitive rendering benchmark suite (flutter/flutter#185270) 2026-04-25 okorohelijah@google.com Enable SPM for GoogleMobileAds (flutter/flutter#185548) 2026-04-25 engine-flutter-autoroll@skia.org Roll Skia from 3f467a581942 to 185f6b57d64f (1 revision) (flutter/flutter#185564) 2026-04-25 victorsanniay@gmail.com Fix Table crash when a cell child paints below the row bottom (flutter/flutter#185323) 2026-04-24 ahmedsameha1@gmail.com Make sure that an Image doesn't crash in 0x0 environment (flutter/flutter#181154) 2026-04-24 engine-flutter-autoroll@skia.org Roll Skia from 300d432048b0 to 3f467a581942 (3 revisions) (flutter/flutter#185558) 2026-04-24 engine-flutter-autoroll@skia.org Roll Dart SDK from c26627715892 to 01228cb7af42 (4 revisions) (flutter/flutter#185559) 2026-04-24 jacksongardner@google.com Reland "[web] Fix LateInitializationError in CkSurface and SkwasmSurface (#185116)" (flutter/flutter#185553) 2026-04-24 41930132+hellohuanlin@users.noreply.github.com [github]fix git ls-file glob pattern in labeler.yml instruction (flutter/flutter#185495) 2026-04-24 41930132+hellohuanlin@users.noreply.github.com [ios]update ios-reviewers tags to include more files (flutter/flutter#185490) 2026-04-24 tomac@google.com Add initial support for Cross-Origin Storage (flutter/flutter#184149) 2026-04-24 30870216+gaaclarke@users.noreply.github.com tool: Skip cached engine artifacts with local engine (flutter/flutter#185546) ...
This change refactors the keyboard inset and animation tracking logic from
FlutterViewControllerinto a dedicated internal keyboard inset manager class.This is one in a series of refactorings that aim to slim down
FlutterViewController. The goal is to improvemaintainability/readability, make it easier to reason about the code, and allow for deterministic testing of keyboard geometry without a full view controller lifecycle.
Background for future archaeologists:
iOS doesn't provide us with a frame-by-frame callback for keyboard transitions, but we need to animate our views smoothly to account for keyboard size/position changes. To ensure Flutter's layout animates in perfect sync with the system keyboard, we use a hidden view synchronisation trick:
UIKeyboardWillShow) is received, the manager animates a hidden UIView's frame using the native iOS duration and curve.VSyncClienttracks thepresentationLayerof this hidden view on every vsync.During testing, I found the rare flake in the old code. I added a
layoutIfNeededcall to the view animation block to ensure that theCAAnimationis instantiated immediately, allowing it to be captured by the VSyncClient and unit tests and prevent these race conditions.Manually verified behaviour on an iPad simulator to ensure that Split View keyboards in split-view scenarios don't trigger Flutter layout shifts.
I've also added more detailed doc comments to the code.
This change does not migrate the class to Swift because it currently relies on C++ types for the vsync client:
fml::TimePoint,fml::RefPtr<fml::TaskRunner>, andstd::unique_ptr<flutter::FrameTimingsRecorder>Because we need a valid
uiTaskRunnerduringVSyncCLientinitialisation, I've migrated several tests to useFlutterEnginePartialMock. Using a realFlutterEngine, involves pretty heavy/potentially flaky shell initialisation just so we can get at the task runners. Instead, the partial mock provides a lightweight runner on the current thread. I'm not at all happy about adding more usage of OCMock, but once we've got it separated out, I can work on generating the fakes we need to move off it.Issue: #157140
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
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.