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] introduce TextInputControl #76072

Merged
merged 8 commits into from
Oct 4, 2022

Conversation

jpnurmi
Copy link
Member

@jpnurmi jpnurmi commented Feb 15, 2021

The functionality provided by this PR makes it possible to replace the default platform text input control with a custom in-app virtual keyboard on platforms that do not have a system-provided keyboard.

This PR adds:

  • TextInputControl, a mixin that receives text input state changes and visual text input control requests from the framework, and communicates user input back to the framework.
  • TextInput.setInputControl(), a method to set a custom (or null, which solves Request: Add option to not open keyboard on TextField focus #16863) text input control
  • TextInput.restorePlatformTextInputControl(), a method to restore the default platform text input control

Based on the lessons learned from the previous failed attempt (#69146), the method channel-based platform text input control continues to receive text input state changes even when a custom text input control is installed. This ensures that after installing a custom input control, the platform text input continues to work and the underlying input model stays in sync.

Fixes #68988

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 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 Hixie said the PR is test-exempt.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 15, 2021
@google-cla google-cla bot added the cla: yes label Feb 15, 2021
@jpnurmi
Copy link
Member Author

jpnurmi commented Feb 15, 2021

CC @LongCatIsLooong is it right that autofill should be only done for the current control?

I'm still not 100% sure about it, but I have considered autofill a visual thing because the TextInputConnection.requestAutofill() documentation states:

Requests the system autofill UI to appear.

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.

Overall I think this approach looks good and should handle the problems we encountered in the last PR. It's nice that this will make it possible to solve #16863.

I think I definitely want to see an example of what a virtual keyboard would look like using this code. I commented about that. Otherwise my comments are just a few questions and small stuff.

I'm interested in @LongCatIsLooong's opinion from an autofill perspective.

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/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/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/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/services/text_input.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/services/text_input.dart Outdated Show resolved Hide resolved
@jpnurmi
Copy link
Member Author

jpnurmi commented Feb 26, 2021

Thank you for the review!

I'm interested in @LongCatIsLooong's opinion from an autofill perspective.

I noticed this comment in TextInputConnection.requestAutofill() docs:

Requests the system autofill UI to appear.

It would be a problem if multiple text input controls tried to show autofill UI, which is why I ended up placing the autofill functionality to TextInputControl instead of TextInputHandler.

@goderbauer
Copy link
Member

(PR triage): @jpnurmi @LongCatIsLooong @justinmc Are there still plans for this before it becomes too stale?

@jpnurmi
Copy link
Member Author

jpnurmi commented Mar 24, 2021

I'll get back to this. I was waiting for comments/feedback because I hadn't noticed the thumbs up above.

@jpnurmi jpnurmi changed the title [text_input] introduce TextInputHandler & TextInputControl [text_input] introduce TextInputControl Mar 25, 2021
@justinmc
Copy link
Contributor

@jpnurmi Can you fix the conflicts with master? Sorry, that's probably our fault for not giving you a final review fast enough.

@justinmc
Copy link
Contributor

justinmc commented Apr 6, 2021

There's a failure but it looks like an infrastructure problem, rerunning...

@jpnurmi
Copy link
Member Author

jpnurmi commented Apr 16, 2021

Is there anything I could do to get things rolling?

@LongCatIsLooong
Copy link
Contributor

Sorry for the delay. Looks good to me in general.

@LongCatIsLooong
Copy link
Contributor

Hi @mdebbar would you like to take a look at this? I think this change may have implications on the web.

@jpnurmi
Copy link
Member Author

jpnurmi commented May 13, 2021

This PR is based on the idea that the show() and hide() methods are only called on the currently active text input control. This way, only one keyboard pops up. However, Flutter for Web requires show() to be called, or else it won't enter the editing state at all:

https://github.com/flutter/engine/blob/eb658e840d92ab4e8a9862e912ab6a4351f5aa90/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L1414

In other words, installing a custom text input control on Web blocks platform text input e.g. physical keyboard.

@jpnurmi
Copy link
Member Author

jpnurmi commented May 13, 2021

Would it be an option to change it so that the editing state is controlled from setClient() and clearClient() instead, and only editing focus is set from show()?

https://github.com/flutter/engine/blob/eb658e840d92ab4e8a9862e912ab6a4351f5aa90/lib/web_ui/lib/src/engine/text_editing/text_editing.dart#L1000

@jpnurmi
Copy link
Member Author

jpnurmi commented May 26, 2021

A potential web-compatible solution:

  • introduce TextInputType.none in flutter/services
  • when a custom text input control is in use, pass "none" in the text input configuration to the platform text input control
  • adapt the engine for all platforms:
    • web: set the "inputmode" attribute to "none"
    • others: don't show the keyboard when the input type is "none"
  • bonus: it will be possible to disable the keyboard for individual TextFields

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/inputmode

@jpnurmi jpnurmi force-pushed the text-input-handler branch from 902bde0 to 09a4314 Compare May 27, 2021 09:05
jpnurmi added a commit to jpnurmi/engine that referenced this pull request May 27, 2021
jpnurmi added a commit to jpnurmi/engine that referenced this pull request May 27, 2021
@jpnurmi
Copy link
Member Author

jpnurmi commented May 27, 2021

Experimental engine changes for Linux: jpnurmi/engine@387e5c4

jpnurmi added a commit to jpnurmi/engine that referenced this pull request May 27, 2021
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.

LGTM 👍 , but I think we should add a test for the example if you're up for it. The situation with breaking changes looks good now!

@jpnurmi
Copy link
Member Author

jpnurmi commented Oct 6, 2022

@justinmc thank you! 🎉

@Eerey
Copy link

Eerey commented Oct 13, 2022

@itsjustkevin so this PR got removed and no follow up? Why does it break? We are longing for this feature.

@Eerey
Copy link

Eerey commented Oct 13, 2022

@itsjustkevin @jpnurmi Can we/the community help on the related issues on why it causes breakages?

@jpnurmi
Copy link
Member Author

jpnurmi commented Oct 13, 2022

@Eerey I wasn't aware of the revert but notice that it was closed without merging. TextInputControl still exists in the latest main branch:

@itsjustkevin What was the problem? Is this change still causing issues somewhere or was it a false alarm?

@zanderso
Copy link
Member

This was a breaking change. I'm not sure why this PR didn't fail on the customer testing shard, but it is now blocking Engine rolls into the framework. See: #113666. In particular super_editor is failing due to https://github.com/superlistapp/super_editor/blob/main/super_editor/lib/src/default_editor/document_input_ime.dart#L75.

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 d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support/enablers for custom/in-app virtual keyboards