From 688a695f2bc1be8a01a7458deb3a81dfd896d027 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Thu, 19 Oct 2023 00:52:53 +0300 Subject: [PATCH] [web] Ensure handled key event is not propagated to IME (flutter/engine#46829) Fixes [136460](https://github.com/flutter/flutter/issues/136460) Changes: - Raw keyboard event is handled during capture phase. This is to ensure that the framework processes the event before reaching to IME text area and raw keyboard can stop the propagation for handled events. - `RawKeyboard` event handler is invoked from `KeyboardBinding` event handler. This is to prevent race condition because both handlers now run in capture phase and `KeyboardBinding` needs to process the event first. *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 - [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] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- .../lib/src/engine/keyboard_binding.dart | 9 +++- .../web_ui/lib/src/engine/raw_keyboard.dart | 23 ++------- .../test/common/keyboard_test_common.dart | 7 +++ .../web_ui/test/engine/raw_keyboard_test.dart | 4 ++ .../web_ui/test/engine/text_editing_test.dart | 48 +++++++++++++++++++ .../lib/web_locale_keymap/locale_keymap.dart | 3 ++ 6 files changed, 74 insertions(+), 20 deletions(-) diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/keyboard_binding.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/keyboard_binding.dart index 7a851b16c2d..c12d1a36a9f 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/keyboard_binding.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/keyboard_binding.dart @@ -13,6 +13,7 @@ import 'browser_detection.dart'; import 'dom.dart'; import 'key_map.g.dart'; import 'platform_dispatcher.dart'; +import 'raw_keyboard.dart'; import 'semantics.dart'; typedef _VoidCallback = void Function(); @@ -104,9 +105,12 @@ class KeyboardBinding { _addEventListener('keydown', (DomEvent domEvent) { final FlutterHtmlKeyboardEvent event = FlutterHtmlKeyboardEvent(domEvent as DomKeyboardEvent); _converter.handleEvent(event); + RawKeyboard.instance?.handleHtmlEvent(domEvent); }); - _addEventListener('keyup', (DomEvent event) { - _converter.handleEvent(FlutterHtmlKeyboardEvent(event as DomKeyboardEvent)); + _addEventListener('keyup', (DomEvent domEvent) { + final FlutterHtmlKeyboardEvent event = FlutterHtmlKeyboardEvent(domEvent as DomKeyboardEvent); + _converter.handleEvent(event); + RawKeyboard.instance?.handleHtmlEvent(domEvent); }); } @@ -209,6 +213,7 @@ class FlutterHtmlKeyboardEvent { bool getModifierState(String key) => _event.getModifierState(key); void preventDefault() => _event.preventDefault(); + void stopPropagation() => _event.stopPropagation(); bool get defaultPrevented => _event.defaultPrevented; } diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/raw_keyboard.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/raw_keyboard.dart index b299437b21f..d4aa87b6bbc 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/raw_keyboard.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/raw_keyboard.dart @@ -15,15 +15,6 @@ import 'services.dart'; /// Provides keyboard bindings, such as the `flutter/keyevent` channel. class RawKeyboard { RawKeyboard._(this._onMacOs) { - _keydownListener = createDomEventListener((DomEvent event) { - _handleHtmlEvent(event); - }); - domWindow.addEventListener('keydown', _keydownListener); - - _keyupListener = createDomEventListener((DomEvent event) { - _handleHtmlEvent(event); - }); - domWindow.addEventListener('keyup', _keyupListener); registerHotRestartListener(() { dispose(); }); @@ -34,6 +25,9 @@ class RawKeyboard { /// Use the [instance] getter to get the singleton after calling this method. static void initialize({bool onMacOs = false}) { _instance ??= RawKeyboard._(onMacOs); + // KeyboardBinding is responsible for forwarding the keyboard + // events to the RawKeyboard handler. + KeyboardBinding.initInstance(); } /// The [RawKeyboard] singleton. @@ -46,24 +40,16 @@ class RawKeyboard { /// if no repeat events were received. final Map _keydownTimers = {}; - DomEventListener? _keydownListener; - DomEventListener? _keyupListener; - /// Uninitializes the [RawKeyboard] singleton. /// /// After calling this method this object becomes unusable and [instance] /// becomes `null`. Call [initialize] again to initialize a new singleton. void dispose() { - domWindow.removeEventListener('keydown', _keydownListener); - domWindow.removeEventListener('keyup', _keyupListener); - for (final String key in _keydownTimers.keys) { _keydownTimers[key]!.cancel(); } _keydownTimers.clear(); - _keydownListener = null; - _keyupListener = null; _instance = null; } @@ -96,7 +82,7 @@ class RawKeyboard { return event.type == 'keydown' && event.key == 'Tab' && event.isComposing; } - void _handleHtmlEvent(DomEvent domEvent) { + void handleHtmlEvent(DomEvent domEvent) { if (!domInstanceOfString(domEvent, 'KeyboardEvent')) { return; } @@ -158,6 +144,7 @@ class RawKeyboard { if (jsonResponse['handled'] as bool) { // If the framework handled it, then don't propagate it any further. event.preventDefault(); + event.stopPropagation(); } }, ); diff --git a/engine/src/flutter/lib/web_ui/test/common/keyboard_test_common.dart b/engine/src/flutter/lib/web_ui/test/common/keyboard_test_common.dart index 0b447d8b3b5..0ec1e5d75ac 100644 --- a/engine/src/flutter/lib/web_ui/test/common/keyboard_test_common.dart +++ b/engine/src/flutter/lib/web_ui/test/common/keyboard_test_common.dart @@ -22,6 +22,7 @@ class MockKeyboardEvent implements FlutterHtmlKeyboardEvent { bool altGrKey = false, this.location = 0, this.onPreventDefault, + this.onStopPropagation, }) : modifierState = { if (altKey) 'Alt', @@ -84,6 +85,12 @@ class MockKeyboardEvent implements FlutterHtmlKeyboardEvent { bool get defaultPrevented => _defaultPrevented; bool _defaultPrevented = false; + @override + void stopPropagation() { + onStopPropagation?.call(); + } + VoidCallback? onStopPropagation; + static bool get lastDefaultPrevented => _lastEvent?.defaultPrevented ?? false; static MockKeyboardEvent? _lastEvent; } diff --git a/engine/src/flutter/lib/web_ui/test/engine/raw_keyboard_test.dart b/engine/src/flutter/lib/web_ui/test/engine/raw_keyboard_test.dart index d9f4cd0fe40..0b1681b0404 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/raw_keyboard_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/raw_keyboard_test.dart @@ -52,6 +52,10 @@ void testMain() { DomKeyboardEvent event; + // Dispatch a keydown event first so that KeyboardBinding will recognize the keyup event. + // and will not set preventDefault on it. + event = dispatchKeyboardEvent('keydown', key: 'SomeKey', code: 'SomeCode', keyCode: 1); + event = dispatchKeyboardEvent('keyup', key: 'SomeKey', code: 'SomeCode', keyCode: 1); expect(event.defaultPrevented, isFalse); diff --git a/engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart b/engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart index a6e5f02c0a3..37d44b3f321 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart @@ -12,12 +12,14 @@ import 'package:test/test.dart'; import 'package:ui/src/engine.dart' show flutterViewEmbedder; import 'package:ui/src/engine/browser_detection.dart'; import 'package:ui/src/engine/dom.dart'; +import 'package:ui/src/engine/raw_keyboard.dart'; import 'package:ui/src/engine/services.dart'; import 'package:ui/src/engine/text_editing/autofill_hint.dart'; import 'package:ui/src/engine/text_editing/input_type.dart'; import 'package:ui/src/engine/text_editing/text_editing.dart'; import 'package:ui/src/engine/util.dart'; import 'package:ui/src/engine/vector_math.dart'; +import 'package:ui/ui.dart' as ui; import '../common/spy.dart'; import '../common/test_initialization.dart'; @@ -370,6 +372,52 @@ Future testMain() async { expect(lastInputAction, 'TextInputAction.done'); }); + test('handling keyboard event prevents triggering input action', () { + final ui.PlatformMessageCallback? savedCallback = ui.window.onPlatformMessage; + + bool markTextEventHandled = false; + ui.window.onPlatformMessage = (String channel, ByteData? data, + ui.PlatformMessageResponseCallback? callback) { + final ByteData response = const JSONMessageCodec() + .encodeMessage({'handled': markTextEventHandled})!; + callback!(response); + }; + RawKeyboard.initialize(); + + final InputConfiguration config = InputConfiguration(); + editingStrategy!.enable( + config, + onChange: trackEditingState, + onAction: trackInputAction, + ); + + // No input action so far. + expect(lastInputAction, isNull); + + markTextEventHandled = true; + dispatchKeyboardEvent( + editingStrategy!.domElement!, + 'keydown', + keyCode: _kReturnKeyCode, + ); + + // Input action prevented by platform message callback. + expect(lastInputAction, isNull); + + markTextEventHandled = false; + dispatchKeyboardEvent( + editingStrategy!.domElement!, + 'keydown', + keyCode: _kReturnKeyCode, + ); + + // Input action received. + expect(lastInputAction, 'TextInputAction.done'); + + ui.window.onPlatformMessage = savedCallback; + RawKeyboard.instance?.dispose(); + }); + test('Triggers input action in multi-line mode', () { final InputConfiguration config = InputConfiguration( inputType: EngineInputType.multiline, diff --git a/engine/src/flutter/third_party/web_locale_keymap/lib/web_locale_keymap/locale_keymap.dart b/engine/src/flutter/third_party/web_locale_keymap/lib/web_locale_keymap/locale_keymap.dart index dd2faa527fc..5c261f66557 100644 --- a/engine/src/flutter/third_party/web_locale_keymap/lib/web_locale_keymap/locale_keymap.dart +++ b/engine/src/flutter/third_party/web_locale_keymap/lib/web_locale_keymap/locale_keymap.dart @@ -41,6 +41,9 @@ class LocaleKeymap { return eventKeyCode; } if (result == null) { + if ((eventCode ?? '').isEmpty && (eventKey ?? '').isEmpty) { + return null; + } final int? heuristicResult = heuristicMapper(eventCode ?? '', eventKey ?? ''); if (heuristicResult != null) { return heuristicResult;