[Web] Synthesize key events for modifier keys on pointer events. #36724

bleroux · 2022-10-12T15:00:06Z

Description

This PR implements a way to synchronize modifier keys state based on pointer data.
This is a web only implementation to discuss the implementation choice. It might leads to non-WIP PRs for each platforms.

What problem does it solve?

Engine and framework communicate to maintain information about the current keyboard state.
When a Flutter window lost the main focus, it has no way to be notified of modifier keys state changes which leads to possible bugs such as flutter/flutter#112488.

Implementation

The implementation is based on @dkwingsmt insights from flutter/flutter#112488 (comment) :
"

When the engine receives a pointer event, extract its keyboard state, and send it to the keyboard manager.
The keyboard manager synchronizes with the states, dispatch synthesized events as needed.
If this procedure is done synchronously, the synthesized key events will arrive at the framework before the pointer events, thus correctly affecting the next pointer event.

Notes:

This works the best for the hardware keyboard system.
If we really want to support the (to be deprecated) raw keyboard system, the raw keyboard manager can add the key state to the raw key data JSON payload, and process synchronization on the framework side.

"

Related Issue

Fixes flutter/flutter#112488
flutter/flutter#115066

Tests

Adds 4 tests to validate that key up and key down events are synthesized if needed for the main modifiers (Alt, Ctrl, Meta, Shift).

bleroux · 2022-10-13T19:40:56Z

Hey @dkwingsmt !
Before going further (implementing support for all modifier keys and add tests), I'm very interested by your feedback on this work in progress.
For the moment:

only shift key is handled (I will refactor this to handled other modifier keys).
on each pointer event we check if the known shift key state has changed.
if the state has changed, a key up or down event is synthesized.
I manually tested this on TextField selection start does not move flutter#112488 and it seems to fix it.

dkwingsmt

Good job! Everything so far looks great. Thank you for working on it.

Another task is to support all 3 event adapters: PointerEvent, MouseEvent, and TouchEvent. For now you only supported the first.

The MouseEvent and PointerEvent are basically the same since the latter inherits the former. However, TouchEvent uses the traditional xxxKey methods. Combining with my suggestion of making a universal interface for KeyboardBinding, maybe consider creating an interface class called KeyboardSynchronizingDelegate.

lib/web_ui/lib/src/engine/keyboard_binding.dart

dkwingsmt · 2022-10-13T21:36:41Z

lib/web_ui/lib/src/engine/keyboard_binding.dart

+      timeStamp: Duration.zero,
+      type: type,
+      physical: kWebToPhysicalKey['ShiftLeft']!,
+      logical: _kLogicalShiftLeft,


This magic value is concerning, but for now we don't have other ways than keeping a static map from physical keys to logical keys.

I added _kPhysicalxxx variables (similar to the _kLogicalxxx ones) to centralize those magic values.
Maybe adding a generated enum in key_map.g.dart could help to not dissiminate those magic values and might make it possible to rely on const variables?

dkwingsmt · 2022-10-13T21:42:10Z

lib/web_ui/lib/src/engine/keyboard_binding.dart

@@ -152,6 +152,35 @@ class KeyboardBinding {
    _converter = KeyboardConverter(_onKeyData, onMacOs: operatingSystem == OperatingSystem.macOs);
  }

+  // Synthesize shift key up or down event only when the known pressing state is different.
+  void synthesizeShiftKeyIfNeeded(ui.KeyEventType type) {
+    // TODO(bleroux): should we take care of shift left AND shift right?


Yes.

Let's use a "least event dispatched" strategy:

If the true state is pressed, and neither key is pressed, press the left key.

If the true state is not pressed, and any key is pressed, release these keys.

Otherwise, dispatch no events.

dkwingsmt · 2022-10-13T21:46:25Z

lib/web_ui/lib/src/engine/keyboard_binding.dart

@@ -152,6 +152,35 @@ class KeyboardBinding {
    _converter = KeyboardConverter(_onKeyData, onMacOs: operatingSystem == OperatingSystem.macOs);
  }

+  // Synthesize shift key up or down event only when the known pressing state is different.
+  void synthesizeShiftKeyIfNeeded(ui.KeyEventType type) {


Can we make KeyboardBinding's new public method handle all four modifiers? (Also see my review comment)

Can we make KeyboardBinding's new public method handle all four modifiers? (Also see my #36724 (review))

I replaced it with synthesizeModifiersIfNeeded with 4 bool parameters. I wanted to use something more generic but those bools make it clear which modifiers are supported for the moment.
While writing this I realize that this public method could be considered as a new API and updating it later might be a breaking change, is it? If yes, is what you suggested it to define an interface that will make it possible for this method to query the modifier state instead of receiving them?

bleroux · 2022-10-18T14:00:59Z

Good job! Everything so far looks great. Thank you for working on it.

Thanks!

Another task is to support all 3 event adapters: PointerEvent, MouseEvent, and TouchEvent. For now you only supported the first.

While implementing this, I noticed that TouchEvent modifiers accesors are not defined. I filed a PR to add them, thank you to review it: #36836.

bleroux · 2022-10-19T15:18:05Z

@dkwingsmt
I have updated the PR mainly to add tests.

To make testing easier and make the dependence between PointerBinding and KeyboardBinding more explicit, I choose to inject the KeyboardConverter in PointerBinding.initInstance.

dkwingsmt

LGTM, thanks for working on this!

flutter/engine#36724)

…113847) * e27e1964f Add touch-input-test to test_suites.yaml (flutter/engine#36900) * b923707a3 [Impeller] remove solid stroke contents and allow strokes/vertices to use color sources (flutter/engine#36896) * 85e4fa84d Roll Dart SDK from e06db8e1b620 to 1d418b40d8bd (1 revision) (flutter/engine#36902) * 0b57b1cab Roll buildroot to a680bb1. (flutter/engine#36901) * f1634aa30 Roll Fuchsia Mac SDK from IdQEnRNQNY7ZrLZ04... to jB4jUAxe89I2A-yqv... (flutter/engine#36904) * f24ea1a04 Roll Fuchsia Linux SDK from g6-kU8so3PDiR1106... to mdl-0MUwR6uuQdKIm... (flutter/engine#36905) * 04fa86e1b Added integration test for platform channels on windows. (flutter/engine#36853) * 1dbf3ff76 Convert the executable directory path to UTF-8 on Windows (flutter/engine#36908) * c3d4fc953 Roll Dart SDK from 1d418b40d8bd to f1d4c7c808bd (2 revisions) (flutter/engine#36913) * 51b66c968 [Web] Synthesize key events for shift key on pointer events. (flutter/engine#36724) * 584fffb67 Roll Fuchsia Mac SDK from jB4jUAxe89I2A-yqv... to fcFu9Z2KJH6oQvHnG... (flutter/engine#36919) * 4369421b8 Roll Fuchsia Linux SDK from mdl-0MUwR6uuQdKIm... to NqPnoRHl3WYqH3SrC... (flutter/engine#36920) * d6d38abb2 [Impeller] fix null geometry (flutter/engine#36922) * d7f987ee3 [Impeller] Eliminate unused shader output (flutter/engine#36923) * c255470e7 Roll Dart SDK from f1d4c7c808bd to eafe0119c9f5 (2 revisions) (flutter/engine#36925) * 224a3def0 Restore support for building the web SDK without a prebuilt Dart SDK (flutter/engine#36926) * c7c21e56f Re-landing Robolectric 4.8.1 (flutter/engine#34272) * ab9802379 Roll libtess2 to 725e5e08ec8751477565f1d603fd7eb9058c277c (flutter/engine#36928) * 95e937a44 Revert "Roll libtess2 to 725e5e08ec8751477565f1d603fd7eb9058c277c (#36928)" (flutter/engine#36932) * 83092c04c Revert Dart SDK to 2.19.0-324.0.dev (flutter/engine#36930)

flutter/engine#36724)

flutter-dashboard bot added the platform-web Code specifically for the web engine label Oct 12, 2022

bleroux requested review from dkwingsmt and mdebbar October 12, 2022 15:00

dkwingsmt reviewed Oct 13, 2022

View reviewed changes

bleroux mentioned this pull request Oct 18, 2022

Add DOM TouchEvent modifiers accessors #36836

Merged

bleroux force-pushed the synthesize_modifier_keys_events_on_pointer_events branch from 18cd182 to 9ab52b6 Compare October 18, 2022 13:46

bleroux added 3 commits October 19, 2022 08:24

Synthesize key events for shift key on pointer events.

c727189

Get the timestamp from the DOM event

a5c151a

Check all modifiers for PointerEvent, MouseEvent and TouchEvent

7bfc20b

bleroux force-pushed the synthesize_modifier_keys_events_on_pointer_events branch from 9ab52b6 to df6220d Compare October 19, 2022 14:11

Inject KeyboardConverter + add tests

14820cc

bleroux force-pushed the synthesize_modifier_keys_events_on_pointer_events branch from df6220d to 14820cc Compare October 19, 2022 14:27

bleroux changed the title ~~[WIP] Synthesize key events for shift key on pointer events.~~ [Web] Synthesize key events for shift key on pointer events. Oct 19, 2022

bleroux marked this pull request as ready for review October 19, 2022 14:28

bleroux requested a review from dkwingsmt October 19, 2022 15:18

dkwingsmt approved these changes Oct 21, 2022

View reviewed changes

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 21, 2022

auto-submit bot merged commit 51b66c9 into flutter:main Oct 21, 2022

bleroux deleted the synthesize_modifier_keys_events_on_pointer_events branch October 21, 2022 06:06

engine-flutter-autoroll mentioned this pull request Oct 21, 2022

Roll Flutter Engine from f24ea1a04d93 to 4369421b86f9 (6 revisions) flutter/flutter#113837

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2022

51b66c968 [Web] Synthesize key events for shift key on pointer events. (

f5d481f

flutter/engine#36724)

engine-flutter-autoroll mentioned this pull request Oct 21, 2022

Roll Flutter Engine from 2b21a3125ca2 to d6d38abb2d4c (13 revisions) flutter/flutter#113839

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2022

51b66c968 [Web] Synthesize key events for shift key on pointer events. (

94ed2d3

flutter/engine#36724)

engine-flutter-autoroll mentioned this pull request Oct 21, 2022

Roll Flutter Engine from 2b21a3125ca2 to d7f987ee3d74 (14 revisions) flutter/flutter#113841

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2022

51b66c968 [Web] Synthesize key events for shift key on pointer events. (

b68f86d

flutter/engine#36724)

engine-flutter-autoroll mentioned this pull request Oct 21, 2022

Roll Flutter Engine from 2b21a3125ca2 to c255470e7029 (15 revisions) flutter/flutter#113842

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2022

51b66c968 [Web] Synthesize key events for shift key on pointer events. (

9959e67

flutter/engine#36724)

engine-flutter-autoroll mentioned this pull request Oct 21, 2022

Roll Flutter Engine from 2b21a3125ca2 to 224a3def01a4 (16 revisions) flutter/flutter#113843

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2022

51b66c968 [Web] Synthesize key events for shift key on pointer events. (

ed1a479

flutter/engine#36724)

engine-flutter-autoroll mentioned this pull request Oct 21, 2022

Roll Flutter Engine from 2b21a3125ca2 to c7c21e56f27b (17 revisions) flutter/flutter#113844

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2022

51b66c968 [Web] Synthesize key events for shift key on pointer events. (

83b7370

flutter/engine#36724)

engine-flutter-autoroll mentioned this pull request Oct 21, 2022

Roll Flutter Engine from 2b21a3125ca2 to 83092c04c105 (20 revisions) flutter/flutter#113847

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2022

51b66c968 [Web] Synthesize key events for shift key on pointer events. (

ae96743

flutter/engine#36724)

engine-flutter-autoroll mentioned this pull request Oct 21, 2022

Roll Flutter Engine from 2b21a3125ca2 to de83ef6d2c26 (21 revisions) flutter/flutter#113849

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2022

51b66c968 [Web] Synthesize key events for shift key on pointer events. (

1f2580b

flutter/engine#36724)

bleroux self-assigned this Oct 25, 2022

dkwingsmt changed the title ~~[Web] Synthesize key events for shift key on pointer events.~~ [Web] Synthesize key events for modifier keys on pointer events. Oct 25, 2022

This was referenced Nov 3, 2022

[Web] Synthesize modifiers key up based on known logical key #37280

Merged

Modifier keys state should also be synchronized based on pointer events flutter/flutter#115066

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Web] Synthesize key events for modifier keys on pointer events. #36724

[Web] Synthesize key events for modifier keys on pointer events. #36724

bleroux commented Oct 12, 2022 •

edited

Loading

bleroux commented Oct 13, 2022

dkwingsmt left a comment •

edited

Loading

dkwingsmt Oct 13, 2022

bleroux Oct 18, 2022

dkwingsmt Oct 13, 2022

bleroux Oct 18, 2022

dkwingsmt Oct 13, 2022 •

edited

Loading

bleroux Oct 18, 2022

bleroux commented Oct 18, 2022 •

edited

Loading

bleroux commented Oct 19, 2022

dkwingsmt left a comment

[Web] Synthesize key events for modifier keys on pointer events. #36724

[Web] Synthesize key events for modifier keys on pointer events. #36724

Conversation

bleroux commented Oct 12, 2022 • edited Loading

Description

What problem does it solve?

Implementation

Related Issue

Tests

bleroux commented Oct 13, 2022

dkwingsmt left a comment • edited Loading

Choose a reason for hiding this comment

dkwingsmt Oct 13, 2022

Choose a reason for hiding this comment

bleroux Oct 18, 2022

Choose a reason for hiding this comment

dkwingsmt Oct 13, 2022

Choose a reason for hiding this comment

bleroux Oct 18, 2022

Choose a reason for hiding this comment

dkwingsmt Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

bleroux Oct 18, 2022

Choose a reason for hiding this comment

bleroux commented Oct 18, 2022 • edited Loading

bleroux commented Oct 19, 2022

dkwingsmt left a comment

Choose a reason for hiding this comment

bleroux commented Oct 12, 2022 •

edited

Loading

dkwingsmt left a comment •

edited

Loading

dkwingsmt Oct 13, 2022 •

edited

Loading

bleroux commented Oct 18, 2022 •

edited

Loading