Increase TextField's minimum height from 40 to 48 #42449

justinmc · 2019-10-10T17:19:26Z

Description

Here are some screenshots for comparison. The red square is just a Container of size 48x48 as a reference. The green is the background of the TextField.

Before	After

Related Issues

Closes #41200

Tests

I added two tests to verify the minimum height is preserved for the default case and the case where text is very small. I also updated the numbers in tons of tests to reflect the new dimensions.

Breaking Change

This will break many visual diff tests due to the change in size of default TextFields and the change in the location of the baseline. I will follow the breaking changes guildelines after getting code review.

fluttergithubbot · 2019-10-10T17:19:28Z

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.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

justinmc · 2019-10-15T17:08:03Z

Some things I need to check on:

Are outlined text fields affected by this?
- Update: They are also limited to a minimum height of 48px. However, by default, they are already taller than this.
What happens if the textfield is forced to be very tall? Baseline alignment should work the same before and after this PR.
- Update: Behavior is the same. For multiline fields, vertical alignment is set by the textAlignVertical parameter. For TextFields with no border, the default is top. With a border, the default is center. If either type of field is made very tall by setting the font size to a large value, then the text remains vertically centered.

justinmc · 2019-10-17T00:07:09Z

packages/flutter/test/material/text_field_test.dart

@@ -7249,11 +7250,11 @@ void main() {

    // Tap the TextField to put the cursor into it and bring it into view.
    expect(scrollController.offset, 0.0);
-    await tester.tap(find.byType(TextField));
+    await tester.tapAt(tester.getTopLeft(find.byType(TextField)));


This seems weird... I don't know why it isn't receiving a tap with the previous line now. If anyone has a hunch please let me know.

justinmc · 2019-10-17T18:14:27Z

Goldens are being updated here: flutter/goldens#58

HansMuller

This looks good and its nice to see that such a big change can be accomplished with such a small change.

I think we might want to make isDense:true an exception to the min height = 48; just restore the original behavior in that case

jonahwilliams · 2019-10-17T22:23:56Z

This should probably utilize https://api.flutter.dev/flutter/material/MaterialTapTargetSize-class.html in some way to avoid extra padding where it doesn't matter, like desktop

…ug for 0.0 constrained inputs

justinmc · 2019-10-17T23:33:50Z

I discussed the MaterialTapTargetSize offline and we decided to leave it out of this PR in anticipation of some upcoming work for Material.

For flutter/flutter#42449

justinmc · 2019-10-18T20:57:33Z

Draft breaking change email:

I'm proposing to increase the default height of TextField by eight pixels in order to comply with the Material spec's minimum interactive size guideline.

Issue: #41200
PR: #42449

Currently, TextFields are 40px high with default settings. This change would set a minimum height of 48px for all TextFields where the isDense flag on InputDecorator is not true. Any field whose height is calculated to be less than 48px would be bumped up to 48 and its content vertically centered. Smaller TextFields could still be created with the isDense flag set to true.

Anyone that currently has a TextField that is below 48px high in their app, including those just using the default TextField settings, will see the size of these fields increase. These affected apps will need to update any visual diff tests covering these fields, and may also need to adjust their layouts to accommodate the larger fields.

If someone wishes to keep their TextFields at a height of less than 48px, they may do so by using the isDense flag. As a specific example, the following code will create a TextField that is still the original default height of 40px:

TextField(
style: TextStyle(height: 1.5),
decoration: InputDecoration(isDense: true),
)

Please let me know if you have any trouble migrating and need some help! I'm reachable at this email address or on Github in the given PR (@justinmc).

If anyone objects to this change or would like to discuss further, please comment in the linked PR.

Thanks,

Justin

justinmc · 2019-10-23T16:00:22Z

@Piinks If I still see goldens failures after merging my goldens PR, does that mean it's my fault and the goldens I added don't match?

Sorry, I'm forgetting if there's anything else I need to do to update the goldens here.

Piinks · 2019-10-23T16:29:05Z

Have the version numbers been incremented to match the golden file changes?

justinmc · 2019-10-23T16:35:24Z

That's it, I knew I forgot something. Thank you!

Piinks · 2019-10-28T17:18:20Z

#43371 has changed golden file testing, which will need a few minor adjustments here. When you resolve the merge conflicts, the goldens.version file will no longer exist.
It looks like this change affects already existing tests, so in order for them to pass pre-submit here, an ignore will have to be put in place for the affected tests in Flutter Gold.
As far as I can tell it looks like the affected tests are

text_field_cursor_test.material.0
text_field_cursor_test.material.1
text_field_opacity_test.0

Take a look at the updated Writing a golden file test for package:flutter, and let me know if I can help migrate this change to the new system. :)

HansMuller

LGTM

Piinks · 2019-10-29T20:32:15Z

Can we wait to land this until tomorrow?

The duration of the ignore should be set to cover the amount of time it will take to be triaged. Feel free to edit it to something conservative (a day or two is totally fine).

Also, there are performance improvements that should land by tomorrow AM that will make for a much better experience landing this:

justinmc · 2019-10-29T21:20:04Z

Yeah that's fine with me! This PR has a breaking change so I don't want to rush it anyway. I will hold off until tomorrow. I've updated the expiration to 2 days.

Piinks · 2019-10-31T19:51:57Z

Gold updates have landed, and I renewed the ignore on the dashboard. :)

This is being done to match the Material spec. It will likely break many visual diff tests.

TextField must be at least kMinInteractiveSize

2246cb7

justinmc added the work in progress; do not review label Oct 10, 2019

justinmc self-assigned this Oct 10, 2019

fluttergithubbot added f: material design framework labels Oct 10, 2019

googlebot added the cla: yes label Oct 10, 2019

Vertically center text when smaller than kMinInteractiveDimension

c58ee4a

justinmc force-pushed the text-field-interactive-size branch from 56fed8d to bf0aca1 Compare October 17, 2019 00:05

Fix tests in text_field.dart due to size change

7d53515

justinmc force-pushed the text-field-interactive-size branch from bf0aca1 to 7d53515 Compare October 17, 2019 00:05

justinmc commented Oct 17, 2019

View reviewed changes

justinmc added 2 commits October 17, 2019 09:40

Test min height

c164530

Update measures in input_decorator_test for bigger input

b8342d6

justinmc added c: API break will affect goldens labels Oct 17, 2019

justinmc requested a review from HansMuller October 17, 2019 18:10

justinmc changed the title ~~WIP TextField Interactive Size~~ TextField Interactive Size Oct 17, 2019

justinmc mentioned this pull request Oct 17, 2019

New goldens for increased TextField default height flutter/goldens#58

Merged

HansMuller reviewed Oct 17, 2019

View reviewed changes

HansMuller changed the title ~~TextField Interactive Size~~ Increase TextField's minimum height from 43 to 48 Oct 17, 2019

Do not enforce minimum height when isDense is true

13fc481

Restore values changed before I started ignoring isDense, and fix a b…

7a9e43c

…ug for 0.0 constrained inputs

Test isDense: true

0c46efa

Merge branch 'master' into text-field-interactive-size

daab3f0

justinmc removed the work in progress; do not review label Oct 18, 2019

HansMuller changed the title ~~Increase TextField's minimum height from 43 to 48~~ Increase TextField's minimum height from 40 to 48 Oct 21, 2019

Update goldens vesion

ebf2246

justinmc requested a review from Piinks as a code owner October 23, 2019 17:49

Merge branch 'master' into text-field-interactive-size

78fe05b

justinmc added 2 commits October 28, 2019 15:16

Merge branch 'master' into text-field-interactive-size

617d50f

Merge branch 'master' into text-field-interactive-size

5489f4e

HansMuller approved these changes Oct 29, 2019

View reviewed changes

justinmc merged commit 924e48e into flutter:master Nov 1, 2019

justinmc deleted the text-field-interactive-size branch November 1, 2019 16:36

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

Increase TextField's minimum height from 40 to 48 (flutter#42449)

da437da

This is being done to match the Material spec. It will likely break many visual diff tests.

giantsol mentioned this pull request Dec 15, 2019

Fix TextField too much height issue caused by upgrading Flutter SDK giantsol/Blue-Diary#123

Closed

shihaohong mentioned this pull request Dec 16, 2019

DropdownButtonFormField has additional padding on top and bottom after upgrade to v1.12.13+hotfix.5 #46844

Closed

This was referenced Dec 17, 2019

TextField decoration InputDecoration.collapsed has extra padding in new flutter versions. #46160

Closed

InputDecoration: looks like contentPadding is broken on master branch #44726

Closed

TextField contentPadding + Expanded error #46964

Closed

Piinks mentioned this pull request Apr 22, 2020

SliverFillRemaining Widget creates overflow with TextFields #54729

Closed

justinmc mentioned this pull request Apr 28, 2020

Text field height fix #55911

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase TextField's minimum height from 40 to 48 #42449

Increase TextField's minimum height from 40 to 48 #42449

justinmc commented Oct 10, 2019 •

edited

Loading

fluttergithubbot commented Oct 10, 2019

justinmc commented Oct 15, 2019 •

edited

Loading

justinmc Oct 17, 2019

justinmc commented Oct 17, 2019

HansMuller left a comment

jonahwilliams commented Oct 17, 2019

justinmc commented Oct 17, 2019

justinmc commented Oct 18, 2019 •

edited

Loading

justinmc commented Oct 23, 2019

Piinks commented Oct 23, 2019

justinmc commented Oct 23, 2019

Piinks commented Oct 28, 2019

HansMuller left a comment

Piinks commented Oct 29, 2019 •

edited

Loading

justinmc commented Oct 29, 2019

Piinks commented Oct 31, 2019

Increase TextField's minimum height from 40 to 48 #42449

Increase TextField's minimum height from 40 to 48 #42449

Conversation

justinmc commented Oct 10, 2019 • edited Loading

Description

Related Issues

Tests

Breaking Change

fluttergithubbot commented Oct 10, 2019

justinmc commented Oct 15, 2019 • edited Loading

justinmc Oct 17, 2019

Choose a reason for hiding this comment

justinmc commented Oct 17, 2019

HansMuller left a comment

Choose a reason for hiding this comment

jonahwilliams commented Oct 17, 2019

justinmc commented Oct 17, 2019

justinmc commented Oct 18, 2019 • edited Loading

justinmc commented Oct 23, 2019

Piinks commented Oct 23, 2019

justinmc commented Oct 23, 2019

Piinks commented Oct 28, 2019

HansMuller left a comment

Choose a reason for hiding this comment

Piinks commented Oct 29, 2019 • edited Loading

justinmc commented Oct 29, 2019

Piinks commented Oct 31, 2019

justinmc commented Oct 10, 2019 •

edited

Loading

justinmc commented Oct 15, 2019 •

edited

Loading

justinmc commented Oct 18, 2019 •

edited

Loading

Piinks commented Oct 29, 2019 •

edited

Loading