Skip to content

Commit

Permalink
Revert selectable update back to be a postframecallback or microtask (#…
Browse files Browse the repository at this point in the history
…125140)

The regression was caused by the previous pr #124624 changes postframecallback to scheduleframecallback. The reason is that if a new postframecallback was scheduled when running a postframecallback. The newly added postframecallback will be execute on the next frame. However, adding postframecallback will not schedule a new frame. So if there isn't other widget that schedule a new frame, the newly added postframecallback will never gets run.

After changing to scheduleframecallback, it causes an issue that transient callback may be called when rendering tree contains dirty layout information that are waiting to be rebuilt.

Therefore, I use microtask to get around of the postframecallback issue instead of scheduleframecallback.

fixes #125065
  • Loading branch information
chunhtai committed Apr 19, 2023
1 parent 898767f commit 14e191f
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 17 deletions.
36 changes: 26 additions & 10 deletions packages/flutter/lib/src/widgets/selectable_region.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';
import 'dart:math';

import 'package:flutter/foundation.dart';
Expand Down Expand Up @@ -1477,7 +1478,7 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
Selectable? _endHandleLayerOwner;

bool _isHandlingSelectionEvent = false;
int? _scheduledSelectableUpdateCallbackId;
bool _scheduledSelectableUpdate = false;
bool _selectionInProgress = false;
Set<Selectable> _additions = <Selectable>{};

Expand All @@ -1493,6 +1494,10 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
@override
void remove(Selectable selectable) {
if (_additions.remove(selectable)) {
// The same selectable was added in the same frame and is not yet
// incorporated into the selectables.
//
// Removing such selectable doesn't require selection geometry update.
return;
}
_removeSelectable(selectable);
Expand All @@ -1505,13 +1510,26 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
}

void _scheduleSelectableUpdate() {
_scheduledSelectableUpdateCallbackId ??= SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) {
if (_scheduledSelectableUpdateCallbackId == null) {
return;
if (!_scheduledSelectableUpdate) {
_scheduledSelectableUpdate = true;
void runScheduledTask([Duration? duration]) {
if (!_scheduledSelectableUpdate) {
return;
}
_scheduledSelectableUpdate = false;
_updateSelectables();
}

if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.postFrameCallbacks) {
// A new task can be scheduled as a result of running the scheduled task
// from another MultiSelectableSelectionContainerDelegate. This can
// happen if nesting two SelectionContainers. The selectable can be
// safely updated in the same frame in this case.
scheduleMicrotask(runScheduledTask);
} else {
SchedulerBinding.instance.addPostFrameCallback(runScheduledTask);
}
_scheduledSelectableUpdateCallbackId = null;
_updateSelectables();
});
}
}

void _updateSelectables() {
Expand Down Expand Up @@ -2042,9 +2060,7 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
selectable.removeListener(_handleSelectableGeometryChange);
}
selectables = const <Selectable>[];
if (_scheduledSelectableUpdateCallbackId != null) {
SchedulerBinding.instance.cancelFrameCallbackWithId(_scheduledSelectableUpdateCallbackId!);
}
_scheduledSelectableUpdate = false;
super.dispose();
}

Expand Down
14 changes: 7 additions & 7 deletions packages/flutter/test/widgets/selectable_region_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@ void main() {
testWidgets('mouse selection sends correct events', (WidgetTester tester) async {
final UniqueKey spy = UniqueKey();
await tester.pumpWidget(
MaterialApp(
home: SelectableRegion(
focusNode: FocusNode(),
selectionControls: materialTextSelectionControls,
child: SelectionSpy(key: spy),
),
)
MaterialApp(
home: SelectableRegion(
focusNode: FocusNode(),
selectionControls: materialTextSelectionControls,
child: SelectionSpy(key: spy),
),
),
);
await tester.pumpAndSettle();

Expand Down
27 changes: 27 additions & 0 deletions packages/flutter/test/widgets/selection_container_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,33 @@ void main() {
expect(tester.takeException(), isNull);
});

testWidgets('Can update within one frame', (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.pump();
// Should finish update after flushing the micro tasks.
await tester.idle();
expect(registrar.selectables.length, 1);
expect(delegate.value.hasContent, isTrue);
});

testWidgets('selection container registers itself if there is a selectable child', (WidgetTester tester) async {
final TestSelectionRegistrar registrar = TestSelectionRegistrar();
final TestContainerDelegate delegate = TestContainerDelegate();
Expand Down

0 comments on commit 14e191f

Please sign in to comment.