Repository navigation
[ios]: Preserve text field focus after 'Look Up' action - #173846
okorohelijah wants to merge 4 commits into
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. |
github-actions
Bot
added
a: text input
![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 addresses a bug on iOS where using the native "Look Up" context menu action would cause the text field to lose focus. The solution involves adding a flag to temporarily prevent the resignFirstResponder call from tearing down the text connection, which is a sound approach. The native implementation looks correct. However, I've identified a critical issue where a corresponding change in the Dart code calls a new method on the native side that doesn't appear to be handled, which would break the feature for Flutter-rendered toolbars. Additionally, there's a minor documentation issue for the new public native APIs.
|
Just wanted to add a brief note on the pr for context. The "Look Up" action originates entirely from the native iOS side, which created a race condition that made a framework-level (Dart) fix difficult. Early attempts to solve this included:
Both approaches failed for the same core reason: the iOS operating system calls resignFirstResponder immediately when the "Look Up" view is presented, tearing down the text connection before any asynchronous logic or cross-plugin notification could effectively intercept it. The tradeoff: The solution's main tradeoff is its dependency on the current presentation behavior of iOS's native look-up view. While it's unlikely Apple will alter this fundamental view controller lifecycle, any future OS updates could potentially require this fix to be revisited. |
LongCatIsLooong
left a comment
LongCatIsLooong
left a comment
There was a problem hiding this comment.
The fix is "Look Up" specific, but I think there's a more generic issue that needs to be addressed (the engine itself is not properly clearing up the clientID after telling the framework that the connection is closed). If we fix that then this fix will stop working because now the _clientID is set to nil and UIKit won't be able to tell this text field to become first responder (see the canBecomeFirstResponder implementation).
So that makes me feel that this fix is more fragile that I initially thought (it kinda relies on an existing bug, plus that there's no guarantee that UIKit always gives the first responder back to the text field).
| NSMutableDictionary<UIScribbleElementIdentifier, NSValue*>* scribbleElements; | ||
|
|
||
| /** | ||
| * A temporary flag to prevent the text field from losing focus when the |
There was a problem hiding this comment.
Could you explain what do you mean by "temporary"?
There was a problem hiding this comment.
Also does this flag have to be added to the interface?
There was a problem hiding this comment.
This is look up specific so I would probably give it a name that's more closely tied to look up (e.g., isPresentingLookUpViewController).
| @@ -1057,6 +1057,7 @@ - (void)flutterTextInputView:(FlutterTextInputView*)textInputView | |||
|
|
|||
| - (void)flutterTextInputView:(FlutterTextInputView*)textInputView | |||
| lookUpSelectedText:(NSString*)selectedText { | |||
There was a problem hiding this comment.
Who calls this method? If it's the text input plugin can you call prepareForLookUp in the caller instead?
|
|
||
| - (BOOL)resignFirstResponder { | ||
| if (self.textInputPlugin.preventUnfocusOnNextResign) { | ||
| self.textInputPlugin.preventUnfocusOnNextResign = NO; |
There was a problem hiding this comment.
I think there are other places that this flag needs to be reset to NO (e.g., hot restart, the framework side closing the text input connection).
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Greetings from stale PR triage! 👋 |
|
This has circled back to stale PR triage, @okorohelijah do you still want to work on this? Otherwise, we will close it for now. Thanks! |
|
IIRC, @LongCatIsLooong and I decided against manually forcing focus back to the TextField because of potential side effects on the focus lifecycle. The main trade-off is that if a system-level event or another process (like a system alert or a timed overlay) is scheduled to take focus while the 'Look Up' view is active, manually returning focus to the TextField upon dismissal could 'steal' that focus. This creates a race condition where the wrong element ends up focused, leading to a confusing user experience. While this specific edge case is rare and largely depends on how iOS manages the view controller lifecycle, a manual fix here is fragile. The more robust approach would be refactoring the engine's 'Look Up' implementation to better align with native behavior, though that is likely out of scope for this specific issue. |
This PR fixes a bug on iOS where using the native "Look Up" context menu on a TextField causes it to lose focus, leaving the keyboard in an unstable state.
The root cause is that presenting the native dictionary view forces the FlutterTextInputView to immediately resignFirstResponder, which tears down the framework's text connection. This PR addresses the issue by completing a dormant, native communication pathway within the engine.
To address this while leaving the Dart API unchanged, this change introduces a two-part native solution:
The FlutterEngine now implements the flutterTextInputView:lookUpSelectedText: delegate method. The native handleLookUpAction was already correctly calling this delegate when the "Look Up" action was triggered, but no implementation existed, causing the event to be unhandled.
Upon receiving this delegate call, the Engine gives a command to the FlutterTextInputPlugin, which sets a temporary flag. This flag intercepts the subsequent resignFirstResponder call, allowing the view to resign focus at the UIKit level while preventing the Flutter text connection from being destroyed.
This ensures that when the "Look Up" view is dismissed, the text field's connection is still active, allowing UIKit to seamlessly restore focus.
Screen.Recording.2025-08-15.at.6.05.09.AM.mov
Fixes #144463
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.