Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Text Input] Only unfocus when receive connectionClosed on platform except web #123929

Merged
merged 5 commits into from
Apr 10, 2023

Conversation

luckysmg
Copy link
Contributor

@luckysmg luckysmg commented Apr 1, 2023

One of #123523

Currently from engine side, in web engine we call connectionClosed from
https://github.com/flutter/engine/blob/b789b197c2188717dd48a15c2188cb1a1300d397/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L2162

But no call from other platform. When on platform like iOS or android, we actually expect a simple unfocus operation without calling other things.

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], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Apr 1, 2023
@luckysmg luckysmg changed the title [Text input] Only unfocus when receive connectionClosed on platform except web [Text Input] Only unfocus when receive connectionClosed on platform except web Apr 1, 2023
@luckysmg luckysmg marked this pull request as ready for review April 3, 2023 02:04
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that nothing in _finalizeEditing needs to be called when the connection is closed? There could be people that are relying on that behavior. It's reassuring that no tests have failed, though. At least no one in the past has had a strong enough opinion about this to add a test for it.

packages/flutter/test/widgets/editable_text_test.dart Outdated Show resolved Hide resolved
@luckysmg
Copy link
Contributor Author

luckysmg commented Apr 4, 2023

Emm seems google test failed again😳😳

@goderbauer
Copy link
Member

(triage) On first look, the google testing failures look unrelated. Can you rebase this PR with the latest master to trigger another run?

@luckysmg
Copy link
Contributor Author

luckysmg commented Apr 5, 2023

Are you sure that nothing in _finalizeEditing needs to be called when the connection is closed

@justinmc
For other platform except web, no call from engine. This will be called when platform close the connection first.
If we call _finalizeEditing, if onEditComplete is not null, it will execute so the text field will not focus, and if onSubmit passed, it will be called too. But when platform close first, we just need a unfocus without submit anything. So I think this makes sense.

@luckysmg
Copy link
Contributor Author

luckysmg commented Apr 5, 2023

@goderbauer google test still failed😳😳

@goderbauer
Copy link
Member

The Google Testing service is having some issues right now. We'll have to wait until those are fixed. Sorry about that.

@luckysmg
Copy link
Contributor Author

All tests passed. Applying label for merging. ^_^

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2023
exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
…xcept web (flutter#123929)

[Text Input] Only unfocus when receive connectionClosed on platform except web
auto-submit bot pushed a commit that referenced this pull request Sep 28, 2023
…r tabs on mobile web (#134870)

This PR fixes #134846. As discussed in the issue, the onSubmitted callback of a TextField is called when the browser switches tabs or is sent to the background if the flutter app is running in any mobile browser (desktop browsers are not affected). Furthermore there is no straight forward way to distinguish between onSubmitted being called because the user pressed the enter key and it being called because the user switched tabs. For example in a chat app this would cause a message to be sent when the user submits the text by pressing "send" on the virtual keyboard as well as when the user switches to another tab. The later action is likely not so much intended.

The next section explains what causes the bug and explains the proposed fix.

## Bug Analysis
The root cause for this behaviour is line 3494 in editable_text.dart: https://github.com/flutter/flutter/blob/0b540a87f1be9a5bb7e550c777dfe5221c53a112/packages/flutter/lib/src/widgets/editable_text.dart#L3487-L3499
Only if the app is running on the web `_finalizeEditing` is called and this will then trigger the onSubmitted callback. If flutter is running on the web, there are only exactly 3 cases, in which the function is called. The following call trace analysis will describe why.
  - `connectionClosed()` is only called by in one location, `_handleTextInputInvocation` of the TextInput service.
https://github.com/flutter/flutter/blob/367203b3011fc1752cfa1f51adf9751d090c94e6/packages/flutter/lib/src/services/text_input.dart#L1896C12-L1899
  - In particular it is only called if the TextInput service receives a 'TextInputClient.onConnectionClosed' message from the engine.
  - The only location where the web part of the engine send this message is the `onConnectionClosed` function of the TextEditingChannel.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L2242-L2254
  - `onConnectionClosed` in turn is only called by the `sendTextConnectionClosedToFrameworkIfAny` function of `HybridTextEditing`.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L2340-L2345

The function `sendTextConnectionClosedToFrameworkIfAny` is only called at 3 distinct locations of the web engine.

### 1. IOSTextEditingStrategy 
As described in the comment `sendTextConnectionClosedToFrameworkIfAny` is called if the browser is sent to the background or the tab is changed.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L1632-L1656

### 2. AndroidTextEditingStrategy
Same situation as for iOS. `sendTextConnectionClosedToFrameworkIfAny` is also called if `windowHasFocus` is false, which is the case if the browser is sent to background or the tab is changed.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L1773-L1785

### 3. TextInputFinishAutofillContext
This call seems to always happen when `finishAutofillContext` is triggered by the framework.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L2075-L2083

## Proposed Fix
The fixed proposed and implemented by this PR is to simply delete the call to`_finalizeEditing` in the `connectionClosed` function of editable_text.dart.
https://github.com/flutter/flutter/blob/0b540a87f1be9a5bb7e550c777dfe5221c53a112/packages/flutter/lib/src/widgets/editable_text.dart#L3487-L3499

The reasoning for this being:
   * `_finalizeEditing` is only called in `connectionClosed` for the web engine.
   * As explained by the trace analysis above, the web engine only triggers this `_finalizeEditing` call in 3 cases.
   * In the 2 cases for IOSTextEditingStrategy and AndroidTextEditingStrategy the web engine triggering the call only causes the undesired behaviour reported in the issue.
   * In the third case for TextInputFinishAutofillContext, I can't see a good reason why this would require calling `_finalizeEditing` as it only instructs the platform to save the current values. Other platforms also don't have anything that would trigger onSubmitted being called, so it seems safe to remove it.
   * For other platforms the onConnectionClosed function was recently incorporated to only unfocus the TextField. So removing the call `_finalizeEditing` unifies the platform behaviour. See also
     #123929
     flutter/engine#41500

*List which issues are fixed by this PR. You must list at least one issue.*
#134846

To simplify the evaluation, here are two versions of the minimal example given in the issue, build with the current master and with this PR applied:
current master: https://tauu.github.io/flutter-onsubmit-test/build/web-master/
current master   PR applied: https://tauu.github.io/flutter-onsubmit-test/build/web/
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
…r tabs on mobile web (flutter#134870)

This PR fixes flutter#134846. As discussed in the issue, the onSubmitted callback of a TextField is called when the browser switches tabs or is sent to the background if the flutter app is running in any mobile browser (desktop browsers are not affected). Furthermore there is no straight forward way to distinguish between onSubmitted being called because the user pressed the enter key and it being called because the user switched tabs. For example in a chat app this would cause a message to be sent when the user submits the text by pressing "send" on the virtual keyboard as well as when the user switches to another tab. The later action is likely not so much intended.

The next section explains what causes the bug and explains the proposed fix.

## Bug Analysis
The root cause for this behaviour is line 3494 in editable_text.dart: https://github.com/flutter/flutter/blob/0b540a87f1be9a5bb7e550c777dfe5221c53a112/packages/flutter/lib/src/widgets/editable_text.dart#L3487-L3499
Only if the app is running on the web `_finalizeEditing` is called and this will then trigger the onSubmitted callback. If flutter is running on the web, there are only exactly 3 cases, in which the function is called. The following call trace analysis will describe why.
  - `connectionClosed()` is only called by in one location, `_handleTextInputInvocation` of the TextInput service.
https://github.com/flutter/flutter/blob/367203b3011fc1752cfa1f51adf9751d090c94e6/packages/flutter/lib/src/services/text_input.dart#L1896C12-L1899
  - In particular it is only called if the TextInput service receives a 'TextInputClient.onConnectionClosed' message from the engine.
  - The only location where the web part of the engine send this message is the `onConnectionClosed` function of the TextEditingChannel.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L2242-L2254
  - `onConnectionClosed` in turn is only called by the `sendTextConnectionClosedToFrameworkIfAny` function of `HybridTextEditing`.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L2340-L2345

The function `sendTextConnectionClosedToFrameworkIfAny` is only called at 3 distinct locations of the web engine.

### 1. IOSTextEditingStrategy 
As described in the comment `sendTextConnectionClosedToFrameworkIfAny` is called if the browser is sent to the background or the tab is changed.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L1632-L1656

### 2. AndroidTextEditingStrategy
Same situation as for iOS. `sendTextConnectionClosedToFrameworkIfAny` is also called if `windowHasFocus` is false, which is the case if the browser is sent to background or the tab is changed.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L1773-L1785

### 3. TextInputFinishAutofillContext
This call seems to always happen when `finishAutofillContext` is triggered by the framework.
https://github.com/flutter/engine/blob/cbda68a720904137ee9dfdf840db323afcf52705/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L2075-L2083

## Proposed Fix
The fixed proposed and implemented by this PR is to simply delete the call to`_finalizeEditing` in the `connectionClosed` function of editable_text.dart.
https://github.com/flutter/flutter/blob/0b540a87f1be9a5bb7e550c777dfe5221c53a112/packages/flutter/lib/src/widgets/editable_text.dart#L3487-L3499

The reasoning for this being:
   * `_finalizeEditing` is only called in `connectionClosed` for the web engine.
   * As explained by the trace analysis above, the web engine only triggers this `_finalizeEditing` call in 3 cases.
   * In the 2 cases for IOSTextEditingStrategy and AndroidTextEditingStrategy the web engine triggering the call only causes the undesired behaviour reported in the issue.
   * In the third case for TextInputFinishAutofillContext, I can't see a good reason why this would require calling `_finalizeEditing` as it only instructs the platform to save the current values. Other platforms also don't have anything that would trigger onSubmitted being called, so it seems safe to remove it.
   * For other platforms the onConnectionClosed function was recently incorporated to only unfocus the TextField. So removing the call `_finalizeEditing` unifies the platform behaviour. See also
     flutter#123929
     flutter/engine#41500

*List which issues are fixed by this PR. You must list at least one issue.*
flutter#134846

To simplify the evaluation, here are two versions of the minimal example given in the issue, build with the current master and with this PR applied:
current master: https://tauu.github.io/flutter-onsubmit-test/build/web-master/
current master   PR applied: https://tauu.github.io/flutter-onsubmit-test/build/web/
MeandNi added a commit to MeandNi/flutter that referenced this pull request Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants