Rename "subtext" to "supportingTextPadding" in FormField's documentation - #183582

Shouldn't this be affected by errorPadding? Also the rtl case below, where the end padding should have been more critical.

I have just noticed i misunderstood the end padding in rtl case, what previously happened was that the counter , error and helper padding was affected by the content padding , as far as i know endPadding will take effect on the counter which in the current approach is handled with the old contentPadding so is that the case you mentioned and should the counter padding should be handled with end padding just as the contentPadding did before?
or helper and error padding should be separated from content Padding for now and later on maybe we can consider separating counter Padding as well to add more flexibility

TL;DR
do we add endError to control the counter padding along with the errorPadding or should it be handled with content Padding?

Yes, I think it makes the most sense for this new padding to control the counter's alignment as well. If we only decouple the error text from contentPadding but leave the counter tied to it, they might become visually misaligned. Let's use the end value of the new padding for the counter.

I agree with you on this part , in the last commit i separated counter padding from the contentPadding

Since this padding is also used for helper, I think we should also mention helper somewhere. I prefer changing the name to helperErrorPadding or errorHelperPadding, but if you prefer errorPadding, we can at least mention this in the property document.

Also, should we also consider vertical padding here?

you are right about the naming my main focus was on the error itself since the first issue #181755 was introduced , also i don't know if we should consider vertical padding or not, i guess then we will have to deal with the size itself of the size of the widget below in that method

927|  _SubtextSize? _computeSubtextSizes({
928| required BoxConstraints constraints,
929| required ChildLayouter layoutChild,
930|   required _ChildBaselineGetter getBaseline,
931|  }) {

and as mentioned before this will take effect on the bottom row which have error, counter and helper.
if we considered control over vertical padding in that case maybe we can use error vertical padding instead of the default subtextGap value if vertical padding like that

final double topPadding = errorPadding?.top ?? subtextGap;
final double bottomPadding = errorPadding?.bottom ?? 0.0;

that will take effect on the whole row and using the old subtextGap will not break any old application but is adding such flexibility for the developer is good considering UI guidelines also do we need to consider more flexibility and separate padding values of helper , text and counter padding .
my opinion if we going to consider vertical padding maybe
A- we can start padding with the whole row so adding vertical padding will affect counter, error and helper
B- we can separate vertical padding for each one of them in that case we will consider helper Padding , counter padding and error padding values
C- we can stick with the current hardcoded subtextGap value that controls the vertical padding of the bottom row
from my perspective "C" is the current approach, we can consider "A" as our next plan and maybe we can consider "B" as optional future plan.
let me know what do you think .

I think we should directly go with Option A. It provides a much cleaner and more predictable API if this single property controls the EdgeInsets (both horizontal and vertical) for the entire subtext row (error, helper, and counter), which is also more intuitive for developers when they see this property.

We can still ensure backward compatibility with option A: When the new padding is null, we can fall back to the legacy behavior.

In fact, if we go with option C now, migrating to option A will be a breaking change, since vertical padding has been ignored but will be effective in the future.

Also, in terms of name, since it now includes counter as well, we might call it supportingTextPadding. Although Flutter has not started using the name "supporting text" yet, it is an official name in the M3 spec (https://m3.material.io/components/text-fields/specs ).

following the M3 spec everything looks clear now regarding the naming and how i handled the padding in the new commit,
counter , helper and error text will be aligned using supportingTextPadding .
let me know if there's anything is missing

Example with new API:

 supportingTextPadding: EdgeInsets.only(left: 10,right: 16,top: 10,bottom: 5),
 contentPadding: EdgeInsets.only(left: 12,right: 12,top: 12,bottom: 12),

Extra line?

I have updated computeMinIntrinsicHeight to include the supportingTextPadding vertical and fallback to default value subtextGap in case of supportingTextPadding is not provided

It should also clearly state that when non-null, it completely overrides the default behavior (including subtextGap).

Should this be

I misunderstood bottomHeight as the space including supporting text and the space below it , not as the "bottom" refers to the whole bottom widgets below the main container .

I have fixed it in e720eaae720eaa as well as giving the docs more context.

I guess we covered all the points now, let me know if there's any more concerns , do we need to provide specific test for computeMinIntrinsicHeight ?

@dkwingsmt since now there's code freeze what is the suggested next step ?

Phrasing like "fallback to the legacy code" isn't ideal for future readers of the codebase because it doesn't describe the actual behavior. It's better to explicitly state what happens when the value is null. Something like:

    // The vertical padding around the row containing the helper, error, and counter widgets.
    // Defaults to `subtextGap` at the top and 0.0 at the bottom when `supportingTextPadding` is null.

This macro isn't defined. Did you write it by mistake?

I wrote macro by mistake and removed it on latest commit, also reformatted both files on current PR Flutter version

I think I found a bug here, contentConstraints are used for subtextSize but never consider supportingTextPadding override. This potentially causes overlaps where the supporting text is laid out with the size of the contentPadding instead of that of the supportingTextPadding.

Center(
  child: Container(
    width: 300.0,
    decoration: BoxDecoration(
      border: Border.all(color: Colors.red, width: 2.0),
    ),
    child: const InputDecorator(
      decoration: InputDecoration(
        filled: true,
        labelText: 'Enter Text',
        supportingTextPadding: EdgeInsets.symmetric(
          horizontal: 80.0,
        ),
        helperText:
            'This is helper text that is long enough to wrap if constrained correctly but overflows instead.',
        helperMaxLines: 3,
      ),
    ),
  ),
),

Hi @Renzo-Olivares , Thank you for addressing these concerns.
contentConstraints was passed to _computeSubtextSizes
I'd suggest going with separate constraints for supportingText/subText rather relying on changing the original contentConstraints because prefix and suffix depends on contentConstraints as well.
so my suggestion includes adding

final BoxConstraints subTextConstraints = containerConstraints.deflate(
      EdgeInsetsDirectional.only(
        start: (supportingTextPadding?.start??contentPadding.start) + decoration.inputGap ,
        end: (supportingTextPadding?.end??contentPadding.end) + decoration.inputGap,
      ),
    );

with a safe fallback to contentPadding and use it for _computeSubtextSizes like this

final _SubtextSize? subtextSize = _computeSubtextSizes(
      constraints: subTextConstraints,
      layoutChild: layoutChild,
      getBaseline: getBaseline,
    );    

But a small concern about the naming :
do we go with subTextConstraints to match the legacy naming or supportingTextConstraints to match M3 spec?

I think we should probably change all references from subText to supportingText to match the material spec and so we do not have two names for the same concept which may be confusing to readers.

I suggest the following:

In one commit implement as subTextConstraints, and in the next commit change all references of subText to supportingText. That will make it easier to review since there are a few references of subText.

This should probably take into account supportingTextPadding for counterHeight and helperErrorAvailableWidth.

I'd suggest introducing supportingTextWidth right before calculating prefix/suffix icon sizes, since the subtext row (error, helper, and counter) is not inline with the prefix/suffix area., just to avoid the miscalculation conflicts and it would be like this

    width = math.max(width - iconWidth, 0.0);
    final double supportingTextWidth = math.max(width - (supportingTextPadding?.horizontal ?? contentPadding.horizontal) - decoration.inputGap * 2, 0.0);

    final double prefixIconHeight = _minHeight(prefixIcon, width);
    final double prefixIconWidth = _minWidth(prefixIcon, prefixIconHeight);
    final double suffixIconHeight = _minHeight(suffixIcon, width);
    final double suffixIconWidth = _minWidth(suffixIcon, suffixIconHeight);

    width = math.max(width - contentPadding.horizontal - decoration.inputGap * 2, 0.0);
    // supportingTextWidth  used instead of width
    final double counterHeight = _minHeight(counter, supportingTextWidth);
    final double counterWidth = _minWidth(counter, counterHeight);

    final double counterPadding = counter != null ? _kSubtextCounterPadding : 0.0;
    // supportingTextWidth  used instead of width 
    final double helperErrorAvailableWidth = math.max(supportingTextWidth - counterWidth - counterPadding, 0.0);

Let me know what do you think.

Sounds good to me!

nit: consider making this a template and re-use it in this file.

Also I wonder if we should exclude subtextGap since I don't think its part of the public API, so mentioning it is potentially leaking implementation detail. Just mentioning vertical gap should be fine.

The InputDecorationTheme class should also add support for the new supportingTextPadding property. While InputDecorationThemeData has been correctly updated to include supportingTextPadding, the InputDecorationTheme class (which is an InheritedTheme wrapping InputDecorationThemeData) has not.

Not from this PR, but this check is redundant as other.disabledBorder == disabledBorder is already checked on line 5685.

@Renzo-Olivares I have added the missing property supportingTextPadding to InputDecorationTheme class and created separate PR for the redundant check.