Fix M3 text field height + initial step for input decorator M3 test migration [prod-leak-fix] #142981
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
framework
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #142981 at sha e26a950a55cb9a254ed6c12963ac749a4b491996 |
bleroux
changed the title
bleroux
changed the title
bleroux
changed the title
bleroux
changed the title
justinmc
approved these changes
Feb 6, 2024
Contributor
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 (but we should see if the Google tests pass)
Good catch with the buildInputDecorator thing. I've started the Google tests in case this breaks any visual diff tests. At least it looks like it's passing the customer tests.
Contributor
There was a problem hiding this comment.
Maybe add a TODO here and point to #139076? Just to make sure that it's clear that we still intend to migrate these tests to M3. Or maybe down where runAllM2Tests is called.
Contributor
There was a problem hiding this comment.
Will these be moved inside of runAllTests after you've migrated them all to M3?
Contributor
Author
There was a problem hiding this comment.
runAllTests was introduced because it had a useMaterial3 parameter.
After this PR, it won't be needed so my plan is to put the migrated tests in main(similarly to most of the other test files) and to keep runAllM2Tests for the M2 tests.
I also wonder if we should consider moving all M2 tests to another file? (for instance input_decorator_m2_test.dart)
Contributor
There was a problem hiding this comment.
Got it, thanks for the explanation. I like the idea of a separate m2 test file.
github-actions
bot
added
a: tests
bleroux
force-pushed
the
M3_migration_for_input_decorator_test_step1
branch
from
5034870 to
747742e
Compare
github-actions
bot
removed
a: tests
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 7, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 7, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 7, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 7, 2024
polina-c
added
the
a: leak tracking
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Feb 7, 2024
flutter/flutter@e6ba809...8431cae 2024-02-07 [email protected] Roll Flutter Engine from 1ac6beb8a3c2 to 6807342305e4 (6 revisions) (flutter/flutter#143082) 2024-02-07 [email protected] Manual roll Flutter Engine from 808886312e2b to 1ac6beb8a3c2 (22 revisions) (flutter/flutter#143039) 2024-02-07 [email protected] Roll Packages from 1a5a7ce to e4ea6bf (4 revisions) (flutter/flutter#143076) 2024-02-07 [email protected] Fix M3 text field height + initial step for input decorator M3 test migration (flutter/flutter#142981) 2024-02-07 [email protected] [reland] Add `AnimationStyle` to `showSnackBar` (flutter/flutter#143052) 2024-02-07 [email protected] Run Mac x64 build tests in postsubmit only (flutter/flutter#142334) 2024-02-07 [email protected] Copy the flutter version JSON file into the simulated Flutter SDK used by update_packages (flutter/flutter#143035) 2024-02-07 [email protected] Mark `Windows_android hot_mode_dev_cycle_win__benchmark` as no longer flaky (flutter/flutter#143016) 2024-02-07 [email protected] Dispose precached image info (flutter/flutter#143017) 2024-02-07 [email protected] Instrument CurvedAnimation. (flutter/flutter#143007) 2024-02-07 [email protected] Update _goldens_io.dart to generate failure images during a size mism� (flutter/flutter#142177) 2024-02-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Activate InkSparkle on CanvasKit" (flutter/flutter#143036) 2024-02-07 [email protected] Activate InkSparkle on CanvasKit (flutter/flutter#138545) 2024-02-07 [email protected] [Windows] Fix signed/unsigned int comparison (flutter/flutter#142341) 2024-02-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Move native assets to `isolated/` directory" (flutter/flutter#143027) 2024-02-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 808886312e2b to 07cdaab7f531 (18 revisions)" (flutter/flutter#143025) 2024-02-06 [email protected] Make destructiveRed a CupertinoDynamicColor (flutter/flutter#141364) 2024-02-06 [email protected] Move native assets to `isolated/` directory (flutter/flutter#142709) 2024-02-06 [email protected] Roll Flutter Engine from 808886312e2b to 07cdaab7f531 (18 revisions) (flutter/flutter#143005) 2024-02-06 [email protected] Fix CupertinoPageScaffold resizeToAvoidBottomInset (flutter/flutter#142776) 2024-02-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add `AnimationStyle` to `showSnackBar`" (flutter/flutter#143001) 2024-02-06 [email protected] Material 3 - Tab indicator stretch animation (flutter/flutter#141954) 2024-02-06 [email protected] Add `AnimationStyle` to `showSnackBar` (flutter/flutter#142825) 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],[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
auto-submit bot
pushed a commit
that referenced
this pull request
Feb 14, 2024
## Description This PR is the second step for the M3 test migration for `InputDecorator` (step 1 was #142981). This PR migrate the two first tests of the M2 section. Those were big tests. I splitted them in several testsn organized in groups, and I narrowed their scope when possible. @justinmc I did not move yet the M2 tests to a separate file (I move them to a group) because it would mean we loss the line history which is useful during the migration. In the next step, I will focus on moving out some tests that are in the 'Material2' group (the ones that are ok with both M2 and M3). ## Related Issue Related to #139076 ## Tests Adds several tests for M3.
This was referenced Feb 15, 2024
auto-submit bot
pushed a commit
that referenced
this pull request
Feb 17, 2024
## Description This PR updates the `InputDecoration.contentPadding` documentation to detail both Material 3 and Material 2 default values. ## Related Issue Follow-up to #142981. ## Tests Documentation only.
auto-submit bot
pushed a commit
that referenced
this pull request
Feb 17, 2024
## Description This PR is the third step for the M3 test migration for `InputDecorator`. Step 1: #142981 Step 2: #143369 This PR moves some tests out of the 'Material2' group (the ones that are ok on M3). @justinmc The diff is almost unreadable, I moved the tests as carefully as possible and I checked that before and after the number of tests is exactly the same. ## Related Issue Related to #139076 ## Tests Move some tests from 'Material2' group to main().
auto-submit bot
pushed a commit
that referenced
this pull request
Mar 13, 2024
…4932) ## Description This PR migrates `InputDecorator` helper/counter/error related tests to M3 and adds some missing tests. It is the fifth step for the M3 test migration for `InputDecorator`. Step 1: #142981 Step 2: #143369 Step 3: #143520 Step 4: #144169 ## Related Issue Related to #139076 fixes #138213
auto-submit bot
pushed a commit
that referenced
this pull request
Mar 18, 2024
## Description This PR migrates `InputDecorator.constraints` related tests to M3 and also various regression tests. It is the fifth step for the M3 test migration for `InputDecorator`. Step 1: #142981 Step 2: #143369 Step 3: #143520 Step 4: #144169 Step 5: #144932 ## Related Issue Related to #139076 @justinmc A somewhat small PR, I wanted to migrate the small group related to 'InputDecoration.constraints' (starting at line 2801), it adds a test checking min interactive height and two others that shows one way to bypass the standard min interactive height.
auto-submit bot
pushed a commit
that referenced
this pull request
Mar 29, 2024
## Description This PRs adds a test group for the input decorator container, this group is organised similarly to the M3 spec, see https://m3.material.io/components/text-fields/specs. It is step 7 for the M3 test migration for `InputDecorator`. Step 1: #142981 Step 2: #143369 Step 3: #143520 Step 4: #144169 Step 5: #144932 Step 6: #145213 ## Related Issue Related to #139076 @justinmc The diff is not very readable. The tests added by this PR are from line 298 to line 975. Other changes are due to moving some existing tests to this new 'container' group.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 14, 2024
polina-c
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR main purpose is to make progress on the M3 test migration for
InputDecorator(see #139076).Before this PR more than 80 of the 156 tests defined in
input_decorator_test.dartfail on M3.Migrating all those tests in one shot is not easy at all because many failures are related to wrong positionning due to M3 typography changes. Another reason is that several M3 specific changes are required in order to get a proper M3 compliant text field, for instance:
TextFieldstyles result in cursor overlappinglabelText#141354Most of the tests were relying on an helper function (
buildInputDecorator) which had auseMaterial3parameter. Unfortunately whenuseMaterial3: truewas passed to this function it forceduseMaterial3: falseat the top level but overrided it at a lower level, which was very misleading because people could assume that the tests are ok with M3 (but in fact they were run using M2 typography but have some M3 logic in use).I considered various way to make this change and I finally decided to run all existing tests only on M2 for the moment. Next step will be to move most of those tests to M3. In this PR, I migrated two of these existing tests for illustration.
Because many of the existing tests are checking input decorator height, I think it would also make sense to fix #142972 first. That's why I choosed to include a fix to #142972 in this PR.
A M3 filled
TextFieldon Android:Related Issue
Fixes #142972
Related to #139076
Tests
Updates many existing tests