Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Apr 9, 2024

Description

This PRs makes the label vertical position depend on visual density for filled text field.
Previously, for M2 and M3, the label vertical offset was always the same (12 on M2, 8 and M3) despite different visual density configuration.
This was noticable on desktop and can lead to weird rendering especially on M3 where line height makes the cursor taller.

Screenshots for a filled text field:

Before After
M3 macOs image image
M2 macOs image image

Related Issue

Fixes #141354

Tests

Adds 2 tests, updates 2 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 9, 2024
@bleroux bleroux requested a review from justinmc April 9, 2024 09:26
@bleroux
Copy link
Contributor Author

bleroux commented Apr 9, 2024

@justinmc I found this while migrating some visual density tests.
The issue existed in M2. If it breaks too much internal tests, I can make it M3 only but I think it is worth fixing it for M2 and M3 as the result is way better.
For M3, it makes the cursor height issue less critical: the label is positioned 4 pixels higher than before so there is enough vertical spacing beetwen the label and the cursor.

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 if the google tests pass. I just started them so let's see. Thanks for opening a fix for this!

@bleroux
Copy link
Contributor Author

bleroux commented Apr 9, 2024

Looks like Google tests are ok 🎉

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2024
@auto-submit auto-submit bot merged commit 906ce36 into flutter:master Apr 9, 2024
@bleroux bleroux deleted the fix_input_decorator_label_position_ignore_visual_density branch April 9, 2024 21:56
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Apr 10, 2024
flutter/flutter@4967a94...97cd47a

2024-04-10 [email protected] Roll Flutter Engine from eaf73cd39cf8 to cee489a4e275 (2 revisions) (flutter/flutter#146541)
2024-04-10 [email protected] Roll Flutter Engine from e4e274898efc to eaf73cd39cf8 (6 revisions) (flutter/flutter#146538)
2024-04-09 [email protected] Correct doc for AnimationMin (flutter/flutter#146531)
2024-04-09 [email protected] Roll Flutter Engine from 5dca50d3e268 to e4e274898efc (2 revisions) (flutter/flutter#146527)
2024-04-09 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.2.0 to 4.3.0 (flutter/flutter#146528)
2024-04-09 [email protected] Fix InputDecorator label position ignore visual density (flutter/flutter#146488)
2024-04-09 [email protected] Roll Flutter Engine from 5a824e22deb2 to 5dca50d3e268 (10 revisions) (flutter/flutter#146524)
2024-04-09 [email protected] Support mdns when attaching to proxied devices. (flutter/flutter#146021)
2024-04-09 [email protected] Fix skwasm tests (flutter/flutter#145570)
2024-04-09 [email protected] Roll pub packages (flutter/flutter#146515)
2024-04-09 [email protected] Roll Packages from 6b4d8b6 to 17f55d3 (6 revisions) (flutter/flutter#146508)

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 [email protected],[email protected] 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
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
## Description

This PRs makes the label vertical position depend on visual density for filled text field.
Previously, for M2 and M3, the label vertical offset was always the same (12 on M2, 8 and M3) despite different visual density configuration.
This was noticable on desktop and can lead to weird rendering especially on M3 where line height makes the cursor taller.

Screenshots for a filled text field:

| | Before | After |
|--------|--------|--------|
|M3 macOs| ![image](https://github.com/flutter/flutter/assets/840911/bd9bdb6e-477c-4db0-ae8f-74e18d19f29e) | ![image](https://github.com/flutter/flutter/assets/840911/25e59c44-0139-4813-be28-472302d6970e) | 
|M2 macOs| ![image](https://github.com/flutter/flutter/assets/840911/1c52493b-b17b-407b-93cc-69120207b716) | ![image](https://github.com/flutter/flutter/assets/840911/1fc4a8b6-429b-476c-b5bf-ff2934bf5293) | 

</details> 

## Related Issue

Fixes flutter#141354

## Tests

Adds 2 tests, updates 2 tests.
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
flutter/flutter@4967a94...97cd47a

2024-04-10 [email protected] Roll Flutter Engine from eaf73cd39cf8 to cee489a4e275 (2 revisions) (flutter/flutter#146541)
2024-04-10 [email protected] Roll Flutter Engine from e4e274898efc to eaf73cd39cf8 (6 revisions) (flutter/flutter#146538)
2024-04-09 [email protected] Correct doc for AnimationMin (flutter/flutter#146531)
2024-04-09 [email protected] Roll Flutter Engine from 5dca50d3e268 to e4e274898efc (2 revisions) (flutter/flutter#146527)
2024-04-09 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.2.0 to 4.3.0 (flutter/flutter#146528)
2024-04-09 [email protected] Fix InputDecorator label position ignore visual density (flutter/flutter#146488)
2024-04-09 [email protected] Roll Flutter Engine from 5a824e22deb2 to 5dca50d3e268 (10 revisions) (flutter/flutter#146524)
2024-04-09 [email protected] Support mdns when attaching to proxied devices. (flutter/flutter#146021)
2024-04-09 [email protected] Fix skwasm tests (flutter/flutter#145570)
2024-04-09 [email protected] Roll pub packages (flutter/flutter#146515)
2024-04-09 [email protected] Roll Packages from 6b4d8b6 to 17f55d3 (6 revisions) (flutter/flutter#146508)

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 [email protected],[email protected] 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.

Default TextField styles result in cursor overlapping labelText

2 participants