[web] Ensure handled key event is not propagated to IME #46829

knopp · 2023-10-12T15:03:11Z

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

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
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.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-10-12T15:03:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

dkwingsmt · 2023-10-12T23:32:20Z

The change looks ok, but it doesn't seem to me that the unit test this PR modifies is related to the change. Is this change tested?

knopp · 2023-10-13T12:21:25Z

The change looks ok, but it doesn't seem to me that the unit test this PR modifies is related to the change. Is this change tested?

The test was missing indeed. Added now, can you take another look?

Renzo-Olivares · 2023-10-13T19:04:02Z

third_party/web_locale_keymap/lib/web_locale_keymap/locale_keymap.dart

      return eventKeyCode;
    }
    if (result == null) {
+      if ((eventCode ?? '').isEmpty && (eventKey ?? '').isEmpty) {


nit: could this be eventCode == null && eventKey == null ?

No. We're passing empty strings there during some tests unfortunately.

Renzo-Olivares

LGTM with a small nit. Thanks for the change!

knopp · 2023-10-18T08:41:59Z

@dkwingsmt, friendly ping

dkwingsmt · 2023-10-18T20:33:51Z

Is anything related to stopPropagation tested? Actually I thought this was the major change of this PR.

knopp · 2023-10-18T21:20:45Z

Is anything related to stopPropagation tested? Actually I thought this was the major change of this PR.

It is. The handling keyboard event prevents triggering input action test will fail without the stopPropagation() call.

…utter/engine#46829)

…)" This reverts commit d3c97bb.

…utter/engine#46829)

) Reverts #46829 Fixes flutter/flutter#136857 Speculative fix for The builds breaking on the web text tests as seen here: https://github.com/flutter/flutter/runs/17840697842

…utter/engine#46829)

…136872) flutter/engine@8d51b64...d0d7b4d 2023-10-19 [email protected] Revert "[web] Ensure handled key event is not propagated to IME" (flutter/engine#47086) 2023-10-19 [email protected] Roll Fuchsia Linux SDK from 2HSBpWikGWvPJlHOP... to Z2mOZ5cuPJWvKn22P... (flutter/engine#47094) 2023-10-19 [email protected] Roll Skia from 64c5de6663a4 to 62acc1e162cc (1 revision) (flutter/engine#47093) 2023-10-19 [email protected] Roll Dart SDK from eb3fb9a13e16 to 1b425a25a6dd (1 revision) (flutter/engine#47092) 2023-10-19 [email protected] Roll Skia from 85971b25cf4b to 64c5de6663a4 (2 revisions) (flutter/engine#47091) 2023-10-19 [email protected] Roll Fuchsia Mac SDK from PxJM5ivbnbYoVkqBg... to 0hLSNJ_DCTzRvTVJe... (flutter/engine#47090) 2023-10-19 [email protected] Roll Dart SDK from d9b1e60fe442 to eb3fb9a13e16 (1 revision) (flutter/engine#47084) 2023-10-18 [email protected] Update buildroot to ab76ccba2ca4ce4. (flutter/engine#47078) 2023-10-18 [email protected] migrate const_finder to use the implementation from package:kernel (flutter/engine#47077) 2023-10-18 [email protected] Roll Skia from d561b276b5d9 to 85971b25cf4b (1 revision) (flutter/engine#47076) 2023-10-18 [email protected] [Impeller] Guard execution of ReactorGLES operations with a mutex (flutter/engine#47068) 2023-10-18 [email protected] [Impeller] Document ReactorGLES. (flutter/engine#47070) 2023-10-18 [email protected] [web] Ensure handled key event is not propagated to IME (flutter/engine#46829) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 2HSBpWikGWvP to Z2mOZ5cuPJWv fuchsia/sdk/core/mac-amd64 from PxJM5ivbnbYo to 0hLSNJ_DCTzR If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

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].  [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

) Reverts #46829 Fixes flutter/flutter#136857 Speculative fix for The builds breaking on the web text tests as seen here: https://github.com/flutter/flutter/runs/17840697842

#136874) flutter/engine#46829 changed event handling sequence on web so that KeyboardBinding/RawKeyboard handles the text event first before it reaches IME. That means when dispatching synthetised events to IME every keydown event must have a corresponding keyup, otherwise consistency assertions in `KeyboardBindings` are triggered. ## 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. - [x] 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] 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 [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#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/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

This PR relands #46829, which got reverted in #47086 because of flutter/flutter#136857. There are no changes in the PR compared to the reverted one. The issue are missing `keyup` events in the integration test, which triggers assertion in `KeyboardBindings` and which should be fixed by flutter/flutter#136874. 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. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] 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. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] 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

github-actions bot added the platform-web Code specifically for the web engine label Oct 12, 2023

knopp force-pushed the 136460_web_no_propagate_handled_event_to_ime branch from 9f87c1b to b24fedd Compare October 12, 2023 16:11

knopp changed the title ~~WIP: [web] Ensure handled key event is not propagated to IME~~ [web] Ensure handled key event is not propagated to IME Oct 12, 2023

knopp requested review from Renzo-Olivares and dkwingsmt October 12, 2023 19:55

knopp force-pushed the 136460_web_no_propagate_handled_event_to_ime branch from 6811026 to 60bbdc3 Compare October 13, 2023 10:36

Renzo-Olivares reviewed Oct 13, 2023

View reviewed changes

Renzo-Olivares approved these changes Oct 13, 2023

View reviewed changes

dkwingsmt approved these changes Oct 18, 2023

View reviewed changes

knopp added 10 commits October 19, 2023 00:21

[web] Ensure handled key event is not propagated to IME

1a2d263

Add missing implementation

96ba7a3

Use relative import

767e2c6

Init keyboard binding instance

203cfc2

Check channel name in test

2afa26c

Fix more tests

364c9c7

typo

18b7702

Remove channel name check

e79ca64

Fix exception on empty eventCode and eventKey

021da3b

Add test

129cce8

knopp force-pushed the 136460_web_no_propagate_handled_event_to_ime branch from 60bbdc3 to 129cce8 Compare October 18, 2023 21:21

knopp merged commit d3c97bb into flutter:main Oct 18, 2023

knopp deleted the 136460_web_no_propagate_handled_event_to_ime branch October 18, 2023 21:52

engine-flutter-autoroll mentioned this pull request Oct 18, 2023

Roll Flutter Engine from 8d51b64e9440 to 40aee62d7d39 (4 revisions) flutter/flutter#136850

Closed

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

d3c97bbe6 [web] Ensure handled key event is not propagated to IME (fl…

940dcb9

…utter/engine#46829)

engine-flutter-autoroll mentioned this pull request Oct 18, 2023

Roll Flutter Engine from 8d51b64e9440 to 56d09075b2ee (6 revisions) flutter/flutter#136852

Closed

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

d3c97bbe6 [web] Ensure handled key event is not propagated to IME (fl…

add2a69

…utter/engine#46829)

flar mentioned this pull request Oct 19, 2023

Engine roll failing on web text test flutter/flutter#136857

Closed

flar added a commit that referenced this pull request Oct 19, 2023

Revert "[web] Ensure handled key event is not propagated to IME (#46829…

b0b3f41

…)" This reverts commit d3c97bb.

flar mentioned this pull request Oct 19, 2023

Revert "[web] Ensure handled key event is not propagated to IME" #47086

Merged

engine-flutter-autoroll mentioned this pull request Oct 19, 2023

Roll Flutter Engine from 8d51b64e9440 to 0f9b275fb7e9 (7 revisions) flutter/flutter#136859

Closed

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

d3c97bbe6 [web] Ensure handled key event is not propagated to IME (fl…

1f53a65

…utter/engine#46829)

engine-flutter-autoroll mentioned this pull request Oct 19, 2023

Roll Flutter Engine from 8d51b64e9440 to 0f9b275fb7e9 (7 revisions) flutter/flutter#136863

Closed

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

d3c97bbe6 [web] Ensure handled key event is not propagated to IME (fl…

ad4bf40

…utter/engine#46829)

engine-flutter-autoroll mentioned this pull request Oct 19, 2023

Roll Flutter Engine from 8d51b64e9440 to e4daa2620a49 (8 revisions) flutter/flutter#136866

Closed

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

d3c97bbe6 [web] Ensure handled key event is not propagated to IME (fl…

c8e40a6

…utter/engine#46829)

engine-flutter-autoroll mentioned this pull request Oct 19, 2023

Roll Flutter Engine from 8d51b64e9440 to 16a25b1acaf6 (10 revisions) flutter/flutter#136869

Closed

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

d3c97bbe6 [web] Ensure handled key event is not propagated to IME (fl…

458eb5f

…utter/engine#46829)

engine-flutter-autoroll mentioned this pull request Oct 19, 2023

Roll Flutter Engine from 8d51b64e9440 to d0d7b4d61265 (13 revisions) flutter/flutter#136872

Merged

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

d3c97bbe6 [web] Ensure handled key event is not propagated to IME (fl…

29a03d2

…utter/engine#46829)

This was referenced Oct 19, 2023

[web] dispatch corresponding keyup events in text editing integrations flutter/flutter#136874

Merged

Reland: [web] Ensure handled key event is not propagated to IME #47099

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[web] Ensure handled key event is not propagated to IME #46829

[web] Ensure handled key event is not propagated to IME #46829

Uh oh!

knopp commented Oct 12, 2023

Uh oh!

flutter-dashboard bot commented Oct 12, 2023

Uh oh!

dkwingsmt commented Oct 12, 2023

Uh oh!

knopp commented Oct 13, 2023

Uh oh!

Renzo-Olivares Oct 13, 2023

Uh oh!

knopp Oct 13, 2023

Uh oh!

Renzo-Olivares left a comment

Uh oh!

knopp commented Oct 18, 2023

Uh oh!

dkwingsmt commented Oct 18, 2023

Uh oh!

knopp commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[web] Ensure handled key event is not propagated to IME #46829

[web] Ensure handled key event is not propagated to IME #46829

Uh oh!

Conversation

knopp commented Oct 12, 2023

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Oct 12, 2023

Uh oh!

dkwingsmt commented Oct 12, 2023

Uh oh!

knopp commented Oct 13, 2023

Uh oh!

Renzo-Olivares Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

knopp Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares left a comment

Choose a reason for hiding this comment

Uh oh!

knopp commented Oct 18, 2023

Uh oh!

dkwingsmt commented Oct 18, 2023

Uh oh!

knopp commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants