Skip to content

Commit

Permalink
[web] Ensure handled key event is not propagated to IME (flutter#46829)
Browse files Browse the repository at this point in the history
Fixes [136460](flutter/flutter#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].

<!-- Links -->
[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
  • Loading branch information
knopp authored Oct 18, 2023
1 parent 8d51b64 commit d3c97bb
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 20 deletions.
9 changes: 7 additions & 2 deletions lib/web_ui/lib/src/engine/keyboard_binding.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
});
}

Expand Down Expand Up @@ -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;
}

Expand Down
23 changes: 5 additions & 18 deletions lib/web_ui/lib/src/engine/raw_keyboard.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand All @@ -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.
Expand All @@ -46,24 +40,16 @@ class RawKeyboard {
/// if no repeat events were received.
final Map<String, Timer> _keydownTimers = <String, Timer>{};

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;
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
}
},
);
Expand Down
7 changes: 7 additions & 0 deletions lib/web_ui/test/common/keyboard_test_common.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class MockKeyboardEvent implements FlutterHtmlKeyboardEvent {
bool altGrKey = false,
this.location = 0,
this.onPreventDefault,
this.onStopPropagation,
}) : modifierState =
<String>{
if (altKey) 'Alt',
Expand Down Expand Up @@ -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;
}
4 changes: 4 additions & 0 deletions lib/web_ui/test/engine/raw_keyboard_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
48 changes: 48 additions & 0 deletions lib/web_ui/test/engine/text_editing_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -370,6 +372,52 @@ Future<void> 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(<String, dynamic>{'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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit d3c97bb

Please sign in to comment.