Make sure that a DesktopTextSelectionToolbar doesn't crash in 0x0 env… #173928
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a test case to ensure that DesktopTextSelectionToolbar does not crash when rendered in a zero-sized environment, which is a good addition to prevent regressions. My feedback focuses on improving the new test by adding an explicit assertion to make its intent clearer and more robust.
| testWidgets('DesktopTextSelectionToolbar renders at zero area', (WidgetTester tester) async { | ||
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: SizedBox.shrink( | ||
| child: DesktopTextSelectionToolbar( | ||
| anchor: const Offset(10, 10), | ||
| children: const <Widget>[Text('X')], | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
While this test correctly checks that pumping the widget in a zero-sized area doesn't cause a crash, it would be more robust and clearer to add an assertion to verify that the widget is actually present in the widget tree.1
Adding an expect call ensures that the test is not passing simply because the widget failed to render silently, making the test's purpose more explicit.
testWidgets('DesktopTextSelectionToolbar renders at zero area', (WidgetTester tester) async {
await tester.pumpWidget(
MaterialApp(
home: SizedBox.shrink(
child: DesktopTextSelectionToolbar(
anchor: const Offset(10, 10),
children: const <Widget>[Text('X')],
),
),
),
);
expect(find.byType(DesktopTextSelectionToolbar), findsOneWidget);
});
Style Guide References
Footnotes
There was a problem hiding this comment.
I think this is fine either way.
There was a problem hiding this comment.
Actually, in all of these PRs, we should have an a expect that checks that the relevant widget (DesktopTextSelectionToolbar here) has a size of 0x0. Can you add that @ahmedsameha1?
Thanks @LongCatIsLooong for flagging this.
There was a problem hiding this comment.
@justinmc We don't need any expect. The original issue only wants to make sure that it doesn't crash, not even needing the actual size to be zero.
There was a problem hiding this comment.
LGTM 👍
@dkwingsmt During triage you mentioned you were concerned about this test, were you wondering if the toolbar would end up in an Overlay or what was it?
| testWidgets('DesktopTextSelectionToolbar renders at zero area', (WidgetTester tester) async { | ||
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: SizedBox.shrink( | ||
| child: DesktopTextSelectionToolbar( | ||
| anchor: const Offset(10, 10), | ||
| children: const <Widget>[Text('X')], | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I think this is fine either way.
|
Yeah, I was saying I wanted someone familiar with this to verify its validity, such as making sure it's not laying out overlay. |
|
In that case we are good, the Overlay thing is done in text_selection.dart, not as a part of DesktopTextSelectionToolbar itself 👍 . Thanks for clarifying! |
| testWidgets('DesktopTextSelectionToolbar renders at zero area', (WidgetTester tester) async { | ||
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: SizedBox.shrink( | ||
| child: DesktopTextSelectionToolbar( | ||
| anchor: const Offset(10, 10), | ||
| children: const <Widget>[Text('X')], | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Actually, in all of these PRs, we should have an a expect that checks that the relevant widget (DesktopTextSelectionToolbar here) has a size of 0x0. Can you add that @ahmedsameha1?
Thanks @LongCatIsLooong for flagging this.
|
@justinmc Could you resolve this in #6537 because, according to #6537 (comment), there is no need to add that expectation. |
This is my attempt to handle #6537 for the DesktopTextSelectionToolbar UI control.