Adding handling of TextInputClient.onConnectionClosed messages handli… by nturgut · Pull Request #43466 · flutter/flutter

nturgut · 2019-10-25T00:34:16Z

Adding handling of TextInputClient.onConnectionClosed messages handling to Framework

Description

Adding a message to the Flutter Framework for clients to call if a connection is closed.

Information in following situations can be synced with the framework using this method call.

if the mobile browser is sent to background by pressing the home button on a mobile device.
If the blur event is triggered on an input element in a web browser.
the user closed the virtual keyboard indication they stopped editing.

This PR is merged and rolled back before: #35100

Related Issues

#43034

Tests

I added the following tests:

Unit tests for editable_text_test.dart
Unit tests for text_input.dart

Manually tested with the Flutter Web Engine.

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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

Announcement: https://groups.google.com/forum/#!topic/flutter-announce/IwG8xGA7Kmo

mdebbar

Thanks for tackling this!

Could you please double-check that this PR doesn't introduce the same regressions that caused the other PR to be reverted? cc @matthew-carroll

packages/flutter/test/widgets/editable_text_test.dart

matthew-carroll · 2019-10-25T06:59:59Z

I think we may have said we were waiting for new test infra to attempt another solution to this due to the nuanced ways a fix might introduce new bugs, but I'll defer to @HansMuller @gspencergoog and @collinjackson on that topic.

packages/flutter/lib/src/services/text_input.dart

packages/flutter/test/services/text_input_test.dart

packages/flutter/test/widgets/editable_text_test.dart

packages/flutter_test/lib/src/test_text_input.dart

goderbauer · 2019-10-25T16:38:03Z

@matthew-carroll From the comments on #39523 it sounded like the issue that caused the rollback was how we send this event from Android?

…ng to Framework

…e tests

nturgut · 2019-10-30T22:13:42Z

We chat with @matthew-carroll and @gspencergoog, and decided to continue with the framework change for now. Android change will follow when the integration tests for testing the behavior is supported.

matthew-carroll · 2019-10-30T22:21:19Z

@nturgut the update here based on @xster's comment is that if we prioritize this for the upcoming code freeze then we will probably have to implement the Android fix before we have new testing infra. I think web can continue as discussed, but we may be bringing in the Android side earlier than anticipated.

mdebbar

LGTM with some comments about the tests.

packages/flutter/lib/src/widgets/editable_text.dart

packages/flutter/test/widgets/editable_text_test.dart

packages/flutter_test/lib/src/test_text_input.dart

goderbauer

Approach looks good.

Added some comments about documentation. Since the text connection is a delicate dance between framework and engine to keep everything in sync we should document very clearly how things are intended to be used.

packages/flutter/lib/src/services/text_input.dart

packages/flutter/lib/src/widgets/editable_text.dart

packages/flutter/lib/src/services/text_input.dart

dnfield

This appears to re-introduce bugs for Android from the linked PR.

We should not land this without a test asserting that text input works as expected on Android with these changes.

nturgut · 2019-10-31T17:32:00Z

This appears to re-introduce bugs for Android from the linked PR.

We should not land this without a test asserting that text input works as expected on Android with these changes.

We already discussed it with @matthew-carroll @goderbauer @gspencergoog it is safe to land

dnfield

After looking a bit more at this with @nturgut, this can't cause problems for Android (or iOS) becuase those embeddings don't ever call this method at this point.

This won't impact Android.

…e tests. Added comments to the interfaces.

goderbauer

This looks fine (modulo the breaking change requirements).

However, there are two similar PRs out now. The other one is #43959. Just to double check: Do the addressed use cases really need two new APIs?

packages/flutter/lib/src/services/text_input.dart

goderbauer

LGTM

goderbauer · 2019-11-04T17:24:13Z

packages/flutter/lib/src/services/system_channels.dart

  ///
+  ///  * `TextInputClient.onConnectionClosed`: The text input connection closed
+  ///    on the platform side. For example the application is moved to
+  ///    background or used closed the virtual keyboard. This method informs


used? Oh, maybe "the user closed..."?

goderbauer · 2019-11-04T17:25:00Z

packages/flutter/lib/src/services/system_channels.dart

+  ///    on the platform side. For example the application is moved to
+  ///    background or used closed the virtual keyboard. This method informs
+  ///    [TextInputClient] to clear connection and finalize editing.
+  ///    `TextInput.clearClient` and `TextInput.hide` is not called after


is not called -> must not be called?

goderbauer · 2019-11-04T17:25:48Z

Please link the breaking change announcement from the PR description.

nturgut · 2019-11-04T17:31:14Z

Please link the breaking change announcement from the PR description.

Done! Thanks.

flutter#43466) * Adding handling of TextInputClient.onConnectionClosed messages handling to Framework * Adding more test cases for closing connection to editable_text_test * fixing analyze error. * Fixing analyze error in the test file * Fixing comments on the new method * Adding more closing connection examples. * Indentation change * Remove auto-add white space * Changing the oncloseconnection behaviour to stop editing. Updating the tests * Addressing PR comments. Added explicit log for method channnels to the tests. Added comments to the interfaces. * add more documentation

nturgut requested review from goderbauer and mdebbar October 25, 2019 00:34

fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Oct 25, 2019

googlebot added the cla: yes label Oct 25, 2019

nturgut mentioned this pull request Oct 25, 2019

[DRAFT] Calling onConnectionClosed when the input element is blurred. flutter/engine#13344

Closed

mdebbar reviewed Oct 25, 2019

View reviewed changes

packages/flutter/test/widgets/editable_text_test.dart Outdated Show resolved Hide resolved

goderbauer reviewed Oct 25, 2019

View reviewed changes

Nurhan Turgut added 2 commits October 25, 2019 18:09

Adding handling of TextInputClient.onConnectionClosed messages handli…

97b2694

…ng to Framework

Adding more test cases for closing connection to editable_text_test

b79771a

nturgut force-pushed the clean_client_framework2 branch from fbc80b9 to b79771a Compare October 26, 2019 03:42

nturgut added 6 commits October 28, 2019 08:13

fixing analyze error.

6a0cf40

Fixing analyze error in the test file

ed43aea

Fixing comments on the new method

32c62ce

Adding more closing connection examples.

27e586f

Indentation change

a64e02d

Remove auto-add white space

d00542f

nturgut requested review from goderbauer and mdebbar October 28, 2019 16:21

Piinks added a: text input Entering text in a text field or keyboard related problems platform-web Web applications specifically labels Oct 29, 2019

Changing the oncloseconnection behaviour to stop editing. Updating th…

6bf1ad4

…e tests

xster mentioned this pull request Oct 30, 2019

[Add2App] It's possible to lose the ability to input characters using the keyboard on Android #35054

Closed

mdebbar approved these changes Oct 30, 2019

View reviewed changes

goderbauer reviewed Oct 30, 2019

View reviewed changes

packages/flutter/lib/src/services/text_input.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/services/text_input.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/editable_text.dart Outdated Show resolved Hide resolved

dnfield reviewed Oct 31, 2019

View reviewed changes

packages/flutter/lib/src/services/text_input.dart Show resolved Hide resolved

dnfield previously requested changes Oct 31, 2019

View reviewed changes

dnfield reviewed Oct 31, 2019

View reviewed changes

Addressing PR comments. Added explicit log for method channnels to th…

2f88cbf

…e tests. Added comments to the interfaces.

nturgut requested a review from goderbauer November 1, 2019 16:09

goderbauer reviewed Nov 1, 2019

View reviewed changes

packages/flutter/lib/src/services/text_input.dart Show resolved Hide resolved

packages/flutter/lib/src/services/text_input.dart Show resolved Hide resolved

add more documentation

bf16646

goderbauer approved these changes Nov 4, 2019

View reviewed changes

goderbauer added the c: API break Backwards-incompatible API changes label Nov 4, 2019

nturgut added the waiting for tree to go green label Nov 4, 2019

nturgut merged commit 97cf355 into flutter:master Nov 4, 2019

nturgut mentioned this pull request Dec 13, 2019

Calling onConnectionClosed when the input element is blurred flutter/engine#14484

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Conversation

nturgut commented Oct 25, 2019 • 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!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthew-carroll commented Oct 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Oct 25, 2019

Uh oh!

nturgut commented Oct 30, 2019

Uh oh!

matthew-carroll commented Oct 30, 2019

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

nturgut commented Oct 31, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

goderbauer Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Nov 4, 2019

Uh oh!

nturgut commented Nov 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nturgut commented Oct 25, 2019 •

edited

Loading