Unverified Commit 2b138fd7 authored by krisgiesing's avatar krisgiesing Committed by GitHub

Fix ReorderableListView's use of child keys (#41334) (#41338)

ReorderableListView was constructing a GlobalObjectKey using
the child key as the value. This only had the intended behavior
if the child key was identical across build method invocations.

The new strategy is to scope the child key's value to the
State object's identity, allowing child keys to have value
compare semantics while disambiguating among different list view
instances.
parent d7a8dda2
...@@ -4,8 +4,9 @@ ...@@ -4,8 +4,9 @@
import 'dart:math'; import 'dart:math';
import 'package:flutter/widgets.dart'; import 'package:flutter/foundation.dart';
import 'package:flutter/rendering.dart'; import 'package:flutter/rendering.dart';
import 'package:flutter/widgets.dart';
import 'debug.dart'; import 'debug.dart';
import 'material.dart'; import 'material.dart';
...@@ -344,7 +345,11 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T ...@@ -344,7 +345,11 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T
// Handles up the logic for dragging and reordering items in the list. // Handles up the logic for dragging and reordering items in the list.
Widget _wrap(Widget toWrap, int index, BoxConstraints constraints) { Widget _wrap(Widget toWrap, int index, BoxConstraints constraints) {
assert(toWrap.key != null); assert(toWrap.key != null);
final GlobalObjectKey keyIndexGlobalKey = GlobalObjectKey(toWrap.key); final _ScopedValueGlobalKey<_ReorderableListContentState> keyIndexGlobalKey =
_ScopedValueGlobalKey<_ReorderableListContentState>(
scope: this,
value: toWrap.key,
);
// We pass the toWrapWithGlobalKey into the Draggable so that when a list // We pass the toWrapWithGlobalKey into the Draggable so that when a list
// item gets dragged, the accessibility framework can preserve the selected // item gets dragged, the accessibility framework can preserve the selected
// state of the dragging item. // state of the dragging item.
...@@ -574,3 +579,32 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T ...@@ -574,3 +579,32 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T
}); });
} }
} }
/// A [GlobalKey] that uses a scope and a value to determine equality.
///
/// The scope is compared using [identical], while the value is compared
/// using [operator ==]. This allows a locally scoped value to be turned
/// into a globally unique key.
class _ScopedValueGlobalKey<T extends State<StatefulWidget>> extends GlobalKey<T> {
const _ScopedValueGlobalKey({this.scope, this.value}) : super.constructor();
final Object scope;
final Object value;
@override
int get hashCode => hashValues(identityHashCode(scope), value.hashCode);
@override
bool operator ==(dynamic other) {
if (other.runtimeType != runtimeType) {
return false;
}
final _ScopedValueGlobalKey<T> typedOther = other;
return identical(scope, typedOther.scope) && value == typedOther.value;
}
@override
String toString() {
return '[$runtimeType ${describeIdentity(scope)} ${describeIdentity(value)}]';
}
}
...@@ -655,6 +655,51 @@ void main() { ...@@ -655,6 +655,51 @@ void main() {
expect(findState(const Key('A')).checked, true); expect(findState(const Key('A')).checked, true);
}); });
testWidgets('Preserves children states across reorder when keys are not identical', (WidgetTester tester) async {
_StatefulState findState(Key key) {
return find.byElementPredicate((Element element) => element.ancestorWidgetOfExactType(_Stateful)?.key == key)
.evaluate()
.first
.ancestorStateOfType(const TypeMatcher<_StatefulState>());
}
await tester.pumpWidget(MaterialApp(
home: ReorderableListView(
children: <Widget>[
_Stateful(key: const ObjectKey('A')),
_Stateful(key: const ObjectKey('B')),
_Stateful(key: const ObjectKey('C')),
],
onReorder: (int oldIndex, int newIndex) { },
scrollDirection: Axis.horizontal,
),
));
await tester.tap(find.byKey(const ObjectKey('A')));
await tester.pumpAndSettle();
// Only the 'A' widget should be checked.
expect(findState(const ObjectKey('A')).checked, true);
expect(findState(const ObjectKey('B')).checked, false);
expect(findState(const ObjectKey('C')).checked, false);
// Rebuild with distinct key objects.
await tester.pumpWidget(MaterialApp(
home: ReorderableListView(
children: <Widget>[
// Deliberately avoid the const constructor below to ensure keys are
// distinct objects.
_Stateful(key: ObjectKey('B')), // ignore:prefer_const_constructors
_Stateful(key: ObjectKey('C')), // ignore:prefer_const_constructors
_Stateful(key: ObjectKey('A')), // ignore:prefer_const_constructors
],
onReorder: (int oldIndex, int newIndex) { },
scrollDirection: Axis.horizontal,
),
));
// Only the 'A' widget should be checked.
expect(findState(const ObjectKey('B')).checked, false);
expect(findState(const ObjectKey('C')).checked, false);
expect(findState(const ObjectKey('A')).checked, true);
});
group('Accessibility (a11y/Semantics)', () { group('Accessibility (a11y/Semantics)', () {
Map<CustomSemanticsAction, VoidCallback> getSemanticsActions(int index) { Map<CustomSemanticsAction, VoidCallback> getSemanticsActions(int index) {
final Semantics semantics = find.ancestor( final Semantics semantics = find.ancestor(
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment