Ignore key events on edit control on web platform (#52656) by cjng96 · Pull Request #52661 · flutter/flutter

cjng96 · 2020-03-16T11:33:31Z

Description

Ignore key event from web platform to prevent twice key action by one key press.
For now It's done on Flutter Engine's keyboard.dart code. but I move that code here since we can use a key event via RawKeyListener on web platform(It's working well on Android)

After this pull-request is merged, I will issue new pull-request on Flutter Engine that allowing to send keyEvent which occurs on input element with web.

Related Issues

#52656

#44133

Tests

I added the following tests:

editable_test.dart / ignore key event from web platform

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.

googlebot · 2020-03-16T11:33:36Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-03-16T11:47:47Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

chunhtai · 2020-03-16T16:56:49Z

Instead of exposing this private method, I would initialize the flutter binding and uses

ServicesBinding.instance.defaultBinaryMessenger.handlePlatformMessage( SystemChannels.keyEvent.name, SystemChannels.keyEvent.codec.encodeMessage(data), (ByteData data) {}, );

Good!. Thank you. :)

As you suggested, I tried to use the ServicesBinding class.

It seems to be working if I use testWidgets and EditableText for pumpWidget. ( You can see it on updated pull-request's source code)
I failed to find a other solution to use both RenderEditable and ServicesBinding together without pumpWidget.

This test code is working well. but I am not sure if this is correct implementation under flutter testing rule. :(

Sorry I forgot to mention you will need TestWidgetsFlutterBinding.ensureInitialized();
See
https://github.com/flutter/flutter/blob/master/packages/flutter/test/services/platform_channel_test.dart

chunhtai · 2020-03-16T17:01:30Z

This comment is unclear, is the key down only apply to arrow key?

This code and one more pull-request(I does't issue yet) on engine repo replace flutter engine's pull-request #13699. so ignore all keyEvent from input control on web.

Arrow key(_handleMovement function) and delete key(_handleDelete function) should be ignored.

But I didn't check the keys(ctrl+c, ctrl+v, ctrl+x and ctrl+a) which be handled in _handleShortcuts yet. I am going to check those keys should be ignored or not. then renew pull-request. Thank you.

Ctrl+c, ctrl+v keys are working well. so I modify the code to ignore movement keys and delete key only.
Thank you.

chunhtai

added some comments, also cc @mdebbar

chunhtai · 2020-03-18T18:16:42Z

chunhtai · 2020-03-18T18:18:12Z

remove this?

chunhtai · 2020-03-18T18:20:38Z

I am surprised that uses TestWidgetsFlutterBinding.ensureInitialized(); still requires testWidget. what was the issue you were facing? If this is necessary, this test should be put in editable_text_test.dart

Without testWidgets and pumpWidget, I could't use SystemBinding. :(
I will paste error message and sample code I tried because this can be user error(I'm newbie on Flutter).

I don't like this code which uses EditableText here. It's good if I can implement without testWidgets and pumpWidget function. and in other case, I will move this test code to editable_text_test.dart as you said.

Thank you.

I am surprised that uses TestWidgetsFlutterBinding.ensureInitialized(); still requires testWidget. what was the issue you were facing? If this is necessary, this test should be put in editable_text_test.dart

Sorry today I succeed to make test code without testWidgets and pumpWidget functions as you said. maybe I was a little bit confused about test framework. :(
I updated my pull-request. Thank you for your comments. :)

chunhtai · 2020-03-18T18:29:05Z

just return code here and return null at line 115?

This code is copied from _getKeyCode() function, this style is there. so I follow same style.

Anyway I think the style you suggest is better too. :)

chunhtai · 2020-03-18T18:30:42Z

lets follow the same style here final int scanCode = platform == 'web' ? -1 : _getScanCode(physicalKey, platform);

Good. OK. :)

mdebbar · 2020-03-18T19:44:16Z

The reason we decided to let the browser handle all key events for us is because it's difficult to hardcode the handling of all keys.

For example, movement keys and delete are not the only ones we should ignore here. What about backspace, home, end, page-up, page-down, enter, etc?

Could you explain your use-case a bit more? Like, why do you need to listen to keys when the user is editing a text field?

Hello thank you for your comment, :)

First, regarding the keys we should ignore on control,
We can ignore all keyEvent here instead of Engine level. If so, it has same result but we can use RawKeyListener with EditableText. (It's not available now due to engine level keyEvent preventation.)

About my use-case,
I need to implement short-cut key so I need RawKeyLisener for it.

For example,
When the user press ctrl+r key(any key promised), I want
(1) Popup dialog for recommanded text list,
(2) Reset to default text status,
(3) Change input dialog layout(normal user or expert user)

Future more, I want to implement powerful rich editor based on flutter platform. as far as I know, text_editing.dart service should be used for it. but I can't receive keyEvent from it. so it's impossible on web. :(

Sorry for the delay. Yeah, those use-cases make sense.

chunhtai · 2020-03-19T17:41:02Z

this can just be

await simulateKeyDownEvent(LogicalKeyboardKey.arrowRight, platform: 'web');

Oh... I didn't know there is that global function. Thank you for shorten code.

chunhtai · 2020-03-19T17:41:10Z

chunhtai

I don't have anything else, it lgtm if it also lgt @mdebbar

mdebbar

Could you also share the engine PR so we can review both at the same time?

mdebbar · 2020-03-20T17:21:11Z

There's no need to check the type of the event. You should use the available constant kIsWeb.

And I would put this check in the beginning of the method to avoid doing unnecessary work like collapsing synonyms, etc.

Thank you. I modify the source to use kIsWeb. so I also modify test function. I activate my test function on web platform but omit other test functions in editable_test.dart on web. :(
I'm not sure if it's rightapproach. please confirm new pull-request.

mdebbar · 2020-03-20T17:39:26Z

Overall, the PR looks good to me at this point (except the isWeb check that I commented about). But I'll wait for the engine PR so we can review both and make sure they will work well together.

goderbauer · 2020-03-25T21:41:06Z

(PR triage): This PR is blocked on flutter/engine#17242.

mdebbar

cjng96 · 2020-03-31T02:09:48Z

I wonder when this pull-request is merged.

mdebbar · 2020-03-31T17:47:54Z

@cjng96 I can merge the PRs. I think the framework PR should be merged first, correct?

cjng96 · 2020-04-01T02:08:39Z

@cjng96 I can merge the PRs. I think the framework PR should be merged first, correct?

Yes, framework PR should be merged first. Thank you.

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 16, 2020

googlebot added the cla: no label Mar 16, 2020

cjng96 force-pushed the ignore-key branch from 1ba3467 to 808c5c2 Compare March 16, 2020 11:47

googlebot added cla: yes and removed cla: no labels Mar 16, 2020

chunhtai reviewed Mar 16, 2020

View reviewed changes

cjng96 force-pushed the ignore-key branch from 808c5c2 to fece464 Compare March 18, 2020 16:19

cjng96 requested a review from chunhtai March 18, 2020 16:29

cjng96 force-pushed the ignore-key branch from fece464 to c3d0a62 Compare March 18, 2020 16:31

chunhtai reviewed Mar 18, 2020

View reviewed changes

Piinks added a: text input Entering text in a text field or keyboard related problems platform-web Web applications specifically labels Mar 18, 2020

mdebbar reviewed Mar 18, 2020

View reviewed changes

cjng96 force-pushed the ignore-key branch 3 times, most recently from 5dea0b3 to df97ef9 Compare March 19, 2020 14:33

chunhtai reviewed Mar 19, 2020

View reviewed changes

cjng96 force-pushed the ignore-key branch 2 times, most recently from 9b93e7b to 3e51403 Compare March 20, 2020 14:11

chunhtai reviewed Mar 20, 2020

View reviewed changes

mdebbar reviewed Mar 20, 2020

View reviewed changes

cjng96 force-pushed the ignore-key branch 2 times, most recently from 7b9acf5 to 3aed75d Compare March 21, 2020 01:38

Ignore key events on edit control on web platform (flutter#52656)

3aed75d

cjng96 mentioned this pull request Mar 21, 2020

Remove code which skips keyEvent from input element on web flutter/engine#17242

Merged

kbokarius mentioned this pull request Mar 23, 2020

[webview_flutter] Cannot clear cache on Android using webViewController.clearCache() #53122

Closed

cjng96 mentioned this pull request Mar 25, 2020

On web platforms, I want to receive a key event via RawKeyboardListener #52656

Closed

mdebbar approved these changes Mar 25, 2020

View reviewed changes

mdebbar added the waiting for tree to go green label Apr 1, 2020

fluttergithubbot merged commit d62c7ec into flutter:master Apr 1, 2020

cjng96 deleted the ignore-key branch April 1, 2020 11:04

github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Conversation

cjng96 commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

googlebot commented Mar 16, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Mar 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjng96 Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjng96 Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjng96 Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cjng96 commented Mar 16, 2020 •

edited

Loading

cjng96 Mar 18, 2020 •

edited

Loading

cjng96 Mar 17, 2020 •

edited

Loading

cjng96 Mar 19, 2020 •

edited

Loading

goderbauer commented Mar 25, 2020 •

edited

Loading