-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Make sure that a Scaffold doesn't crash in 0x0 environment #177722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a regression test to ensure Scaffold does not crash when rendered in a 0x0 environment. The test is a great addition, but as implemented, it doesn't fully exercise the code path that caused the original crash. I've left a comment with a suggested change to make the test more effective.
Also, as a minor process note, please remember to include the pre-launch checklist in the pull request description as per the repository's contributing guidelines.
| testWidgets('Scaffold does not crash at zero area', (WidgetTester tester) async { | ||
| await tester.pumpWidget( | ||
| const MaterialApp( | ||
| home: Center(child: SizedBox.shrink(child: Scaffold())), | ||
| ), | ||
| ); | ||
| expect(tester.getSize(find.byType(Scaffold)), Size.zero); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current test uses a Scaffold with no body. This means the code path that was causing the crash (_ScaffoldLayout.performLayout's body layout logic) is not executed, as it's guarded by if (hasChild(_ScaffoldSlot.body)). To properly test the fix and prevent regressions, the Scaffold should have at least a body.
Including an appBar would make the test more robust by ensuring layout calculations with non-zero contentTop are also tested under 0x0 size conditions, which was part of the original issue.
| testWidgets('Scaffold does not crash at zero area', (WidgetTester tester) async { | |
| await tester.pumpWidget( | |
| const MaterialApp( | |
| home: Center(child: SizedBox.shrink(child: Scaffold())), | |
| ), | |
| ); | |
| expect(tester.getSize(find.byType(Scaffold)), Size.zero); | |
| }); | |
| testWidgets('Scaffold does not crash at zero area', (WidgetTester tester) async { | |
| await tester.pumpWidget( | |
| const MaterialApp( | |
| home: Center(child: SizedBox.shrink(child: Scaffold(body: SizedBox(), appBar: AppBar()))), | |
| ), | |
| ); | |
| expect(tester.getSize(find.byType(Scaffold)), Size.zero); | |
| }); |
bcc90f0 to
f1154bc
Compare
dkwingsmt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checklist:
- The test is in the correct file
- The test name goes “does not crash at zero area”
- The target widget is wrapped by
Center(or is fullscreen) - The target widget does not have an overlay, or the overlay is tested
- The target widget is expected to have a size of exactly
Size.zero
…77722) This is my attempt to handle flutter#6537 for the Scaffold widget.
This is my attempt to handle #6537 for the Scaffold widget.