-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix: On the Web, cannot support multiline inputting when registering customized TextInputControl #139446
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
Conversation
* added forceMultiline to none input type
|
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. |
| if (TextInput._instance._currentControl != _PlatformTextInputControl.instance) { | ||
| json['inputType'] = TextInputType.none.toJson(); | ||
| json['inputType'] = TextInputType.noneWithOptions( | ||
| forceMultiline: configuration.inputType == TextInputType.multiline, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand we should still continue to send TextInputType.none in both the multi-line case and the single-line case of custom text input controls because that prevents the keyboard from being shown. Won't this make the keyboard show again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the forceMultiline flag can be processed by the web embedder to decide whether to use textarea or input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Renzo-Olivares Yes, this won't show the keyboard again. This still sends TextInputType.none to the engine. And also, send forceMultiline stated in TextInputType.none.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the PR in the web engine to handle forceMultiline . But have not reviewed it yet.
| /// Optimize for none information. | ||
| /// | ||
| /// Requests a none keyboard with additional settings. | ||
| /// The [forceMultiline] parameter is optional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment about how forceMultiline only applies in a web context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we should probably explain why someone might use this i.e. the custom text input control use-case.
| /// | ||
| /// This flag is only used for the [none] input type, otherwise `null`. | ||
| /// Use `const TextInputType.noneWithOptions(forceMultiline: true)` to set this. | ||
| final bool? forceMultiline; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// [TYPE_TEXT_VARIATION_POSTAL_ADDRESS](https://developer.android.com/reference/android/text/InputType#TYPE_TEXT_VARIATION_POSTAL_ADDRESS). | ||
| static const TextInputType streetAddress = TextInputType._(9); | ||
|
|
||
| /// Prevent the OS from showing the on-screen virtual keyboard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment similar to how TextInputType number has in this file.
/// Additional options, such as decimal point and/or positive/negative
/// signs, can be requested using [TextInputType.numberWithOptions].
| const TextInputType._(this.index) | ||
| : signed = null, | ||
| decimal = null; | ||
| decimal = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On looking at the _configurationToJson change, this doesn't have to change the public interface at all I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. I will try it on _configurationToJson and just change the JSON that is sent to the engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I open a new PR. Just added forceMultiline flag to JSON. @Renzo-Olivares @LongCatIsLooong please have a review.
|
replace with #140356. |
For #125875 and flutter/engine#45522 According to [this discussion](#139446 (comment)), Just added `forceMultiline` to JSON on `_configurationToJson`. @LongCatIsLooong @Renzo-Olivares
For #125875 and flutter/engine#45522
According to this discussion, I have added
forceMultilinetoTextInputType.none.When we customize
TextInputControl,_PlatformTextInputControlwill sendTextInputType.nonewithforceMultilineflag to the engine.