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

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

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


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

+      return MaterialApp(
+        home: Scaffold(
+          body: Builder(builder: (BuildContext context) {
+            WidgetsBinding.instance.addPostFrameCallback((_) {


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 <tessertaha@gmail.com>

…failure (flutter/flutter#128566)

flutter/flutter@c40baf4...042c036 2023-06-22 ahmedelsaayid@gmail.com Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066) 2023-06-22 tessertaha@gmail.com Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974) 2023-06-22 utisam@gmail.com Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566) 2023-06-22 tessertaha@gmail.com Add `InputDecorationTheme.merge` (flutter/flutter#129011) 2023-06-22 engine-flutter-autoroll@skia.org Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351) 2023-06-22 42216813+eliasyishak@users.noreply.github.com Prevent crashes on range errors when selecting device (flutter/flutter#129290) 2023-06-22 zanderso@users.noreply.github.com Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331) 2023-06-22 jason-simmons@users.noreply.github.com Manual roll of packages to 9af50d4 (flutter/flutter#129328) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306) 2023-06-21 yjbanov@google.com [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366) 2023-06-21 fluttergithubbot@gmail.com Roll pub packages (flutter/flutter#128966) 2023-06-21 6655696+guidezpl@users.noreply.github.com Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298) 2023-06-21 jmccandless@google.com Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464) 2023-06-21 christopherfujino@gmail.com [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293) 2023-06-21 rmolivares@renzo-olivares.dev 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 dit@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://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

…failure (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 requested 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 <tessertaha@gmail.com>

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
71 checks passed

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

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

3905db7

…failure (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

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

b37335d

…failure (flutter/flutter#128566)

HansMuller mentioned this pull request Jun 30, 2023

Flutter Framework Team: Notable Commits #121415

Open

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

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

3f3df42

…failure (flutter/flutter#128566)

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

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

563fa30

…failure (flutter/flutter#128566)

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

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

136e11a

…failure (flutter/flutter#128566)

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

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

89c9771

…failure (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

utisam commented Jun 9, 2023 •

edited

TahaTesser commented Jun 9, 2023

utisam commented Jun 9, 2023

TahaTesser Jun 14, 2023 •

edited

TahaTesser Jun 14, 2023

Piinks Jun 14, 2023

utisam Jun 15, 2023

TahaTesser commented Jun 14, 2023

Piinks left a comment

TahaTesser left a comment

TahaTesser commented Jun 19, 2023

Piinks left a comment

auto-submit bot commented Jun 21, 2023

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

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

Conversation

utisam commented Jun 9, 2023 • edited

Pre-launch Checklist

TahaTesser commented Jun 9, 2023

utisam commented Jun 9, 2023

TahaTesser Jun 14, 2023 • edited

Choose a reason for hiding this comment

TahaTesser Jun 14, 2023

Choose a reason for hiding this comment

Piinks Jun 14, 2023

Choose a reason for hiding this comment

utisam Jun 15, 2023

Choose a reason for hiding this comment

TahaTesser commented Jun 14, 2023

Piinks left a comment

Choose a reason for hiding this comment

TahaTesser left a comment

Choose a reason for hiding this comment

TahaTesser commented Jun 19, 2023

Piinks left a comment

Choose a reason for hiding this comment

auto-submit bot commented Jun 21, 2023

utisam commented Jun 9, 2023 •

edited

TahaTesser Jun 14, 2023 •

edited