Text field height fix - #55911
Text field height fix#55911
Conversation
…ght unless contentPadding given
|
@HansMuller This makes an attempt at improving our problems with collapsed and isDense (while fixing a regression). The idea is to reduce confusion by always respecting contentPadding when it's given and not depending on isDense. I left a comment on the issue that summarizes the background of this problem. |
| final double minContainerHeight = decoration.isDense || expands | ||
| ? 0.0 | ||
| : kMinInteractiveDimension + densityOffset.dy; | ||
| final double minContainerHeight = decoration.isDense |
There was a problem hiding this comment.
This expression and the following one might be a little easier to read if allowed the lines to get a little wider. Doing it this way makes the similarity of these statements easier to see.
final double minContainerHeight = expands || decoration.isDense || decoration.contentPadding != null
? 0.0
: kMinInteractiveDimension + densityOffset.dy;| ? maxContainerHeight | ||
| : math.min(math.max(contentHeight, minContainerHeight), maxContainerHeight); | ||
|
|
||
| // Ensure the text is vertically centered in cases where the content is |
There was a problem hiding this comment.
Do we really want to lose this comment?
| : const EdgeInsets.fromLTRB(12.0, 24.0, 12.0, 16.0)); | ||
| } | ||
| final double floatingLabelHeight = !decoration.isCollapsed && !border.isOutline | ||
| // 4.0: the vertical gap between the inline elements and the floating label. |
…lds to be smaller than the min
202f63f to
93f2a23
Compare
|
@HansMuller Ready for re-review. I did a much simplified solution by just considering isCollapsed remove the minimum height, as we discussed. It fixes the regression. I'm checking the internal tests now. |
|
Seems like this doesn't cause any failures on internal tests. |
| int errorMaxLines, | ||
| bool hasFloatingPlaceholder, | ||
| FloatingLabelBehavior floatingLabelBehavior, | ||
| bool isCollapsed, |
There was a problem hiding this comment.
The doc comment is no longer accurate.
There was a problem hiding this comment.
Thanks for catching that! It makes me worry about people relying on the old behavior, though I think technically it's not a breaking change. I wonder why isCollapsed was not passed through in the first place.
| ), | ||
| ); | ||
|
|
||
| // Overall height should be 18dps, but the min is kMinInteractiveDimension |
There was a problem hiding this comment.
The requested overall height is 18dps, however the min height is kMinInteractiveDimension
because neither isDense or isCollapsed are true.
| }); | ||
|
|
||
| testWidgets('contentPadding smaller than kMinInteractiveDimension', (WidgetTester tester) async { | ||
| const double verticalPadding = 1.0; |
| expect(tester.getSize(find.text('text')).height, 16.0); | ||
| expect(tester.getTopLeft(find.text('text')).dy, 16.0); | ||
| expect(getOpacity(tester, 'hint'), 0.0); | ||
| expect(getBorderWeight(tester), 1.0); |
There was a problem hiding this comment.
For the sake of clarity, it would be nice to include a second part to this test that verifies that the height is 18 when collapsed is true. And a third part, same thing for isDense.
Description
A regression was introduced in #42449 where inputs with
InputDecoration.collapsedwere forced to have a height of at leastkMinInteractiveDimension. There was also a lot of confusion aroundcollapsed,isDense, andcontentPadding, as explained well in this issue comment. This PR fixes the regressionand deals with the confusion by respectingI've simplified the solution to simply allow small heights whencontentPaddingwhenever it is passed in, regardless of whetherisDenseorcollapsedwere used.isCollapsedis true due to breakages with the original solution.Related Issues
Closes #46160
Tests