Make sure that an AnimatedSwitcher doesn't crash in 0x0 environment#180190
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a regression test for an issue where AnimatedSwitcher could crash when placed in a zero-sized environment. While the added test is a good start, it only covers the initial build and not the case where the child of the AnimatedSwitcher is changed, which was a key part of the original bug report. I've suggested an improvement to the test to make it more comprehensive and ensure the fix is fully covered.
| testWidgets('AnimatedSwitcher does not crash at zero area', (WidgetTester tester) async { | ||
| await tester.pumpWidget( | ||
| const Directionality( | ||
| textDirection: TextDirection.ltr, | ||
| child: Center( | ||
| child: SizedBox.shrink(child: AnimatedSwitcher(duration: Duration(microseconds: 500))), | ||
| ), | ||
| ), | ||
| ); | ||
| expect(tester.getSize(find.byType(AnimatedSwitcher)), Size.zero); | ||
| }); |
There was a problem hiding this comment.
The added test case is a good start for verifying the fix, but it only covers the initial creation of the AnimatedSwitcher with a null child in a zero-sized area. The original issue (#6537) mentions that the crash occurs when the child is changed. To ensure the fix is robust, the test should also cover the scenario of switching children.
testWidgets('AnimatedSwitcher does not crash at zero area', (WidgetTester tester) async {
await tester.pumpWidget(
const Directionality(
textDirection: TextDirection.ltr,
child: Center(
child: SizedBox.shrink(child: AnimatedSwitcher(duration: Duration(microseconds: 500))),
),
),
);
expect(tester.getSize(find.byType(AnimatedSwitcher)), Size.zero);
// Switch to a child.
await tester.pumpWidget(
const Directionality(
textDirection: TextDirection.ltr,
child: Center(
child: SizedBox.shrink(
child: AnimatedSwitcher(
duration: Duration(microseconds: 500),
child: Text('1', key: Key('1')),
),
),
),
),
);
expect(tester.getSize(find.byType(AnimatedSwitcher)), Size.zero);
// Switch to another child.
await tester.pumpWidget(
const Directionality(
textDirection: TextDirection.ltr,
child: Center(
child: SizedBox.shrink(
child: AnimatedSwitcher(
duration: Duration(microseconds: 500),
child: Text('2', key: Key('2')),
),
),
),
),
);
expect(tester.getSize(find.byType(AnimatedSwitcher)), Size.zero);
});
victorsanni
left a comment
There was a problem hiding this comment.
Not sure about a 0x0 test for AnimatedSwitcher. I don't think it has its own renderbox vs. just handling animations for its children.
And wrapping the animating children with an AnimatedSwitcher.layoutBuilder which is just a widget builder. So if the widget returned here already has a 0x0 test I don't know that an additional one is needed. |
|
I think it might be ok to verify that it doesn't add an additional render object on top of the built children. |
|
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. |
|
autosubmit label was removed for flutter/flutter/180190, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
…lutter#180190) This is my attempt to handle flutter#6537 for the AnimatedSwitcher widget. --------- Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com> Co-authored-by: Victor Sanni <victorsanniay@gmail.com>
This is my attempt to handle #6537 for the AnimatedSwitcher widget.