Have FocusManager respond to app lifecycle state changes
#142930
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). 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. |
github-actions
bot
added
framework
|
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. |
|
@gspencergoog the Thanks for your help, and let me know if we need anything else! |
| } else if (_primaryFocus != rootScope) { | ||
| assert(_focusDebug(() => 'suspending $_primaryFocus')); | ||
| _markedForFocus = rootScope; | ||
| _suspendedNode = _primaryFocus; |
There was a problem hiding this comment.
What should happen if something changes the focus while the app is not in "resumed" mode? (Imagine a timer being set to return focus to something at a later time.) Should setting the primary focus to something other than the rootScope modify the _suspendedNode, or should it clear it and set the primary focus?
There was a problem hiding this comment.
That's a really good question. I'm not aware of an app that changes focus on a timer, so there probably isn't a precedent to follow.
But if a developer implements a feature that can change the focus without user input, it probably makes sense to just let it happen, i.e. null out the _suspendedNode and set the primary focus. I'll update this function accordingly.
There was a problem hiding this comment.
Well, it doesn't have to be as simple as a timer, I was just thinking of the simplest way to describe the issue. For a more real-world case, Imagine a slide-in animation that sets the focus on a text field when the animation is done. If you switch the focus away during the animation, this would also cause the same problem.
Your solution looks good, though.
There was a problem hiding this comment.
One thing just occurred to me, though: we might not want to expose didChangeAppLifecycleState as a public function on the FocusManager, since it's not really something that should be called outside of the binding. Perhaps it would be better to aggregate instead of inherit here: create a private class that extends WidgetsBindingObserver and let the FocusManager have a private instance of that class.
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
Thanks for doing this!
loic-sharma
left a comment
loic-sharma
left a comment
There was a problem hiding this comment.
Thanks for the excellent contribution!
Co-authored-by: Loïc Sharma <[email protected]>
gspencergoog
added
the
autosubmit
Previously we merged #142930, to solve issue #87061. Since then, I discovered that the keyboard input wasn't being captured after the app had been paused and resumed. After some digging, I realized that the problem was due to [a line in editable_text.dart](https://github.com/flutter/flutter/blob/d4b1b6e744ba6196e14fb49904f07a4aea4d5869/packages/flutter/lib/src/widgets/editable_text.dart#L3589) that called the `focusNode.consumeKeyboardToken()` method. Luckily, it's a very easy fix: we just use `requestFocus()` instead of `applyFocusChangesIfNeeded()`. @gspencergoog could you take a look when you have a chance?
|
This PR appears to be causing problems on some mobile devices where trying to type on the keyboard causes the text field to lose focus, and then the keyboard goes away. We may need to revert this until we figure out the cause. I'll see if I can get more information and a reproducible test case. (b/327733085) |
|
That's unfortunate to hear. If the keyboard can trigger a lifecycle state change on some devices but not on others, hopefully that can get fixed soon. I agree that for now it makes sense to revert this merge, or maybe add a platform check when activating the listener. |
|
I like the idea of a platform check more than just reverting it. This is a good change for desktop platforms especially. Let's wait and see what the team comes up with for a reproduction case so we can see what the extent of it is. So far it seems to be on specific mobile devices only. My suspicion is that there's an OEM onscreen keyboard that isn't coded similarly to other keyboards, and acts more like a separate app, causing a lifecycle change when it takes focus. |
This PR implements a temporary fix for the mobile device keyboard bug reported in [this comment](#142930 (comment)). CC @gspencergoog
…us (#148067) This change fixes an issue where SelectionArea would clear its selection when the application window lost focus by first checking if the application is running. This is needed because `FocusManager` is aware of the application lifecycle as of #142930 , and triggers a focus lost if the application is not active. Also fixes an issue where the `FocusManager` was not being reset on tests at the right time, causing it always to build with `TargetPlatform.android` as its context.
…us (flutter#148067) This change fixes an issue where SelectionArea would clear its selection when the application window lost focus by first checking if the application is running. This is needed because `FocusManager` is aware of the application lifecycle as of flutter#142930 , and triggers a focus lost if the application is not active. Also fixes an issue where the `FocusManager` was not being reset on tests at the right time, causing it always to build with `TargetPlatform.android` as its context.
3 participants
fixes #87061
It doesn't matter whether I'm using Google Chrome, VS Code, Discord, or a Terminal window: any time a text cursor is blinking, it means that the characters I type will show up there.
And this isn't limited to text fields: if I repeatedly press
Tabto navigate through a website, there's a visual indicator that goes away if I click away from the window, and it comes back if I click orAlt+Tabback into it.Example (Chrome):
This PR adds the same functionality to Flutter apps:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.