Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Mar 13, 2024

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
image image image

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Mar 13, 2024
@bleroux bleroux requested a review from justinmc March 13, 2024 14:56
Copy link
Contributor

@justinmc justinmc left a 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.

Comment on lines +729 to +730
Copy link
Contributor

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.

@bleroux
Copy link
Contributor Author

bleroux commented Mar 13, 2024

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.

Let's see if someone ask for a revert in the coming days 🤞

@bleroux bleroux force-pushed the fix_textfield_helper_top_padding_on_M3 branch from f2e2fe9 to 430a4ff Compare March 13, 2024 19:23
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 13, 2024
@auto-submit auto-submit bot merged commit 33fbb75 into flutter:master Mar 13, 2024
@bleroux bleroux deleted the fix_textfield_helper_top_padding_on_M3 branch March 13, 2024 20:29
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
@Jasguerrero Jasguerrero added the revert Autorevert PR (with "Reason for revert:" comment) label Mar 14, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 14, 2024

A reason for requesting a revert of flutter/flutter/145087 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Mar 14, 2024
@Jasguerrero
Copy link
Contributor

Reason for revert: failing on accessibility checks b/329548765

@Jasguerrero Jasguerrero added the revert Autorevert PR (with "Reason for revert:" comment) label Mar 14, 2024
auto-submit bot pushed a commit that referenced this pull request Mar 14, 2024
@bleroux
Copy link
Contributor Author

bleroux commented Mar 14, 2024

@justinmc
I would says that it is weird that reducing the gap below the container by 4.0 (as done by this PR) changes wether the TextField meet the accessibility guideline or not (the interactive zone is the same so it was probably wrong before too).

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 isDense (one difficulty is that it is no more a thing in M3, so we will have to decide how it impacts rendering). I can focus on such test before relanding this change.

auto-submit bot pushed a commit that referenced this pull request Mar 14, 2024
…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 |
|--------|--------|--------|
| ![image](https://github.com/flutter/flutter/assets/840911/9947f334-d98f-4f7e-9da7-ca6d5c0770ac) | ![image](https://github.com/flutter/flutter/assets/840911/081dec4b-eb9f-4eee-a7dc-2538e7110ff0)| ![image](https://github.com/flutter/flutter/assets/840911/c8c8f045-3b79-43a5-a1a3-cc6d5460644f) |

If this change is accepted, a future step will be to make this value configurable, probably by `InputDecorationTheme`.

## Related Issue

Fixes #144984

## Tests
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
@bleroux
Copy link
Contributor Author

bleroux commented Mar 15, 2024

@justinmc
I checked the InputDecorator implementation.
It is intended that the min size is lower than kMinInteractiveDimension in some cases:

final double minContainerHeight = decoration.isDense! || decoration.isCollapsed || expands
? 0.0
: kMinInteractiveDimension;

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
@justinmc
Copy link
Contributor

@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:

paintBoundsWithOffset = MatrixUtils.transformRect(globalTransform, renderBox.paintBounds.inflate(4.0));

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).

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024
@bleroux
Copy link
Contributor Author

bleroux commented Mar 18, 2024

@justinmc Very interesting findings.
I have not yet figured out how to reproduce this failure. I would be interested in having more information (error message, screenshot, etc).

@justinmc
Copy link
Contributor

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.

@bleroux bleroux mentioned this pull request Mar 27, 2024
5 tasks
@justinmc
Copy link
Contributor

justinmc commented Apr 4, 2024

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();
  });

@bleroux
Copy link
Contributor Author

bleroux commented Apr 4, 2024

@justinmc Thanks for providing this example. 🙏
I see that the background color is set for helperStyle, I will try various combination, but I'm not sure it is related because in #145087 (comment) you mentionned that it was related to the error message (I will tweak errorStyle too).

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 :

   Which: SemanticsNode#5(Rect.fromLTRB(12.0, 64.0, 86.4, 80.0), label: "helper", textDirection:
ltr):
          Expected contrast ratio of at least 4.5 but found 1.94 for a font size of 12.0.
          The computed colors was:
          light - Color(0xfffef7ff), dark - Color(0xffffa000)
          See also: https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-contrast.html

          SemanticsNode#7(Rect.fromLTRB(12.0, 64.0, 86.4, 80.0), label: "helper", textDirection:

@justinmc
Copy link
Contributor

justinmc commented Apr 4, 2024

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.

@bleroux
Copy link
Contributor Author

bleroux commented Apr 9, 2024

@justinmc
#146507 might be related to the failure.
As you rightly pointed:
"the accessibility test 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?"
So yes, for a filled text field, a 1 pixel red line will be inside the area and wasn't before my PR.
I will file a PR for #146507 tomorrow, it might break internal tests if they rely on golden. But if we can land it, I will be eager to see if it helps here.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
auto-submit bot pushed a commit that referenced this pull request May 16, 2024
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 |
|--------|--------|--------|
| ![image](https://github.com/flutter/flutter/assets/840911/9947f334-d98f-4f7e-9da7-ca6d5c0770ac) | ![image](https://github.com/flutter/flutter/assets/840911/081dec4b-eb9f-4eee-a7dc-2538e7110ff0)| ![image](https://github.com/flutter/flutter/assets/840911/c8c8f045-3b79-43a5-a1a3-cc6d5460644f) |

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextField helper text top padding should be 4.0 on Material3

3 participants