Skip to content
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

Always relies on floatingLabelStyle when FloatingLabelBehavior.always #147374

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Apr 25, 2024

Description

With this PR, when InputDecorator.floatingLabelBehavior is set to FloatingLabelBehavior.always the label style is always set to InputDecorator.floatingLabelStyle, previously InputDecorator.labelStyle was used when the field was not focused or was empty.

Related Issue

Fixes #147231

Tests

Adds 1 test for this particular issue and several missing 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 Apr 25, 2024
@@ -2155,7 +2155,7 @@ class _InputDecoratorState extends State<InputDecorator> with TickerProviderStat
child: AnimatedDefaultTextStyle(
duration:_kTransitionDuration,
curve: _kTransitionCurve,
style: widget._labelShouldWithdraw
style: widget._labelShouldWithdraw || decoration.floatingLabelBehavior == FloatingLabelBehavior.always
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if _labelShouldWithdraw should just take into account decoration.floatingLabelBehavior == FloatingLabelBehavior.always. I looked through the usages in this file and it is almost always used along with decoration.floatingLabelBehavior == FloatingLabelBehavior.always so it seems reasonable. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my first thinking.
Then I saw the _labelShouldWithdraw usage related to Semantics and prefix/suffix and I was not sure of the consequences.
See

final bool needsSemanticsSortOrder = widget._labelShouldWithdraw && (input != null ? (hasPrefix || hasSuffix) : (hasPrefix && hasSuffix));
final Widget? prefix = hasPrefix
? _AffixText(
labelIsFloating: widget._labelShouldWithdraw,
text: decoration.prefixText,
style: MaterialStateProperty.resolveAs(decoration.prefixStyle, materialState) ?? hintStyle,
semanticsSortKey: needsSemanticsSortOrder ? _kPrefixSemanticsSortOrder : null,
semanticsTag: _kPrefixSemanticsTag,
child: decoration.prefix,
)
: null;
final Widget? suffix = hasSuffix
? _AffixText(
labelIsFloating: widget._labelShouldWithdraw,
text: decoration.suffixText,
style: MaterialStateProperty.resolveAs(decoration.suffixStyle, materialState) ?? hintStyle,
semanticsSortKey: needsSemanticsSortOrder ? _kSuffixSemanticsSortOrder : null,
semanticsTag: _kSuffixSemanticsTag,
child: decoration.suffix,
)
: null;
if (input != null && needsSemanticsSortOrder) {

But reconsidering this, moving the check to _labelShouldWithdraw makes sense and improve readibility because it simplifies several clauses.
I will push an update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it makes sense to change this even in the semantics case, since its depending on whether the label is floating. I'll keep an eye out if this causes any issues.

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @bleroux, and for all the tests!

@bleroux bleroux force-pushed the label_style_for_floatingLabelBehavior.always branch from a0ae9c4 to 11d9fad Compare April 26, 2024 09:11
@@ -1789,8 +1789,11 @@ class InputDecorator extends StatefulWidget {
/// Whether the label needs to get out of the way of the input, either by
/// floating or disappearing.
///
/// Will withdraw when not empty, or when focused while enabled.
bool get _labelShouldWithdraw => !isEmpty || (isFocused && decoration.enabled);
/// Will withdraw when not empty, or when focused while enabled, or when
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think you can get rid of the first or in this sentence.

@bleroux bleroux force-pushed the label_style_for_floatingLabelBehavior.always branch from 11d9fad to 6cd69f2 Compare April 26, 2024 18:41
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ small nit. Thanks for the contribution!

@Renzo-Olivares
Copy link
Contributor

I'll check on the google testing issues @bleroux.

@bleroux bleroux force-pushed the label_style_for_floatingLabelBehavior.always branch from 6cd69f2 to 9533a27 Compare April 29, 2024 09:02
@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented Apr 30, 2024

@bleroux There is one test failure that i'm not too sure is expected or not. Before this change the prefix/prefixText would not show when the floatingLabelBehavior is set to FloatingLabelBehavior.always, and we unfocus the TextField. After this change the prefix/prefixText always shows when floatingLabelBehavior is set to FloatingLabelBehavior.always. Something similar also happens with suffix/suffixText. The main culprit is this block of code that builds the prefix and suffix

final Widget? prefix = hasPrefix
? _AffixText(
labelIsFloating: widget._labelShouldWithdraw,
text: decoration.prefixText,
style: MaterialStateProperty.resolveAs(decoration.prefixStyle, materialState) ?? hintStyle,
semanticsSortKey: needsSemanticsSortOrder ? _kPrefixSemanticsSortOrder : null,
semanticsTag: _kPrefixSemanticsTag,
child: decoration.prefix,
)
: null;
final Widget? suffix = hasSuffix
? _AffixText(
labelIsFloating: widget._labelShouldWithdraw,
text: decoration.suffixText,
style: MaterialStateProperty.resolveAs(decoration.suffixStyle, materialState) ?? hintStyle,
semanticsSortKey: needsSemanticsSortOrder ? _kSuffixSemanticsSortOrder : null,
semanticsTag: _kSuffixSemanticsTag,
child: decoration.suffix,
)
: null;
. _AffixText depends on whether the label is floating to decide whether or not to visually show the prefix/suffix. Should the prefix/suffix always be displayed in this case? Let me know if this explanation was clear enough or if I can provide more context. I'm leaning towards the new behavior being the correct one, since I don't see a reason not to show the prefix if the label is floating.

@bleroux
Copy link
Contributor Author

bleroux commented Apr 30, 2024

@Renzo-Olivares Many thanks for the detailed information.

I'm leaning towards the new behavior being the correct one, since I don't see a reason not to show the prefix if the label is floating.

I agree with you. I'm currently working on the M3 tests migration for prefix/suffix, I started looking closer at this behavior and I will post an update tomorrow to let you if I found anything that we missed.

@bleroux bleroux force-pushed the label_style_for_floatingLabelBehavior.always branch from 9533a27 to 7be6c58 Compare May 1, 2024 19:48
@bleroux bleroux force-pushed the label_style_for_floatingLabelBehavior.always branch from 7be6c58 to bed083b Compare May 1, 2024 19:49
@bleroux
Copy link
Contributor Author

bleroux commented May 1, 2024

@Renzo-Olivares
I had a closer look and I still fully agree with you on "since I don't see a reason not to show the prefix if the label is floating."

I have updated the PR with two tests related to that behavior (and I will probably add more on the PR I'm working on which goals is to migrate M3 tests related to prefix/suffix).

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label May 3, 2024
@auto-submit auto-submit bot merged commit 99876ba into flutter:master May 3, 2024
72 checks passed
@bleroux bleroux deleted the label_style_for_floatingLabelBehavior.always branch May 3, 2024 15:16
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 6, 2024
flutter/flutter@f1037a0...04424e1

2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from f2bfea5fdecd to a30ae7729c95 (1 revision) (flutter/flutter#147865)
2024-05-06 karel.klic@managry.com Fix Tooltip.decoration comment (flutter/flutter#147858)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2b828f368f6 to f2bfea5fdecd (1 revision) (flutter/flutter#147854)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from c73fd390d10e to e2b828f368f6 (1 revision) (flutter/flutter#147853)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 624dcb987f39 to c73fd390d10e (1 revision) (flutter/flutter#147852)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 900322f23375 to 624dcb987f39 (1 revision) (flutter/flutter#147845)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9080957cd6a0 to 900322f23375 (1 revision) (flutter/flutter#147842)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 66d08d55d788 to 9080957cd6a0 (1 revision) (flutter/flutter#147841)
2024-05-05 polinach@google.com Fix test. (flutter/flutter#147813)
2024-05-05 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `CupertinoSwitch` (flutter/flutter#147821)
2024-05-05 32538273+ValentinVignal@users.noreply.github.com Reland fix memory leaks for tab selector (flutter/flutter#147689)
2024-05-05 sokolovskyi.konstantin@gmail.com Fix memory leak in ExpansionTile. (flutter/flutter#147596)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 135acd5a689a to 66d08d55d788 (2 revisions) (flutter/flutter#147834)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from c937a02c6eb0 to 135acd5a689a (2 revisions) (flutter/flutter#147818)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1d484e57ce2c to c937a02c6eb0 (2 revisions) (flutter/flutter#147812)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2561636602ed to 1d484e57ce2c (1 revision) (flutter/flutter#147808)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 484574426120 to 2561636602ed (1 revision) (flutter/flutter#147805)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from d701f407c8ea to 484574426120 (1 revision) (flutter/flutter#147802)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 250536888a91 to d701f407c8ea (2 revisions) (flutter/flutter#147800)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3eadfd5284c0 to 250536888a91 (1 revision) (flutter/flutter#147796)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 837914f3788b to 3eadfd5284c0 (1 revision) (flutter/flutter#147791)
2024-05-03 102401667+Dimilkalathiya@users.noreply.github.com fixes `SearchAnchor` leak (flutter/flutter#147652)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8cce00433073 to 837914f3788b (1 revision) (flutter/flutter#147780)
2024-05-03 engine-flutter-autoroll@skia.org Roll Packages from aea93d2 to f4719ca (5 revisions) (flutter/flutter#147782)
2024-05-03 leroux_bruno@yahoo.fr Always relies on floatingLabelStyle when FloatingLabelBehavior.always (flutter/flutter#147374)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
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 labelText doesn't use floatingLabelStyle when unfocused
2 participants