Adding supportingTextPadding to InputDecorator and InputDecorationTheme#183582
Adding supportingTextPadding to InputDecorator and InputDecorationTheme#183582MohanadAbdallah-mv wants to merge 18 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an errorPadding property to InputDecorator and InputDecorationThemeData, allowing for separate padding for error text. The implementation looks correct and includes relevant tests. I've made a couple of suggestions to improve the documentation and maintain code consistency.
|
Hi @dkwingsmt |
dkwingsmt
left a comment
There was a problem hiding this comment.
I'm ok with the API design. Minor problems with the implementation. If you agree with the comments, new tests should be added accordingly.
| case TextDirection.ltr: | ||
| start = contentPadding.start + _boxSize(icon).width; | ||
| startError = (errorPadding?.start ?? contentPadding.start) + _boxSize(icon).width; | ||
| end = overallWidth - contentPadding.end; |
There was a problem hiding this comment.
Shouldn't this be affected by errorPadding? Also the rtl case below, where the end padding should have been more critical.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree with you on this part , in the last commit i separated counter padding from the contentPadding
| _boxParentData(helperError).offset = Offset( | ||
| start + decoration.inputGap, | ||
| startError + decoration.inputGap, | ||
| subtextBaseline - helperErrorBaseline, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, should we also consider vertical padding here?
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ).
There was a problem hiding this comment.
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),
- adding vertical padding - adding required tests
There was a problem hiding this comment.
Sorry for late review. I did another round of full review and these are the only thing I can find. Generally minor comments.
Also, there are still usages of subtextGap in this file (namely in computeMinIntrinsicHeight). The subtextGap field should no longer be used except as default values; all previous usages should be converted to something related to supportingTextPadding.
| subtextBaseline - helperErrorBaseline, | ||
| ); | ||
| if (counter != null) { | ||
| _boxParentData(counter).offset = Offset( | ||
| end - counter.size.width - decoration.inputGap, | ||
| endSupporting - counter.size.width - decoration.inputGap, | ||
| // endSupporting - counter.size.width - decoration.inputGap, |
There was a problem hiding this comment.
I have updated computeMinIntrinsicHeight to include the supportingTextPadding vertical and fallback to default value subtextGap in case of supportingTextPadding is not provided
| /// if [supportingTextPadding] is null, the value of [contentPadding] will be used for both supporting text widgets [InputDecoration.helper] , [InputDecoration.counter] | ||
| /// and [InputDecoration.error]. |
There was a problem hiding this comment.
| /// if [supportingTextPadding] is null, the value of [contentPadding] will be used for both supporting text widgets [InputDecoration.helper] , [InputDecoration.counter] | |
| /// and [InputDecoration.error]. | |
| /// If [supportingTextPadding] is null, the value of [contentPadding] will be used | |
| /// for both supporting text widgets [InputDecoration.helper], [InputDecoration.counter], | |
| /// and [InputDecoration.error]. |
It should also clearly state that when non-null, it completely overrides the default behavior (including subtextGap).
| final double bottomHeight = math.max(counterAscent, helperErrorHeight) + subtextGap; | ||
| final double subtextHeight = math.max(counterSize.height, helperErrorHeight) + subtextGap; | ||
| math.max(counterAscent, getBaseline(helperError, helperErrorConstraints)) + topPadding; | ||
| final double bottomHeight = math.max(counterAscent, helperErrorHeight) + bottomPadding; |
There was a problem hiding this comment.
Should this be
| final double bottomHeight = math.max(counterAscent, helperErrorHeight) + bottomPadding; | |
| final double bottomHeight = math.max(counterAscent, helperErrorHeight) + topPadding + bottomPadding; |
There was a problem hiding this comment.
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 ?
| // topPadding and bottomPadding control the vertical padding of the row containing [helper, error, counter] widgets | ||
| // in case of null values it will fallback to the legacy code to avoid breaking any established codebase |
There was a problem hiding this comment.
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.- Update doc comments to clarify override behavior of subtextGap - Fix bottomHeight to include topPadding
dkwingsmt
left a comment
There was a problem hiding this comment.
LGTM except for minor checks.
Also the analyzer shows that there are some format errors in both files, which can be solved by dart format ..
| /// within a [Theme]. | ||
| /// * [InputDecoration.visualDensity], which can override this setting for a | ||
| /// given decorator. | ||
| /// {@macro flutter.material.inputDecoration.visualDensity} |
There was a problem hiding this comment.
This macro isn't defined. Did you write it by mistake?
There was a problem hiding this comment.
I wrote macro by mistake and removed it on latest commit, also reformatted both files on current PR Flutter version
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
- removing undefined macro
There was a problem hiding this comment.
Hi @MohanadAbdallah-mv, thank you for your contribution and patience! I left a few small comments but overall this looks like a reasonable addition!
| ); | ||
| final BoxConstraints contentConstraints = containerConstraints.deflate( | ||
| EdgeInsetsDirectional.only( | ||
| start: contentPadding.start + decoration.inputGap, |
There was a problem hiding this comment.
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,
),
),
),
),There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| final double suffixIconHeight = _minHeight(suffixIcon, width); | ||
| final double suffixIconWidth = _minWidth(suffixIcon, suffixIconHeight); | ||
|
|
||
| width = math.max(width - contentPadding.horizontal - decoration.inputGap * 2, 0.0); |
There was a problem hiding this comment.
This should probably take into account supportingTextPadding for counterHeight and helperErrorAvailableWidth.
There was a problem hiding this comment.
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.
| /// given decorator. | ||
| final VisualDensity? visualDensity; | ||
|
|
||
| /// The padding applied to the supporting text row. |
There was a problem hiding this comment.
nit: consider making this a template and re-use it in this file.
There was a problem hiding this comment.
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.
Renzo-Olivares
left a comment
There was a problem hiding this comment.
@MohanadAbdallah-mv, left a few more comments. We should probably also update the PR title/description to better match the current implementation.
…portingTextPadding - Reuse supportingTextPadding documentation in InputDecorationThemeData
|
@Renzo-Olivares Everything should be up to date now , current implementation follow the plan we discussed earlier including the documentation update, PR title/description and replacing |
|
@MohanadAbdallah-mv I did a quick search and the only other reference I could find is |
|
@Renzo-Olivares yes , along with |
…files to supportingText
Renzo-Olivares
left a comment
There was a problem hiding this comment.
@MohanadAbdallah-mv thank you for your quick changes and patience! Left a few more comments that I noticed.
| this.alignLabelWithHint = false, | ||
| this.constraints, | ||
| this.visualDensity, | ||
| this.supportingTextPadding, |
There was a problem hiding this comment.
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.
| @@ -5602,7 +5689,8 @@ class InputDecorationThemeData with Diagnosticable { | |||
| other.alignLabelWithHint == alignLabelWithHint && | |||
| other.constraints == constraints && | |||
| other.disabledBorder == disabledBorder && | |||
There was a problem hiding this comment.
Not from this PR, but this check is redundant as other.disabledBorder == disabledBorder is already checked on line 5685.
There was a problem hiding this comment.
@Renzo-Olivares I have added the missing property supportingTextPadding to InputDecorationTheme class and created separate PR for the redundant check.
This PR introduces the
supportingTextPaddingproperty toInputDecorator,InputDecoration, andInputDecorationThemeData.While originally conceived as
errorPadding, this property was renamed tosupportingTextPaddingduring review to align with the Material Design 3 specification, as it controls the padding for the entire supporting text row (which includes helper text, error text, and counter widgets).Additionally, this PR addresses layout, intrinsic sizing, and code consistency concerns raised during the review process:
Key Features & Refactors:
supportingTextPadding: Allows complete, independent control over the padding of the supporting text row (helper/error/counter).supportingTextConstraintsin_layout()to bound the supporting text row independently ofcontentConstraints, ensuring custom paddings do not break or shift the layout of inlineprefix/suffixwidgets.computeMinIntrinsicHeight()to computesupportingTextWidthseparately from the primary width deflation path, ensuring intrinsic height calculations accurately predict wrapped text heights.subtextreferences (e.g._SubtextSize,_computeSubtextSizes,subtextHeight) tosupportingTextto match the public property name and the M3 spec.List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
Fixes #175834
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.