-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix TextField helper top padding on M3 #145087
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
Fix TextField helper top padding on M3 #145087
Conversation
justinmc
left a comment
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.
LGTM 👍
I would have thought this might break more tests (internal or Google/customer), but it looks like it's good. I guess fewer people are using M3.
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.
This is probably a good idea. I've seen issues where people wanted to be able to control the various fine spacing in InputDecorator.
Let's see if someone ask for a revert in the coming days 🤞 |
f2e2fe9 to
430a4ff
Compare
|
A reason for requesting a revert of flutter/flutter/145087 could |
|
Reason for revert: failing on accessibility checks b/329548765 |
This reverts commit 33fbb75.
|
@justinmc In either case, this should be fixed, if you have a screenshot or access to this TextField configuration, it would be interesting to know its configuration (maybe isDense is set to true?). I have not yet migrated tests related to |
…145172) Reverts: #145087 Initiated by: Jasguerrero Reason for reverting: failing on accessibility checks b/329548765 Original PR Author: bleroux Reviewed By: {justinmc} This change reverts the following previous change: ## Description `InputDecorator` adds a 8.0 gap beetween the container and the helper text. From the Material 3 specification, this gap should be 4.0. See https://m3.material.io/components/text-fields/specs#0c5c8d6d-2169-4d42-960c-51f6ee42eb57. This PR sets the correct values for M3 without changing the value for M2. | Before | After | M3 Spec | |--------|--------|--------| |  | |  | If this change is accepted, a future step will be to make this value configurable, probably by `InputDecorationTheme`. ## Related Issue Fixes #144984 ## Tests
|
@justinmc flutter/packages/flutter/lib/src/material/input_decorator.dart Lines 1102 to 1104 in 985576d
|
|
@bleroux Looking into this more, it seems like it's not the minimum size that it's breaking, it's the MinimumTextContrastGuideline! Very strange. A field with red error text underneath is now breaking this guideline. The only hunch I have is this line:
It inflates the bounds by 4 pixels. Maybe that's now enough to overlap with the bottom of the field, so it's "seeing" some additional color that it wasn't before? The only difference that I see before and after your change is that paintBoundsWithOffset is shifted up by 4 pixels (which is exactly the difference that I'd expect from this PR). |
|
@justinmc Very interesting findings. |
|
It's throwing here:
I'll try to get some help on the accessibility test, and otherwise I'll try to write a test that reproduces it and post back. |
|
Sorry for the silence here. I haven't been able to track down any help on the accessibility guideline stuff. I did try to create a test that reproduces the failure, but I wasn't able to get it to fail on this branch. Something similar to the below with a few tweaks must do it... My attempt at a repro test testWidgets('text contrast bug', (WidgetTester tester) async {
TextFormField getFormField({
required String labelText,
}) {
return TextFormField(
keyboardType: TextInputType.number,
validator: (String? value) {
return 'Please enter $labelText';
},
decoration: InputDecoration(
labelText: labelText,
filled: true,
errorStyle: const TextStyle(
fontSize: 16.0,
),
),
);
}
final SemanticsHandle handle = tester.ensureSemantics();
final GlobalKey formKey = GlobalKey();
await tester.pumpWidget(MaterialApp(
home: Scaffold(
body: Center(
child: Form(
key: formKey,
child: Column(
children: <Widget>[
getFormField(
labelText: 'Deeeeeeee ID',
),
const SizedBox(
height: 16,
),
getFormField(
labelText: 'Buuuuuuuu ID',
),
const SizedBox(
height: 40,
),
Container(
width: double.infinity,
padding: const EdgeInsetsDirectional.symmetric(horizontal: 8.0),
child: FilledButton(
onPressed: () {
final FormState formState = formKey.currentState! as FormState;
if (!formState.validate()) {
return;
}
},
child: const Text('Immmmmmmmmm'),
),
),
],
),
),
),
),
));
await tester.pumpAndSettle();
await tester.tap(find.byType(FilledButton));
await tester.pumpAndSettle();
await expectLater(tester, meetsGuideline(const MinimumTextContrastGuideline()));
handle.dispose();
}); |
|
@justinmc Thanks for providing this example. 🙏 In the meantime, while playing with this test, I can make it fails by using obviously wrong colors (in this case it fails even without using this branch). Looking at the error message I see that it gives a clue to which text is involved and which colors, it might help us to repro if we can see the error message from the failing customer test. Here is an example of the error message for a test that fails on the text contrast guideline : |
|
Updated my previous comment with improvements to the test code, but it still doesn't fail. I thought it might be checking the contrast during the time that the text color is animating to the error state, but that didn't seem to be it with a quick attempt. I'll keep investigating. |
|
@justinmc |
This is a reland of #145087 which was reverted in #145168. #146637 was merged and might help here. ## Description `InputDecorator` adds a 8.0 gap beetween the container and the helper text. From the Material 3 specification, this gap should be 4.0. See https://m3.material.io/components/text-fields/specs#0c5c8d6d-2169-4d42-960c-51f6ee42eb57. This PR sets the correct values for M3 without changing the value for M2. | Before | After | M3 Spec | |--------|--------|--------| |  | |  | If this change is accepted, a future step will be to make this value configurable, probably by `InputDecorationTheme`. ## Related Issue Fixes #144984 ## Tests Updates a value used by several existing tests.
Description
InputDecoratoradds a 8.0 gap beetween the container and the helper text.From the Material 3 specification, this gap should be 4.0.
See https://m3.material.io/components/text-fields/specs#0c5c8d6d-2169-4d42-960c-51f6ee42eb57.
This PR sets the correct values for M3 without changing the value for M2.
If this change is accepted, a future step will be to make this value configurable, probably by
InputDecorationTheme.Related Issue
Fixes #144984
Tests
Updates a value used by several existing tests.