Warns when single line text fields overflow #63639
Conversation
flutter-dashboard
bot
added
framework
|
Gold has detected about 15 untriaged digest(s) on patchset 5. |
LongCatIsLooong
force-pushed
the
report-text-field-insufficient-height
branch
from
0c237bb to
c12010f
Compare
|
Gold has detected about 15 untriaged digest(s) on patchset 5. |
LongCatIsLooong
force-pushed
the
report-text-field-insufficient-height
branch
from
c12010f to
4762175
Compare
|
Gold has detected about 15 untriaged digest(s) on patchset 6. |
2 similar comments
|
Gold has detected about 15 untriaged digest(s) on patchset 6. |
|
Gold has detected about 15 untriaged digest(s) on patchset 6. |
|
Gold has detected about 15 untriaged digest(s) on patchset 7. |
|
@justinmc @HansMuller I'd like to get a high-level review of this idea. Could there be cases where a single-line text field is intentionally constrained to a small height so the text gets cut off? |
flutter-dashboard
bot
added
the
a: tests
|
I'm on board with this at a high level. I like the way you implemented it, maybe it'll be useful elsewhere in the future. I can't think of a reason off the top of my head that you would want to purposely have a text field overflow, but interested to hear what Hans says. |
|
I put this on frob: cl/326752193. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
…-insufficient-height
…ngCatIsLooong/flutter into report-text-field-insufficient-height
…-insufficient-height
|
Gold has detected about 30 untriaged digest(s) on patchset 10. |
|
Internal tests seem to be passing. |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Just some small code changes and some questions, but overall this looks good. I think the approach of using composition and RenderConstraintsTransformBox is really solid and reusable. We'll have to watch for this breaking anything else.
How did the tests look when you used frob by the way? (I can't figure out how to see the test results)
| _constraintsTransform = _convertAxis; | ||
| } | ||
|
|
||
| /// The axis to retain constraints on, if any. |
There was a problem hiding this comment.
Is there a use for retaining constraints on neither axis?
There was a problem hiding this comment.
Don't think there's one in the framework or material or cupertino. But this PR does not change RenderUnconstrainedBox's API or functionality.
| if (constrainedAxis == null) { | ||
| return const BoxConstraints(); | ||
| } | ||
| switch (constrainedAxis) { | ||
| case Axis.horizontal: | ||
| return constraints.copyWith(maxHeight: double.infinity, minHeight: 0); | ||
| case Axis.vertical: | ||
| return constraints.copyWith(maxWidth: double.infinity, minWidth: 0); | ||
| } | ||
| assert(false); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Could you use default to handle constrainedAxis == null and then just do everything in the switch statement?
There was a problem hiding this comment.
per the style guide default should be avoided: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-using-if-chains-or--or--with-enum-values
There was a problem hiding this comment.
Ah got it, that makes sense.
| TextSpan(text: 'outer', style: TextStyle(fontSize: 20)), | ||
| WidgetSpan( | ||
| child: SizedBox(width: 70, height: 25, child: TextField()), | ||
| child: SizedBox(width: 70, height: 55, child: TextField()), |
There was a problem hiding this comment.
Were these being clipped before?
There was a problem hiding this comment.
Yes, 55 is the minimum height iirc
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM as long as Frob says it's not going to break any tests 👍
There was a problem hiding this comment.
Alone on an Infinite Canvas with No Constraints will be the name of my breakout studio album.
Co-authored-by: Justin McCandless <[email protected]>
LongCatIsLooong
force-pushed
the
report-text-field-insufficient-height
branch
from
f63a552 to
5047a7a
Compare
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
This is a nice piece of work! Just some small-time feedback, mostly wordsmithing.
| /// its child using the resulting [BoxConstraints], treating any overflow | ||
| /// as error. | ||
| /// | ||
| /// This container sizes its child using a normalized [BoxConstraints] created |
There was a problem hiding this comment.
It wasn't clear that "normalized" in this context means "satisfies BoxConstraints.isNormalized". Maybe leave the qualifier out here and document that we're going to assert isNormalized elsewhere?
| /// printed on the console, and black and yellow striped areas will appear where | ||
| /// the overflow occurs. | ||
| /// | ||
| /// This widget can be used to allow the child to enforce some of its intrinsic |
There was a problem hiding this comment.
"some of its intrinsic sizing ..."? This widget doesn't necessarily know what the child's intrinsic sizing rules are?
Maybe just say: here's an example.
Ideally, you'd provide a rule of thumb about when to use this widget.
| /// enough space: | ||
| /// | ||
| /// {@tool snippet} | ||
| /// This snippet allows the [Card] to be at least its intrinsic height high, but |
There was a problem hiding this comment.
Maybe this is a little clearer?
This snippet guarantees that the child [Card]'s height will be at least as tall as its intrinsic height. Unlike an [UnconstrainedBox], the child will be taller if the parent's max height is constrained. If the parent's max height is constrained to be less than the Card's intrinsic height then a warning will be given in debug mode.
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [BoxConstraints], the type constraints of constraints this widget can be |
There was a problem hiding this comment.
Probably don't need this reference (plus "the type constraints of constraints" is a typo), since the API includes BoxConstraints.
| /// | ||
| /// * [BoxConstraints], the type constraints of constraints this widget can be | ||
| /// used to manipulate. | ||
| /// * [ConstrainedBox], which renders a box which imposes constraints |
There was a problem hiding this comment.
A widget that imposes additional constraints on its child.
Best to lift these one-liners from the existing API doc; don't want to explain "boxes" vs widget here.
| /// [AlignmentDirectional]. | ||
| final TextDirection textDirection; | ||
|
|
||
| /// The alignment to use when laying out the child. |
There was a problem hiding this comment.
Shouldn't this explain that it's for cases where the parent isn't the same size as the child?
| /// The alignment to use when laying out the child. | ||
| /// | ||
| /// If this is an [AlignmentDirectional], then [textDirection] must not be | ||
| /// null. |
There was a problem hiding this comment.
Why don't we use Directionality.of(context) if the textDirection parameter is null?
There was a problem hiding this comment.
It does. Copied the docs from unconstrainedbox and didn't check.
| final AlignmentGeometry alignment; | ||
|
|
||
| /// @{template flutter.widgets.constraintsTransform} | ||
| /// The function used to transform the [BoxConstraints] imposed on [child]. |
There was a problem hiding this comment.
If would be a little more helpful to explain that the we're transforming the parent's constraints and then using them to constrain the child's size.
| /// The function used to transform the [BoxConstraints] imposed on [child]. | ||
| /// | ||
| /// Must not be null. The function must not return null. The result will be | ||
| /// normalized. |
There was a problem hiding this comment.
will be or must be?
AFAICT we don't normalize incoming constraints. Is this really necessary?
There was a problem hiding this comment.
Makes sense. Removing the normalize() call from performLayout.
| } | ||
| constraintsTransform ??= _unconstrained; | ||
|
|
||
| return ConstraintsTransformBox( |
…-insufficient-height
|
updated |
…-insufficient-height
This reverts commit 0ab5ecc.
This reverts commit 6536f65.
…lutter#64573) This reverts commit 0ab5ecc.
…lutter#64573) This reverts commit 0ab5ecc.
6 participants
Description
Warns if a single line text field's parent is not able to provide sufficient height to accommodate the text field. So developers don't get surprised when they see weird clipping. Set
clipBehaviortoClip.noneto opt-out (TextFieldcurrently doesn't seem to have aclipBehaviorparameter).Related Issues
Alleviates #51257. Not sure that issue really needs a fix. It happens because we clip the
RenderParagraphwhen the text gets too long along the scroll axis (horizontal for single line and vertical for multiline), but as a side effect the other axis gets clipped as well.Fixes #35706
Tests
I added the following tests:
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.