Fix: Closing bottom sheet and removing FAB cause assertion failure by utisam · Pull Request #128566 · flutter/flutter

utisam · 2023-06-09T07:26:24Z

Fixes #128562

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

TahaTesser · 2023-06-09T10:12:56Z

Thanks for fixing this! @utisam

We'll need a test for this fix.

utisam · 2023-06-09T13:04:23Z

@TahaTesser Test was added at 02404bc

The assertion failure was caused on the latest master channel.

TahaTesser · 2023-06-14T14:27:06Z

packages/flutter/test/material/scaffold_test.dart

I'm not really sure if this is the correct way to test this.

When I comment on these lines and just let pumpWidget, I get errors.

// setState(() => show = false); // await tester.pump(); // start animation // await tester.pump(const Duration(seconds: 1)); // expect(controller?.closed, completes); // expect(find.byType(FloatingActionButton), findsNothing);

TahaTesser · 2023-06-14T14:29:12Z

packages/flutter/test/material/scaffold_test.dart

Is addPostFrameCallback necessary?
It'd be preferable to add and remove the bottom sheet by pressing buttons.

Yeah, maybe can the FAB.onPressed trigger this change?

Thanks for the comments. I rewrite the test using buttons.

Yeah, maybe can the FAB.onPressed trigger this change?

I also tried to trigger 2 changes at FAB.onPressed at once, but it does not reproduce the assertion error.

While rewriting the test, I found the problem is only that while the hiding FAB animation is running, another event starts the closing bottom animation. The reversed order (close bottom sheet → hide FAB) or both at once does not.

TahaTesser · 2023-06-14T14:30:39Z

@Piinks
This seems to be a risky change and test. Do you know anyone who has expertise in this area to review this PR?

Piinks

Thanks for raising this @TahaTesser. Welcome @utisam! Thanks for contributing!
Can you take a look at the test suggestion below? It may be that we just need a bit more context to understand the design of the test - thank you for including one by the way. :)

packages/flutter/test/material/scaffold_test.dart

TahaTesser

LGTM

TahaTesser · 2023-06-19T07:19:26Z

We'll need a second review from @Piinks before landing this.

Piinks

Just a couple of style guide nits from me, which I was able to just tweak here on github. Thanks again for contributing @utisam! LGTM!

auto-submit · 2023-06-21T22:54:18Z

auto label is removed for flutter/flutter, pr: 128566, due to - The status or check suite Linux flutter_plugins has failed. Please fix the issues identified (or deflake) before re-applying this label.

… at the same time

Co-authored-by: Taha Tesser <[email protected]>

…ilure (flutter/flutter#128566)

flutter/flutter@c40baf4...042c036 2023-06-22 [email protected] Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066) 2023-06-22 [email protected] Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974) 2023-06-22 [email protected] Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566) 2023-06-22 [email protected] Add `InputDecorationTheme.merge` (flutter/flutter#129011) 2023-06-22 [email protected] Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351) 2023-06-22 [email protected] Prevent crashes on range errors when selecting device (flutter/flutter#129290) 2023-06-22 [email protected] Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353) 2023-06-22 [email protected] Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339) 2023-06-22 [email protected] Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337) 2023-06-22 [email protected] Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334) 2023-06-22 [email protected] Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331) 2023-06-22 [email protected] Manual roll of packages to 9af50d4 (flutter/flutter#129328) 2023-06-21 [email protected] Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306) 2023-06-21 [email protected] [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366) 2023-06-21 [email protected] Roll pub packages (flutter/flutter#128966) 2023-06-21 [email protected] Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298) 2023-06-21 [email protected] Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464) 2023-06-21 [email protected] [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222) 2023-06-21 [email protected] Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293) 2023-06-21 [email protected] Selection area right click behavior should match native (flutter/flutter#128224) 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ilure (flutter/flutter#128566)

github-actions bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 9, 2023

utisam marked this pull request as ready for review June 9, 2023 08:14

TahaTesser self-requested a review June 9, 2023 10:12

TahaTesser reviewed Jun 14, 2023

View reviewed changes

Piinks reviewed Jun 14, 2023

View reviewed changes

TahaTesser suggested changes Jun 16, 2023

View reviewed changes

packages/flutter/test/material/scaffold_test.dart Outdated Show resolved Hide resolved

utisam force-pushed the fix_fav_animation_assert branch from 7f95711 to 45c1a68 Compare June 19, 2023 03:03

TahaTesser approved these changes Jun 19, 2023

View reviewed changes

TahaTesser requested a review from Piinks June 19, 2023 07:19

Piinks approved these changes Jun 21, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 21, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 21, 2023

Piinks force-pushed the fix_fav_animation_assert branch from 259e266 to 8b9c1fd Compare June 21, 2023 22:55

utisam and others added 5 commits June 22, 2023 11:25

Check widget.child != null before assert

70c4f21

Add test for closing bottom sheet and removing floating action button…

c064cf7

… at the same time

Remove addPostFrameCallback from test

60da2c3

Update packages/flutter/test/material/scaffold_test.dart

92af90e

Co-authored-by: Taha Tesser <[email protected]>

Update scaffold_test.dart

b781357

Piinks force-pushed the fix_fav_animation_assert branch from 8b9c1fd to b781357 Compare June 22, 2023 16:26

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 22, 2023

auto-submit bot merged commit 0da8012 into flutter:master Jun 22, 2023

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to 0da8012cc77d (18 revisions) flutter/packages#4277

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023

0da8012 Fix: Closing bottom sheet and removing FAB cause assertion fa…

3905db7

…ilure (flutter/flutter#128566)

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to 042c0366c9a9 (20 revisions) flutter/packages#4278

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023

0da8012 Fix: Closing bottom sheet and removing FAB cause assertion fa…

b37335d

…ilure (flutter/flutter#128566)

HansMuller mentioned this pull request Jun 30, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

0da8012 Fix: Closing bottom sheet and removing FAB cause assertion fa…

3f3df42

…ilure (flutter/flutter#128566)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

0da8012 Fix: Closing bottom sheet and removing FAB cause assertion fa…

563fa30

…ilure (flutter/flutter#128566)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

0da8012 Fix: Closing bottom sheet and removing FAB cause assertion fa…

136e11a

…ilure (flutter/flutter#128566)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

0da8012 Fix: Closing bottom sheet and removing FAB cause assertion fa…

89c9771

…ilure (flutter/flutter#128566)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Closing bottom sheet and removing FAB cause assertion failure#128566

Fix: Closing bottom sheet and removing FAB cause assertion failure#128566
auto-submit[bot] merged 5 commits intoflutter:masterfrom
utisam:fix_fav_animation_assert

utisam commented Jun 9, 2023 •

edited

Loading

Uh oh!

TahaTesser commented Jun 9, 2023

Uh oh!

utisam commented Jun 9, 2023

Uh oh!

TahaTesser Jun 14, 2023 •

edited

Loading

Uh oh!

TahaTesser Jun 14, 2023

Uh oh!

Piinks Jun 14, 2023

Uh oh!

utisam Jun 15, 2023

Uh oh!

TahaTesser commented Jun 14, 2023

Uh oh!

Piinks left a comment

Uh oh!

Uh oh!

TahaTesser left a comment

Uh oh!

TahaTesser commented Jun 19, 2023

Uh oh!

Piinks left a comment

Uh oh!

auto-submit bot commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

utisam commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

TahaTesser commented Jun 9, 2023

Uh oh!

utisam commented Jun 9, 2023

Uh oh!

TahaTesser Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TahaTesser Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Piinks Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

utisam Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Jun 14, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TahaTesser left a comment

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Jun 19, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

utisam commented Jun 9, 2023 •

edited

Loading

TahaTesser Jun 14, 2023 •

edited

Loading