From cc9ffd3f3b0364f331ceb42f8ca23a051cf19a0c Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Mon, 17 Apr 2023 16:19:11 -0700 Subject: [PATCH] =?UTF-8?q?SelectionContainer's=20listeners=20can=20remove?= =?UTF-8?q?=20itself=20during=20listener=20call=E2=80=A6=20(#124624)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When swapping out delegate of selectioncontainer, if the newly passed in delegate doesn't have any selectable content(which is usually the case), the selectioncontainerstate will notify all of the listeners. One of the listener would be SelectionRegistrant._updateSelectionRegistrarSubscription, and since it doesn't have content, it would remove itself from the listener which causes concurrent modification --- ...ectable_region_toolbar_builder.0_test.dart | 2 +- .../lib/src/widgets/selectable_region.dart | 23 ++++--- .../lib/src/widgets/selection_container.dart | 3 +- .../test/material/selection_area_test.dart | 3 + .../test/widgets/default_colors_test.dart | 1 + .../widgets/scrollable_selection_test.dart | 7 +++ .../test/widgets/selectable_region_test.dart | 23 +++++++ .../widgets/selection_container_test.dart | 60 ++++++++++++++++++- 8 files changed, 105 insertions(+), 17 deletions(-) diff --git a/examples/api/test/material/context_menu/selectable_region_toolbar_builder.0_test.dart b/examples/api/test/material/context_menu/selectable_region_toolbar_builder.0_test.dart index dd07306e1d..31b89ee89d 100644 --- a/examples/api/test/material/context_menu/selectable_region_toolbar_builder.0_test.dart +++ b/examples/api/test/material/context_menu/selectable_region_toolbar_builder.0_test.dart @@ -18,7 +18,7 @@ void main() { expect(BrowserContextMenu.enabled, !kIsWeb); // Allow the selection overlay geometry to be created. - await tester.pump(); + await tester.pumpAndSettle(); expect(find.byType(AdaptiveTextSelectionToolbar), findsNothing); diff --git a/packages/flutter/lib/src/widgets/selectable_region.dart b/packages/flutter/lib/src/widgets/selectable_region.dart index 6846489f80..ab368357f2 100644 --- a/packages/flutter/lib/src/widgets/selectable_region.dart +++ b/packages/flutter/lib/src/widgets/selectable_region.dart @@ -1477,7 +1477,7 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai Selectable? _endHandleLayerOwner; bool _isHandlingSelectionEvent = false; - bool _scheduledSelectableUpdate = false; + int? _scheduledSelectableUpdateCallbackId; bool _selectionInProgress = false; Set _additions = {}; @@ -1505,16 +1505,13 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai } void _scheduleSelectableUpdate() { - if (!_scheduledSelectableUpdate) { - _scheduledSelectableUpdate = true; - SchedulerBinding.instance.addPostFrameCallback((Duration timeStamp) { - if (!_scheduledSelectableUpdate) { - return; - } - _scheduledSelectableUpdate = false; - _updateSelectables(); - }); - } + _scheduledSelectableUpdateCallbackId ??= SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) { + if (_scheduledSelectableUpdateCallbackId == null) { + return; + } + _scheduledSelectableUpdateCallbackId = null; + _updateSelectables(); + }); } void _updateSelectables() { @@ -2045,7 +2042,9 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai selectable.removeListener(_handleSelectableGeometryChange); } selectables = const []; - _scheduledSelectableUpdate = false; + if (_scheduledSelectableUpdateCallbackId != null) { + SchedulerBinding.instance.cancelFrameCallbackWithId(_scheduledSelectableUpdateCallbackId!); + } super.dispose(); } diff --git a/packages/flutter/lib/src/widgets/selection_container.dart b/packages/flutter/lib/src/widgets/selection_container.dart index 0eec34f51c..3c16d7909b 100644 --- a/packages/flutter/lib/src/widgets/selection_container.dart +++ b/packages/flutter/lib/src/widgets/selection_container.dart @@ -133,7 +133,8 @@ class _SelectionContainerState extends State with Selectable _listeners.forEach(widget.delegate!.addListener); } if (oldWidget.delegate?.value != widget.delegate?.value) { - for (final VoidCallback listener in _listeners) { + // Avoid concurrent modification. + for (final VoidCallback listener in _listeners.toList(growable: false)) { listener(); } } diff --git a/packages/flutter/test/material/selection_area_test.dart b/packages/flutter/test/material/selection_area_test.dart index 13f690f1fa..0ab44528e6 100644 --- a/packages/flutter/test/material/selection_area_test.dart +++ b/packages/flutter/test/material/selection_area_test.dart @@ -78,6 +78,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); expect(find.byType(AdaptiveTextSelectionToolbar), findsNothing); @@ -111,6 +112,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); expect(find.byType(AdaptiveTextSelectionToolbar), findsNothing); expect(find.byKey(key), findsNothing); @@ -182,6 +184,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph2 = tester.renderObject(find.descendant(of: find.text('Good, and you?'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph2, 7)); // at the 'a' addTearDown(gesture.removePointer); diff --git a/packages/flutter/test/widgets/default_colors_test.dart b/packages/flutter/test/widgets/default_colors_test.dart index cedab6bb81..c3f3eaf204 100644 --- a/packages/flutter/test/widgets/default_colors_test.dart +++ b/packages/flutter/test/widgets/default_colors_test.dart @@ -93,6 +93,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); await _expectColors( tester, find.byType(Align), diff --git a/packages/flutter/test/widgets/scrollable_selection_test.dart b/packages/flutter/test/widgets/scrollable_selection_test.dart index 418454a550..a6b37798c9 100644 --- a/packages/flutter/test/widgets/scrollable_selection_test.dart +++ b/packages/flutter/test/widgets/scrollable_selection_test.dart @@ -47,6 +47,7 @@ void main() { ), ), )); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('Item 0'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph1, 2), kind: ui.PointerDeviceKind.mouse); @@ -85,6 +86,7 @@ void main() { ), ), )); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('Item 0'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph1, 2), kind: ui.PointerDeviceKind.mouse); @@ -118,6 +120,7 @@ void main() { ), ), )); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('Item 0'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph1, 2), kind: ui.PointerDeviceKind.mouse); @@ -171,6 +174,7 @@ void main() { ), ), )); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('Item 0'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph1, 2), kind: ui.PointerDeviceKind.mouse); @@ -210,6 +214,7 @@ void main() { ), ), )); + await tester.pumpAndSettle(); controller.jumpTo(4000); await tester.pumpAndSettle(); @@ -258,6 +263,7 @@ void main() { ), ), )); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('Item 0'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph1, 2), kind: ui.PointerDeviceKind.mouse); @@ -305,6 +311,7 @@ void main() { ), ), )); + await tester.pumpAndSettle(); controller.jumpTo(2080); await tester.pumpAndSettle(); diff --git a/packages/flutter/test/widgets/selectable_region_test.dart b/packages/flutter/test/widgets/selectable_region_test.dart index 45625821af..7e3cdf45af 100644 --- a/packages/flutter/test/widgets/selectable_region_test.dart +++ b/packages/flutter/test/widgets/selectable_region_test.dart @@ -48,6 +48,7 @@ void main() { ), ) ); + await tester.pumpAndSettle(); final RenderSelectionSpy renderSelectionSpy = tester.renderObject(find.byKey(spy)); final TestGesture gesture = await tester.startGesture(const Offset(200.0, 200.0), kind: PointerDeviceKind.mouse); @@ -121,6 +122,8 @@ void main() { ), ), ); + await tester.pumpAndSettle(); + final TestGesture gesture = await tester.startGesture(tester.getCenter(find.byKey(spy))); addTearDown(gesture.removePointer); await tester.pump(const Duration(milliseconds: 500)); @@ -241,6 +244,7 @@ void main() { ), ) ); + await tester.pumpAndSettle(); final RenderSelectionSpy renderSelectionSpy = tester.renderObject(find.byKey(spy)); final TestGesture gesture = await tester.startGesture(const Offset(200.0, 200.0), kind: PointerDeviceKind.mouse); @@ -260,6 +264,7 @@ void main() { ), ) ); + await tester.pumpAndSettle(); final RenderSelectionSpy renderSelectionSpy = tester.renderObject(find.byKey(spy)); renderSelectionSpy.events.clear(); @@ -284,6 +289,7 @@ void main() { ), ) ); + await tester.pumpAndSettle(); final RenderSelectionSpy renderSelectionSpy = tester.renderObject(find.byKey(spy)); renderSelectionSpy.events.clear(); @@ -315,6 +321,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderSelectionSpy renderSelectionSpy = tester.renderObject(find.byKey(spy)); renderSelectionSpy.events.clear(); @@ -348,6 +355,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph = tester.renderObject(find.descendant(of: find.text('How are you?'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph, 6)); // at the 'r' @@ -708,6 +716,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); focusNode.requestFocus(); // keyboard select all. @@ -897,6 +906,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('How are you?'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph1, 6)); // at the 'r' addTearDown(gesture.removePointer); @@ -928,6 +938,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph2 = tester.renderObject(find.descendant(of: find.text('Good, and you?'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph2, 7)); // at the 'a' @@ -964,6 +975,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph2 = tester.renderObject(find.descendant(of: find.text('Good, and you?'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph2, 7)); // at the 'a' addTearDown(gesture.removePointer); @@ -998,6 +1010,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph3 = tester.renderObject(find.descendant(of: find.text('Fine, thank you.'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph3, 7)); // at the 'h' addTearDown(gesture.removePointer); @@ -1039,6 +1052,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph3 = tester.renderObject(find.descendant(of: find.text('Fine, thank you.'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph3, 7)); // at the 'h' addTearDown(gesture.removePointer); @@ -1075,6 +1089,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph3 = tester.renderObject(find.descendant(of: find.text('Fine, thank you.'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph3, 7)); // at the 'h' addTearDown(gesture.removePointer); @@ -1111,6 +1126,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph3 = tester.renderObject(find.descendant(of: find.text('Fine, thank you.'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph3, 7)); // at the 'h' addTearDown(gesture.removePointer); @@ -1146,6 +1162,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph3 = tester.renderObject(find.descendant(of: find.text('Fine, thank you.'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph3, 7)); // at the 'h' addTearDown(gesture.removePointer); @@ -1621,6 +1638,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph = tester.renderObject( find.descendant( @@ -1676,6 +1694,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('How are you?'), matching: find.byType(RichText))); final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph1, 6)); // at the 'r' @@ -1723,6 +1742,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('How are you?'), matching: find.byType(RichText))); await tester.longPressAt(textOffsetToPosition(paragraph1, 6)); // at the 'r' @@ -1776,6 +1796,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final RenderParagraph paragraph1 = tester.renderObject(find.descendant(of: find.text('How are you?'), matching: find.byType(RichText))); await tester.longPressAt(textOffsetToPosition(paragraph1, 6)); // at the 'r' @@ -1830,6 +1851,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); expect(find.byType(AdaptiveTextSelectionToolbar), findsNothing); @@ -1913,6 +1935,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); final SelectableRegionState state = tester.state(find.byType(SelectableRegion)); diff --git a/packages/flutter/test/widgets/selection_container_test.dart b/packages/flutter/test/widgets/selection_container_test.dart index 22db563b81..385b0d1fa2 100644 --- a/packages/flutter/test/widgets/selection_container_test.dart +++ b/packages/flutter/test/widgets/selection_container_test.dart @@ -10,9 +10,12 @@ void main() { Future pumpContainer(WidgetTester tester, Widget child) async { await tester.pumpWidget( - DefaultSelectionStyle( - selectionColor: Colors.red, - child: child, + Directionality( + textDirection: TextDirection.ltr, + child: DefaultSelectionStyle( + selectionColor: Colors.red, + child: child, + ), ), ); } @@ -34,6 +37,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); expect(registrar.selectables.length, 1); expect(delegate.selectables.length, 3); }); @@ -61,6 +65,53 @@ void main() { expect(delegate.selectables.length, 0); }); + testWidgets('Swapping out container delegate does not crash', (WidgetTester tester) async { + final TestSelectionRegistrar registrar = TestSelectionRegistrar(); + final TestContainerDelegate delegate = TestContainerDelegate(); + final TestContainerDelegate childDelegate = TestContainerDelegate(); + await pumpContainer( + tester, + SelectionContainer( + registrar: registrar, + delegate: delegate, + child: Builder( + builder: (BuildContext context) { + return SelectionContainer( + registrar: SelectionContainer.maybeOf(context), + delegate: childDelegate, + child: const Text('dummy'), + ); + }, + ) + ), + ); + await tester.pumpAndSettle(); + expect(registrar.selectables.length, 1); + expect(delegate.value.hasContent, isTrue); + + final TestContainerDelegate newDelegate = TestContainerDelegate(); + await pumpContainer( + tester, + SelectionContainer( + registrar: registrar, + delegate: delegate, + child: Builder( + builder: (BuildContext context) { + return SelectionContainer( + registrar: SelectionContainer.maybeOf(context), + delegate: newDelegate, + child: const Text('dummy'), + ); + }, + ) + ), + ); + await tester.pumpAndSettle(); + expect(registrar.selectables.length, 1); + expect(delegate.value.hasContent, isTrue); + expect(tester.takeException(), isNull); + }); + testWidgets('selection container registers itself if there is a selectable child', (WidgetTester tester) async { final TestSelectionRegistrar registrar = TestSelectionRegistrar(); final TestContainerDelegate delegate = TestContainerDelegate(); @@ -87,6 +138,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); expect(registrar.selectables.length, 1); await pumpContainer( @@ -98,6 +150,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); expect(registrar.selectables.length, 0); }); @@ -119,6 +172,7 @@ void main() { ), ), ); + await tester.pumpAndSettle(); expect(registrar.selectables.length, 1); }); }