Fixes FocusHighlightMode on Android when typing in software keyboard#180753
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses an issue on Android where typing on the software keyboard incorrectly changes the FocusHighlightMode. The fix introduces a check to identify key events from the software keyboard using Android-specific flags and device IDs, preventing these events from altering the highlight mode. A corresponding test case has been added to ensure the fix works as expected.
|
|
||
| // ignore: use_if_null_to_convert_nulls_to_bools | ||
| if (_lastInteractionRequiresTraditionalHighlights != false) { | ||
| if (!isFromVirtualKeyboard && _lastInteractionRequiresTraditionalHighlights != false) { |
There was a problem hiding this comment.
To improve readability and adhere to the Effective Dart style guide, you can use the null-coalescing operator (??) to handle the nullable boolean. This also allows for the removal of the use_if_null_to_convert_nulls_to_bools lint ignore.
if (!isFromVirtualKeyboard && (_lastInteractionRequiresTraditionalHighlights ?? true)) {References
- The Effective Dart: Style guide recommends using the
??operator to convert null to a boolean value, instead of comparing with!= false. (link)
Yes it looks like you're correct. I suspect we intended to name that |
|
I'm ramping up on our software vs physical keyboard APIs. Gemini says:
My current understandingThe goal of Most virtual key presses do not send Currently, We need to update This approach seems reasonable. However, ideally we'd avoid using deprecated APIs to detect virtual key presses. I'll check with my co-workers to see if they have any ideas there. |
Yeah, looks like I messed up the last-minute rename when I landed #162417. I think this should be named |
loic-sharma
left a comment
There was a problem hiding this comment.
This looks good to me, but I left some minor comments to leave ourselves breadcrumbs to make it easier for us to migrate this logic to HardwareKeyboard in the future.
|
FYI it looks like you have some test failures too: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8693133127529602865/+/u/run_test.dart_for_web_canvaskit_tests_shard_and_subshard_4/stdout |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
LGTM with some small nits and modulo @loic-sharma comments.
|
Hello Flutter team thanks for your review, I'll address comments and update this PR this week |
loic-sharma
left a comment
There was a problem hiding this comment.
I'm heading out on vacation for two weeks, so I'll approve this now even though there are still test failures.
|
@romaingyh FYI there are still a few failures here. |
|
@justinmc I think the web tests fail because my fix is based on flutter/packages/flutter/lib/src/services/raw_keyboard.dart Lines 403 to 425 in eba2fbc As the fix needs |
@romaingyh Yup that's reasonable. Please make sure to add a comment to explain why we skip the test on web though, something like: skip: kIsWeb, // [intended] web is incompatible with Android key events. |
|
Thanks @romaingyh for the wonderful contribution! 🎉 |
|
autosubmit label was removed for flutter/flutter/180753, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
Roll Flutter from d3dd7744e81f to d18214307703 (33 revisions) flutter/flutter@d3dd774...d182143 2026-03-06 [email protected] Roll Packages from 8d5c5cd to fe3de64 (2 revisions) (flutter/flutter#183308) 2026-03-06 [email protected] Roll Dart SDK from 1b51451cdb99 to 7c7c1e3d024d (2 revisions) (flutter/flutter#183294) 2026-03-06 [email protected] Roll Dart SDK from 9ac06cdd1801 to 1b51451cdb99 (9 revisions) (flutter/flutter#183289) 2026-03-06 [email protected] Add GitHub workflows to assist with release tasks (flutter/flutter#181978) 2026-03-06 [email protected] [Impeller] Fix new convex path shadow generation in perspective (flutter/flutter#183187) 2026-03-06 [email protected] Roll pub packages (flutter/flutter#183178) 2026-03-05 [email protected] fix: use double quotes in settings.gradle.kts template (flutter/flutter#183081) 2026-03-05 [email protected] Add fallbackColor for PredictiveBackPageTransitionBuilder and PredictiveBackFullscreenPageTransitionBuilder (flutter/flutter#182690) 2026-03-05 [email protected] Simplify TesterContextGLES (multithreading logic not needed), and enable some tests that now pass (flutter/flutter#183250) 2026-03-05 [email protected] Roll Skia from a94df1cdabb0 to a69ef43650ee (14 revisions) (flutter/flutter#183280) 2026-03-05 [email protected] Windowing implementation of `showDialog` that uses a native desktop window to display the content (flutter/flutter#181861) 2026-03-05 [email protected] Build CocoaPod plugin frameworks for Add to App FlutterPluginRegistrant (flutter/flutter#183239) 2026-03-05 [email protected] Extend the Linux web_skwasm_tests_1 timeout to 45 minutes (flutter/flutter#183247) 2026-03-05 [email protected] Update Dart to 3.12 beta 2 (flutter/flutter#183251) 2026-03-05 [email protected] Replace the rest of the references to `flutter/engine` with `flutter/flutter` (flutter/flutter#182938) 2026-03-05 [email protected] chore: convert android_verified_input to pub-workspace (flutter/flutter#183175) 2026-03-05 [email protected] Add await to flutter_test callsites (flutter/flutter#182983) 2026-03-05 [email protected] [iOS] Skip gesture recognizer reset workaround on iOS 26+ (flutter/flutter#183186) 2026-03-05 [email protected] Add warning for plugins not migrated to UIScene (flutter/flutter#182826) 2026-03-05 [email protected] Roll Fuchsia Linux SDK from JJw5EJ87vLGqFVl4h... to 8ay15_eQOEgPHCypm... (flutter/flutter#183255) 2026-03-05 [email protected] Roll Skia from ada0b7628c79 to a94df1cdabb0 (2 revisions) (flutter/flutter#183249) 2026-03-05 [email protected] Roll Packages from 82baf93 to 8d5c5cd (2 revisions) (flutter/flutter#183269) 2026-03-05 [email protected] Add `UnlabaledLeafNodeEvaluation` (flutter/flutter#182872) 2026-03-04 [email protected] Re-specify the ndk version in various test apps, to prevent ndk download (flutter/flutter#183134) 2026-03-04 [email protected] Eliminate rebuilds for Scaffold FAB animation (flutter/flutter#182331) 2026-03-04 [email protected] Add Michal Kucharski to AUTHORS (flutter/flutter#182366) 2026-03-04 [email protected] Merge changelog from 3.41.4 stable. (flutter/flutter#183243) 2026-03-04 [email protected] Allow stylus support on windows (flutter/flutter#165323) 2026-03-04 [email protected] Fix docs on SingletonFlutterWindow.supportsShowingSystemContextMenu (flutter/flutter#183142) 2026-03-04 [email protected] Roll Packages from 9083bc9 to 82baf93 (5 revisions) (flutter/flutter#183240) 2026-03-04 [email protected] Fixes FocusHighlightMode on Android when typing in software keyboard (flutter/flutter#180753) 2026-03-04 [email protected] Make compileShader() retry without sksl if it fails with sksl. (flutter/flutter#183146) 2026-03-04 [email protected] [web] Use pointer-events: auto for non-interactive leaf semantics nodes (flutter/flutter#183077) 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 [email protected],[email protected] 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: ...
…lutter#180753) When typing in a textfield on android mobile, the backspace key produces a KeyEvent message that switches the FocusHighlightMode from touch to traditional. This PR ignores key messages when raw data include the FLAG_SOFT_KEYBOARD or when the device id is VIRTUAL_KEYBOARD Fixes flutter#180746 I know it uses deprecated APIs `RawKeyEvent` and `RawKeyEventData` but I couldn't find any other solutions and the initial system is also based on deprecated `KeyMessage`. Side note: I think the var `_lastInteractionRequiresTraditionalHighlights` name is a bit misleading. When it's true the mode is switched to touch and when it's false to traditional. Shouldn't it be the opposite? ## 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. - [ ] 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Justin McCandless <[email protected]>
When typing in a textfield on android mobile, the backspace key produces a KeyEvent message that switches the FocusHighlightMode from touch to traditional.
This PR ignores key messages when raw data include the FLAG_SOFT_KEYBOARD or when the device id is VIRTUAL_KEYBOARD
Fixes #180746
I know it uses deprecated APIs
RawKeyEventandRawKeyEventDatabut I couldn't find any other solutions and the initial system is also based on deprecatedKeyMessage.Side note:
I think the var
_lastInteractionRequiresTraditionalHighlightsname is a bit misleading. When it's true the mode is switched to touch and when it's false to traditional. Shouldn't it be the opposite?Pre-launch Checklist
///).