From 2b138fd7d20caa552a50e8f6dfdba0017099a7be Mon Sep 17 00:00:00 2001
From: krisgiesing <kgiesing@google.com>
Date: Fri, 27 Sep 2019 10:48:42 -0700
Subject: [PATCH] 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.
---
 .../lib/src/material/reorderable_list.dart    | 38 +++++++++++++++-
 .../test/material/reorderable_list_test.dart  | 45 +++++++++++++++++++
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/packages/flutter/lib/src/material/reorderable_list.dart b/packages/flutter/lib/src/material/reorderable_list.dart
index 7e901c4d969..a2430c70043 100644
--- a/packages/flutter/lib/src/material/reorderable_list.dart
+++ b/packages/flutter/lib/src/material/reorderable_list.dart
@@ -4,8 +4,9 @@
 
 import 'dart:math';
 
-import 'package:flutter/widgets.dart';
+import 'package:flutter/foundation.dart';
 import 'package:flutter/rendering.dart';
+import 'package:flutter/widgets.dart';
 
 import 'debug.dart';
 import 'material.dart';
@@ -344,7 +345,11 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T
   // Handles up the logic for dragging and reordering items in the list.
   Widget _wrap(Widget toWrap, int index, BoxConstraints constraints) {
     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
     // item gets dragged, the accessibility framework can preserve the selected
     // state of the dragging item.
@@ -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)}]';
+  }
+}
diff --git a/packages/flutter/test/material/reorderable_list_test.dart b/packages/flutter/test/material/reorderable_list_test.dart
index b765d240f0c..5027d1d905e 100644
--- a/packages/flutter/test/material/reorderable_list_test.dart
+++ b/packages/flutter/test/material/reorderable_list_test.dart
@@ -655,6 +655,51 @@ void main() {
         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)', () {
         Map<CustomSemanticsAction, VoidCallback> getSemanticsActions(int index) {
           final Semantics semantics = find.ancestor(