New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Started ignoring remote keyboard notifications. #17981
Started ignoring remote keyboard notifications. #17981
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment about being more defensive against missing keys. Rest lgtm.
|
||
if (@available(iOS 9, *)) { | ||
// Ignore keyboard notifications related to other apps. | ||
if (![info[UIKeyboardIsLocalUserInfoKey] boolValue]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find guidance in the documentation on whether this key will always be present. The discussion in the docs seems to be around multitasking on iPad. As it stands, if the key is not present in the dictionary, the frame change will be ignored. A more defensive writing of this piece of code would be to check if the key exists and then check its bool value. If the key doesn't exist, process the frame change as normal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. In practice it isn't just iPad, these were the very notifications that were messing us up and I was testing with iPhone. I believe the key is always present (except iOS 8). It doesn't hurt to make it safer.
Landing on red, i kicked the windows bot and it worked but didn't refresh: https://ci.chromium.org/raw/build/logs.chromium.org/flutter/led/aaclarke_google.com/1326160b024176593659a45af74d438aeebd02efa5a09492a5551d68170f7e66/+/annotations?server=chromium-swarm.appspot.com |
Relevant issue: flutter/flutter#55624
This is a more stable alternative to #16270