Introduce `MaxLengthEnforcement` by AlexV525 · Pull Request #68086 · flutter/flutter

AlexV525 · 2020-10-14T07:30:53Z

Description

In #63754 a new behavior about the composing TextEditingValue for the LengthLimitingTextInputFormatter was introduced, but it's not optional which somehow breaks the original behavior for the limiting.

This PR:

brings a new enum MaxLengthEnforcement.
deprecate maxLengthEnforced that exist in text fields.

New behaviors for different platforms:

Android, Windows: MaxLengthEnforcement.enforced. The native behavior of these platforms is enforced. The composing will be handled by the IME while users are entering CJK characters.
iOS: MaxLengthEnforcement.truncateAfterCompositionEnds. iOS has no default behavior and it requires users implement the behavior themselves. Allow the composition to exceed to avoid breaking CJK input.
Web, macOS, linux, fuchsia: MaxLengthEnforcement.truncateAfterCompositionEnds. These platforms allow the composition to exceed by default.

Related Issues

Fixes #67898 .

Tests

I added the following tests:

Grouped max length tests in editable text test.
MaxLengthEnforcement tests in cupertino/material text field tests.
"enforced composing truncated" in editable text test.
"composing range removed if it's overflowed the truncated value's length" in editable text test.
truncate behaviors on different platforms.

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.
- I wrote a design doc: https://docs.google.com/document/d/1S900lQaTPOsePnIeE7o3-Ia8f39VZaSv0JnjNyBfCvU
- I got input from the developer relations team, specifically from: @LongCatIsLooong @justinmc
- I wrote a migration guide: Add breaking change "Use maxLengthEnforcement instead of maxLengthEnforced" website#4883

…iting-formatter

AlexV525 · 2020-10-15T03:01:47Z

Should we move these tests into text formatter test?

justinmc · 2020-10-16T00:41:35Z

I think I'm in favor of this solution to the problems discussed in #67898. Limiting length with/without composing characters both seem to have valid uses, so it makes sense to let developers choose which they want. I'm also interested in what @LongCatIsLooong thinks.

Maybe instead of a bool, this should be combined with maxLengthEnforced and made into an enum? Like maxLengthEnforcement or something like that. Then deprecate maxLengthEnforced. Then we don't have to worry about both bools being true, and it's future-proof if more cases appear later.

AlexV525 · 2020-10-16T01:44:48Z

Maybe instead of a bool, this should be combined with maxLengthEnforced and made into an enum?

Before, maxLengthEnforced in TextField controls whether a LengthLimitingTextInputFormatter will be inserted or not. If not, it will be only an error tip when the value reached limitation.

If it can convert to enum, I prefer maxLengthEnforcement too, and there will be three values for it:

none or regular: Same as the current truncate behavior, as default.
warningOnly: Same as maxLength == x && maxLengthEnforced == false.
truncateComposing: Apply length limit on composing value too.

Does it seems valid?

LongCatIsLooong

This doesn't seem to be a breaking change? See https://github.com/flutter/flutter/wiki/Tree-hygiene#1-determine-if-your-change-is-a-breaking-change, the tests seem to be passing?

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

AlexV525 · 2020-10-16T04:31:42Z

This doesn't seem to be a breaking change? See https://github.com/flutter/flutter/wiki/Tree-hygiene#1-determine-if-your-change-is-a-breaking-change, the tests seem to be passing?

It was suggested from #67898 (comment)

LongCatIsLooong · 2020-10-16T04:36:13Z

@AlexV525 Ah I meant this PR per se is not breaking (at least customer_testing is not failing).

Making maxLengthEnforced an enum sounds good to me.

none or regular: Same as the current truncate behavior, as default.
I think none is already covered by setting maxLength to null or -1?

AlexV525 · 2020-10-16T04:43:48Z

I think none is already covered by setting maxLength to null or -1?

So maybe regular is more fit for the case, which means truncated while it's not composing.

AlexV525 · 2020-10-16T04:46:41Z

Ah I meant this PR per se is not breaking (at least customer_testing is not failing).

😕 Then how should we announce this...? Or not?

LongCatIsLooong · 2020-10-16T04:59:54Z

confused Then how should we announce this...? Or not?

Sorry I got ahead of myself in that issue. If we deprecate the maxLengthEnforced parameter then it will be a breaking change (when we eventually remove the parameter it's going to break code that's still using it).

AlexV525 · 2020-10-17T03:19:56Z

Both TextField and CupertinoTextField have maxLengthEnforced, is it okay to put the enum in editable_text.dart ? Maybe text_formatter.dart ?

LongCatIsLooong · 2020-10-17T05:53:12Z

Yeah makes sense to put it in text_formatter.dart since that's where the length limiting formatter is.

justinmc

Mostly just some quick formatting and docs fixes. I think the behavior looks great now, thanks for making those changes.

packages/flutter/lib/src/cupertino/text_field.dart

packages/flutter/lib/src/material/text_field.dart

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

packages/flutter/test/cupertino/text_field_test.dart

justinmc · 2020-12-07T18:12:04Z

In order to get around the customer test failures (which are coming from the gallery), this is what we'll have to do:

Remove the @Deprecated flags from this PR, then merge it when it goes green.
Submit a PR in the Flutter gallery to migrate from maxLengthEnforced to maxLengthEnforcement here.
Submit another PR here to add back the deprecation.

AlexV525 · 2020-12-08T05:11:48Z

There is another serious issue with characters calculation when I was testing "Composing range handled correctly when it's overflowed" in editable text tests.

The calculation of newValue.text is resulting a wrong length, which should be 6. This will cause the test fail but not related to the current PR. /cc @LongCatIsLooong @justinmc

Previously the test won't failed because the range is completely overflowed, but when I tried with a shorter text this happened. So I'm going to keep the current working test and separate this to another issue or PR.

justinmc · 2020-12-08T05:53:54Z

Which test is it that fails like that? I don't follow exactly why it's failing now.

In general though I think that's a good plan, if this is unrelated. You should create a new issue and then add a TODO in the code with a link to the issue.

AlexV525 · 2020-12-08T05:55:26Z

Which test is it that fails like that? I don't follow exactly why it's failing now.

In general though I think that's a good plan, if this is unrelated. You should create a new issue and then add a TODO in the code with a link to the issue.

"Composing range handled correctly when it's overflowed" in editable_text_tests. I've saw some related issues before, I'll file a new one if it doesn't exist.

fluttergithubbot · 2020-12-08T06:23:04Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

justinmc

LGTM 👍

packages/flutter/lib/src/cupertino/text_field.dart

packages/flutter/lib/src/material/text_field.dart

packages/flutter/test/widgets/editable_text_test.dart

LongCatIsLooong

I think this is a good idea! All the input methods I tried on Android adhere to Gboard's convention. Left a few comments about nits regarding documentation.

packages/flutter/lib/src/material/text_field.dart

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

LongCatIsLooong

Sorry for the confusion. LGTM with nits.

AlexV525 · 2020-12-09T23:30:23Z

Thanks for everyone that make efforts to this PR to make it solid! It's a huge change and hope it can truly solve composing issues.

✨ Add composingMaxLengthEnforced to length limiting formatter.

fc2a57d

flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Oct 14, 2020

google-cla bot added the cla: yes label Oct 14, 2020

AlexV525 added 4 commits October 14, 2020 17:26

⚡ Handle composing state in truncate method.

70ef51f

🎨 Group length formatter's tests.

cf57b32

✅ Add enforced composing truncated test.

d1165ed

💡 Documented keepComposing for truncate method.

4ecd768

goderbauer requested a review from justinmc October 14, 2020 21:40

Merge remote-tracking branch 'upstream/master' into update-length-lim…

c7e3de9

…iting-formatter

AlexV525 mentioned this pull request Oct 15, 2020

Add breaking change "Use maxLengthEnforcement instead of maxLengthEnforced" flutter/website#4883

Merged

AlexV525 marked this pull request as ready for review October 15, 2020 02:49

AlexV525 requested a review from LongCatIsLooong October 15, 2020 02:49

AlexV525 removed the work in progress; do not review label Oct 15, 2020

LongCatIsLooong reviewed Oct 16, 2020

View reviewed changes

AlexV525 marked this pull request as draft October 16, 2020 13:25

AlexV525 added 4 commits October 17, 2020 16:39

✨ Introduce MaxLengthEnforcement .

39251a3

🗑 Deprecate maxLengthEnforced .

d19d06b

🚀 Take care of keepComposingRange.

9a5a2b1

📝 Link doc macro.

7666904

AlexV525 added 2 commits December 5, 2020 20:16

💚 Try to fix web tests failure.

9161918

✅ Improve tests for cupertino & material text fields.

9270bac

justinmc reviewed Dec 7, 2020

View reviewed changes

AlexV525 added 4 commits December 8, 2020 09:46

👌 Address @justinmc 's format suggestions.

46daac0

✅ Add type in the middle test.

21a9875

🔥 Remove deprecation.

bbb26b7

🚑 Fix wrong enum usage in enforced test.

9806477

🐛 Fix invalid composing range test.

757a771

AlexV525 added the waiting for tree to go green label Dec 8, 2020

fluttergithubbot removed the waiting for tree to go green label Dec 8, 2020

justinmc approved these changes Dec 8, 2020

View reviewed changes

packages/flutter/lib/src/cupertino/text_field.dart Show resolved Hide resolved

packages/flutter/lib/src/material/text_field.dart Show resolved Hide resolved

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

LongCatIsLooong reviewed Dec 8, 2020

View reviewed changes

AlexV525 added 2 commits December 9, 2020 13:30

📝 ✅ Update docs and test.

2c819fa

♻️ Make enforcement's getter more reliable.

95a9483

Richyraymond approved these changes Dec 9, 2020

View reviewed changes

LongCatIsLooong approved these changes Dec 9, 2020

View reviewed changes

AlexV525 merged commit 135a8c2 into flutter:master Dec 9, 2020

AlexV525 mentioned this pull request Dec 10, 2020

Deprecate maxLengthEnforced for text fields #72043

Merged

10 tasks

AlexV525 deleted the update-length-limiting-formatter branch December 12, 2020 01:14

vanlooverenkoen mentioned this pull request Dec 20, 2020

Max length for inputfield not working correctly icapps/flutter-template#107

Closed

justinmc mentioned this pull request Jan 19, 2021

[1.22.5] TextField doesn't respect maxLength limit #74150

Closed

justinmc mentioned this pull request Mar 24, 2021

Don't explicitly commit the composing text in _finalizeEditing #78926

Closed

8 tasks

Piinks mentioned this pull request Feb 16, 2022

Remove deprecated CupertinoTextField, TextField, TextFormField maxLengthEnforced #98539

Merged

8 tasks

Conversation

AlexV525 commented Oct 14, 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!

AlexV525 commented Oct 15, 2020

Uh oh!

justinmc commented Oct 16, 2020

Uh oh!

AlexV525 commented Oct 16, 2020

Uh oh!

LongCatIsLooong left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexV525 commented Oct 16, 2020

Uh oh!

LongCatIsLooong commented Oct 16, 2020

Uh oh!

AlexV525 commented Oct 16, 2020

Uh oh!

AlexV525 commented Oct 16, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

LongCatIsLooong commented Oct 17, 2020

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinmc commented Dec 7, 2020

Uh oh!

AlexV525 commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Dec 8, 2020

Uh oh!

AlexV525 commented Dec 8, 2020

Uh oh!

fluttergithubbot commented Dec 8, 2020

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

AlexV525 commented Dec 9, 2020

Uh oh!

Reviewers

AlexV525 commented Oct 14, 2020 •

edited

Loading

LongCatIsLooong left a comment •

edited

Loading

LongCatIsLooong commented Oct 16, 2020 •

edited

Loading

AlexV525 commented Oct 17, 2020 •

edited

Loading

AlexV525 commented Dec 8, 2020 •

edited

Loading