From 2dc71a343f087da118356cabb23509e762166c23 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Fri, 31 Jan 2020 12:51:42 -0800 Subject: [PATCH] reland Refactors global key duplication detection (#49896) * reland "Refactors global key duplication detection (#46183)" This reverts commit d2b66dbfcfdad68473fc4366e3042cd2e17706ac. * fix test --- .../lib/src/cupertino/action_sheet.dart | 1 + .../flutter/lib/src/cupertino/dialog.dart | 1 + packages/flutter/lib/src/material/chip.dart | 1 + .../lib/src/material/input_decorator.dart | 1 + .../flutter/lib/src/material/list_tile.dart | 1 + packages/flutter/lib/src/widgets/binding.dart | 1 + .../flutter/lib/src/widgets/framework.dart | 233 ++++--- .../lib/src/widgets/layout_builder.dart | 1 + .../src/widgets/list_wheel_scroll_view.dart | 1 + packages/flutter/lib/src/widgets/sliver.dart | 1 + .../src/widgets/sliver_persistent_header.dart | 1 + packages/flutter/lib/src/widgets/table.dart | 1 + .../foundation/diagnostics_json_test.dart | 5 - .../flutter/test/widgets/framework_test.dart | 591 +++++++++++++++++- 14 files changed, 756 insertions(+), 84 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/action_sheet.dart b/packages/flutter/lib/src/cupertino/action_sheet.dart index 719c6fb117..49f4c533d6 100644 --- a/packages/flutter/lib/src/cupertino/action_sheet.dart +++ b/packages/flutter/lib/src/cupertino/action_sheet.dart @@ -469,6 +469,7 @@ class _CupertinoAlertRenderElement extends RenderObjectElement { } else if (_actionsElement == child) { _actionsElement = null; } + super.forgetChild(child); } @override diff --git a/packages/flutter/lib/src/cupertino/dialog.dart b/packages/flutter/lib/src/cupertino/dialog.dart index b91b8fbb99..305bba5d55 100644 --- a/packages/flutter/lib/src/cupertino/dialog.dart +++ b/packages/flutter/lib/src/cupertino/dialog.dart @@ -464,6 +464,7 @@ class _CupertinoDialogRenderElement extends RenderObjectElement { assert(_actionsElement == child); _actionsElement = null; } + super.forgetChild(child); } @override diff --git a/packages/flutter/lib/src/material/chip.dart b/packages/flutter/lib/src/material/chip.dart index cdcb0d4c6f..6e5db33da8 100644 --- a/packages/flutter/lib/src/material/chip.dart +++ b/packages/flutter/lib/src/material/chip.dart @@ -2072,6 +2072,7 @@ class _RenderChipElement extends RenderObjectElement { final _ChipSlot slot = childToSlot[child]; childToSlot.remove(child); slotToChild.remove(slot); + super.forgetChild(child); } void _mountChild(Widget widget, _ChipSlot slot) { diff --git a/packages/flutter/lib/src/material/input_decorator.dart b/packages/flutter/lib/src/material/input_decorator.dart index 00cbb800b1..b77267ffa4 100644 --- a/packages/flutter/lib/src/material/input_decorator.dart +++ b/packages/flutter/lib/src/material/input_decorator.dart @@ -1511,6 +1511,7 @@ class _RenderDecorationElement extends RenderObjectElement { final _DecorationSlot slot = childToSlot[child]; childToSlot.remove(child); slotToChild.remove(slot); + super.forgetChild(child); } void _mountChild(Widget widget, _DecorationSlot slot) { diff --git a/packages/flutter/lib/src/material/list_tile.dart b/packages/flutter/lib/src/material/list_tile.dart index dba76f30a9..8f05df4363 100644 --- a/packages/flutter/lib/src/material/list_tile.dart +++ b/packages/flutter/lib/src/material/list_tile.dart @@ -992,6 +992,7 @@ class _ListTileElement extends RenderObjectElement { final _ListTileSlot slot = childToSlot[child]; childToSlot.remove(child); slotToChild.remove(slot); + super.forgetChild(child); } void _mountChild(Widget widget, _ListTileSlot slot) { diff --git a/packages/flutter/lib/src/widgets/binding.dart b/packages/flutter/lib/src/widgets/binding.dart index 8e3d813065..c6232ed59a 100644 --- a/packages/flutter/lib/src/widgets/binding.dart +++ b/packages/flutter/lib/src/widgets/binding.dart @@ -1016,6 +1016,7 @@ class RenderObjectToWidgetElement extends RootRenderObje void forgetChild(Element child) { assert(child == _child); _child = null; + super.forgetChild(child); } @override diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index 0807fa944a..2b697db9cf 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -132,7 +132,20 @@ abstract class GlobalKey> extends Key { static final Map _registry = {}; static final Set _debugIllFatedElements = HashSet(); - static final Map _debugReservations = {}; + // This map keeps track which child reserve the global key with the parent. + // Parent, child -> global key. + // This provides us a way to remove old reservation while parent rebuilds the + // child in the same slot. + static final Map> _debugReservations = >{}; + + static void _debugRemoveReservationFor(Element parent, Element child) { + assert(() { + assert(parent != null); + assert(child != null); + _debugReservations[parent]?.remove(child); + return true; + }()); + } void _register(Element element) { assert(() { @@ -160,46 +173,83 @@ abstract class GlobalKey> extends Key { _registry.remove(this); } - void _debugReserveFor(Element parent) { + void _debugReserveFor(Element parent, Element child) { assert(() { assert(parent != null); - if (_debugReservations.containsKey(this) && _debugReservations[this] != parent) { - // Reserving a new parent while the old parent is not attached is ok. - // This can happen when a renderObject detaches and re-attaches to rendering - // tree multiple times. - if (_debugReservations[this].renderObject?.attached == false) { - _debugReservations[this] = parent; - return true; - } - // It's possible for an element to get built multiple times in one - // frame, in which case it'll reserve the same child's key multiple - // times. We catch multiple children of one widget having the same key - // by verifying that an element never steals elements from itself, so we - // don't care to verify that here as well. - final String older = _debugReservations[this].toString(); - final String newer = parent.toString(); - if (older != newer) { - throw FlutterError.fromParts([ - ErrorSummary('Multiple widgets used the same GlobalKey.'), - ErrorDescription( - 'The key $this was used by multiple widgets. The parents of those widgets were:\n' - '- $older\n' - '- $newer\n' - 'A GlobalKey can only be specified on one widget at a time in the widget tree.' - ), - ]); - } - throw FlutterError.fromParts([ - ErrorSummary('Multiple widgets used the same GlobalKey.'), - ErrorDescription( - 'The key $this was used by multiple widgets. The parents of those widgets were ' - 'different widgets that both had the following description:\n' - ' $parent\n' - 'A GlobalKey can only be specified on one widget at a time in the widget tree.' - ), - ]); - } - _debugReservations[this] = parent; + assert(child != null); + _debugReservations[parent] ??= {}; + _debugReservations[parent][child] = this; + return true; + }()); + } + + static void _debugVerifyGlobalKeyReservation() { + assert(() { + final Map keyToParent = {}; + _debugReservations.forEach((Element parent, Map chidToKey) { + // We ignore parent that are detached. + if (parent.renderObject?.attached == false) + return; + chidToKey.forEach((Element child, GlobalKey key) { + // If parent = null, the node is deactivated by its parent and is + // not re-attached to other part of the tree. We should ignore this + // node. + if (child._parent == null) + return; + // It is possible the same key registers to the same parent twice + // with different children. That is illegal, but it is not in the + // scope of this check. Such error will be detected in + // _debugVerifyIllFatedPopulation or + // _debugElementsThatWillNeedToBeRebuiltDueToGlobalKeyShenanigans. + if (keyToParent.containsKey(key) && keyToParent[key] != parent) { + // We have duplication reservations for the same global key. + final Element older = keyToParent[key]; + final Element newer = parent; + FlutterError error; + if (older.toString() != newer.toString()) { + error = FlutterError.fromParts([ + ErrorSummary('Multiple widgets used the same GlobalKey.'), + ErrorDescription( + 'The key $key was used by multiple widgets. The parents of those widgets were:\n' + '- ${older.toString()}\n' + '- ${newer.toString()}\n' + 'A GlobalKey can only be specified on one widget at a time in the widget tree.' + ), + ]); + } else { + error = FlutterError.fromParts([ + ErrorSummary('Multiple widgets used the same GlobalKey.'), + ErrorDescription( + 'The key $key was used by multiple widgets. The parents of those widgets were ' + 'different widgets that both had the following description:\n' + ' ${parent.toString()}\n' + 'A GlobalKey can only be specified on one widget at a time in the widget tree.' + ), + ]); + } + // Fix the tree by removing the duplicated child from one of its + // parents to resolve the duplicated key issue. This allows us to + // tear down the tree during testing without producing additional + // misleading exceptions. + if (child._parent != older) { + older.visitChildren((Element currentChild) { + if (currentChild == child) + older.forgetChild(child); + }); + } + if (child._parent != newer) { + newer.visitChildren((Element currentChild) { + if (currentChild == child) + newer.forgetChild(child); + }); + } + throw error; + } else { + keyToParent[key] = parent; + } + }); + }); + _debugReservations.clear(); return true; }()); } @@ -215,13 +265,13 @@ abstract class GlobalKey> extends Key { final GlobalKey key = element.widget.key as GlobalKey; assert(_registry.containsKey(key)); duplicates ??= >{}; - final Set elements = duplicates.putIfAbsent(key, () => HashSet()); + // Uses ordered set to produce consistent error message. + final Set elements = duplicates.putIfAbsent(key, () => LinkedHashSet()); elements.add(element); elements.add(_registry[key]); } } _debugIllFatedElements.clear(); - _debugReservations.clear(); if (duplicates != null) { final List information = []; information.add(ErrorSummary('Multiple widgets used the same GlobalKey.')); @@ -2640,6 +2690,7 @@ class BuildOwner { }); assert(() { try { + GlobalKey._debugVerifyGlobalKeyReservation(); GlobalKey._debugVerifyIllFatedPopulation(); if (_debugElementsThatWillNeedToBeRebuiltDueToGlobalKeyShenanigans != null && _debugElementsThatWillNeedToBeRebuiltDueToGlobalKeyShenanigans.isNotEmpty) { @@ -3094,18 +3145,12 @@ abstract class Element extends DiagnosticableTree implements BuildContext { /// | **child != null** | Old child is removed, returns null. | Old child updated if possible, returns child or new [Element]. | @protected Element updateChild(Element child, Widget newWidget, dynamic newSlot) { - assert(() { - final Key key = newWidget?.key; - if (key is GlobalKey) { - key._debugReserveFor(this); - } - return true; - }()); if (newWidget == null) { if (child != null) deactivateChild(child); return null; } + Element newChild; if (child != null) { bool hasSameSuperclass = true; // When the type of a widget is changed between Stateful and Stateless via @@ -3129,28 +3174,40 @@ abstract class Element extends DiagnosticableTree implements BuildContext { hasSameSuperclass = oldElementClass == newWidgetClass; return true; }()); - if (hasSameSuperclass) { - if (child.widget == newWidget) { - if (child.slot != newSlot) - updateSlotForChild(child, newSlot); - return child; - } - if (Widget.canUpdate(child.widget, newWidget)) { - if (child.slot != newSlot) - updateSlotForChild(child, newSlot); - child.update(newWidget); - assert(child.widget == newWidget); - assert(() { - child.owner._debugElementWasRebuilt(child); - return true; - }()); - return child; - } + if (hasSameSuperclass && child.widget == newWidget) { + if (child.slot != newSlot) + updateSlotForChild(child, newSlot); + newChild = child; + } else if (hasSameSuperclass && Widget.canUpdate(child.widget, newWidget)) { + if (child.slot != newSlot) + updateSlotForChild(child, newSlot); + child.update(newWidget); + assert(child.widget == newWidget); + assert(() { + child.owner._debugElementWasRebuilt(child); + return true; + }()); + newChild = child; + } else { + deactivateChild(child); + assert(child._parent == null); + newChild = inflateWidget(newWidget, newSlot); } - deactivateChild(child); - assert(child._parent == null); + } else { + newChild = inflateWidget(newWidget, newSlot); } - return inflateWidget(newWidget, newSlot); + + assert(() { + if (child != null) + _debugRemoveGlobalKeyReservation(child); + final Key key = newWidget?.key; + if (key is GlobalKey) { + key._debugReserveFor(this, newChild); + } + return true; + }()); + + return newChild; } /// Add this element to the tree in the given slot of the given parent. @@ -3188,6 +3245,9 @@ abstract class Element extends DiagnosticableTree implements BuildContext { }()); } + void _debugRemoveGlobalKeyReservation(Element child) { + GlobalKey._debugRemoveReservationFor(this, child); + } /// Change the widget used to configure this element. /// /// The framework calls this function when the parent wishes to use a @@ -3206,6 +3266,16 @@ abstract class Element extends DiagnosticableTree implements BuildContext { && depth != null && _active && Widget.canUpdate(widget, newWidget)); + // This Element was told to update and we can now release all the global key + // reservations of forgotten children. We cannot do this earlier because the + // forgotten children still represent global key duplications if the element + // never updates (the forgotten children are not removed from the tree + // until the call to update happens) + assert(() { + _debugForgottenChildrenWithGlobalKey.forEach(_debugRemoveGlobalKeyReservation); + _debugForgottenChildrenWithGlobalKey.clear(); + return true; + }()); _widget = newWidget; } @@ -3402,6 +3472,10 @@ abstract class Element extends DiagnosticableTree implements BuildContext { }()); } + // The children that have been forgotten by forgetChild. This will be used in + // [update] to remove the global key reservations of forgotten children. + final Set _debugForgottenChildrenWithGlobalKey = HashSet(); + /// Remove the given child from the element's child list, in preparation for /// the child being reused elsewhere in the element tree. /// @@ -3410,12 +3484,23 @@ abstract class Element extends DiagnosticableTree implements BuildContext { /// /// The element will still have a valid parent when this is called. After this /// is called, [deactivateChild] is called to sever the link to this object. + /// + /// The [update] is responsible for updating or creating the new child that + /// will replace this [child]. @protected + @mustCallSuper void forgetChild(Element child) { - // TODO(chunhtai): Creates empty body for subclass to call super. This will - // enable us to fix internal tests pro-actively for upcoming breaking - // change. - // https://github.com/flutter/flutter/issues/43780. + // This method is called on the old parent when the given child (with a + // global key) is given a new parent. We cannot remove the global key + // reservation directly in this method because the forgotten child is not + // removed from the tree until this Element is updated in [update]. If + // [update] is never called, the forgotten child still represents a global + // key duplication that we need to catch. + assert(() { + if (child.widget.key is GlobalKey) + _debugForgottenChildrenWithGlobalKey.add(child); + return true; + }()); } void _activateWithParent(Element parent, dynamic newSlot) { @@ -4445,6 +4530,7 @@ abstract class ComponentElement extends Element { void forgetChild(Element child) { assert(child == _child); _child = null; + super.forgetChild(child); } } @@ -5598,6 +5684,7 @@ class LeafRenderObjectElement extends RenderObjectElement { @override void forgetChild(Element child) { assert(false); + super.forgetChild(child); } @override @@ -5647,6 +5734,7 @@ class SingleChildRenderObjectElement extends RenderObjectElement { void forgetChild(Element child) { assert(child == _child); _child = null; + super.forgetChild(child); } @override @@ -5753,6 +5841,7 @@ class MultiChildRenderObjectElement extends RenderObjectElement { assert(_children.contains(child)); assert(!_forgottenChildren.contains(child)); _forgottenChildren.add(child); + super.forgetChild(child); } @override diff --git a/packages/flutter/lib/src/widgets/layout_builder.dart b/packages/flutter/lib/src/widgets/layout_builder.dart index 71382f24d1..77e6399eec 100644 --- a/packages/flutter/lib/src/widgets/layout_builder.dart +++ b/packages/flutter/lib/src/widgets/layout_builder.dart @@ -60,6 +60,7 @@ class _LayoutBuilderElement extends RenderOb void forgetChild(Element child) { assert(child == _child); _child = null; + super.forgetChild(child); } @override diff --git a/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart b/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart index bced3d08ac..40fdc2b0a2 100644 --- a/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart +++ b/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart @@ -924,6 +924,7 @@ class ListWheelElement extends RenderObjectElement implements ListWheelChildMana @override void forgetChild(Element child) { _childElements.remove(child.slot); + super.forgetChild(child); } } diff --git a/packages/flutter/lib/src/widgets/sliver.dart b/packages/flutter/lib/src/widgets/sliver.dart index 2f9a8e4aa6..8dd706cf48 100644 --- a/packages/flutter/lib/src/widgets/sliver.dart +++ b/packages/flutter/lib/src/widgets/sliver.dart @@ -1160,6 +1160,7 @@ class SliverMultiBoxAdaptorElement extends RenderObjectElement implements Render assert(child.slot != null); assert(_childElements.containsKey(child.slot)); _childElements.remove(child.slot); + super.forgetChild(child); } @override diff --git a/packages/flutter/lib/src/widgets/sliver_persistent_header.dart b/packages/flutter/lib/src/widgets/sliver_persistent_header.dart index 97f1a3d385..2be682eb96 100644 --- a/packages/flutter/lib/src/widgets/sliver_persistent_header.dart +++ b/packages/flutter/lib/src/widgets/sliver_persistent_header.dart @@ -230,6 +230,7 @@ class _SliverPersistentHeaderElement extends RenderObjectElement { void forgetChild(Element child) { assert(child == this.child); this.child = null; + super.forgetChild(child); } @override diff --git a/packages/flutter/lib/src/widgets/table.dart b/packages/flutter/lib/src/widgets/table.dart index 8f9f160c08..a6340505b8 100644 --- a/packages/flutter/lib/src/widgets/table.dart +++ b/packages/flutter/lib/src/widgets/table.dart @@ -344,6 +344,7 @@ class _TableElement extends RenderObjectElement { @override bool forgetChild(Element child) { _forgottenChildren.add(child); + super.forgetChild(child); return true; } } diff --git a/packages/flutter/test/foundation/diagnostics_json_test.dart b/packages/flutter/test/foundation/diagnostics_json_test.dart index 4a52f77cd8..b173f1cf54 100644 --- a/packages/flutter/test/foundation/diagnostics_json_test.dart +++ b/packages/flutter/test/foundation/diagnostics_json_test.dart @@ -221,11 +221,6 @@ void main() { class _TestElement extends Element { _TestElement() : super(const Placeholder()); - @override - void forgetChild(Element child) { - // Intentionally left empty. - } - @override void performRebuild() { // Intentionally left empty. diff --git a/packages/flutter/test/widgets/framework_test.dart b/packages/flutter/test/widgets/framework_test.dart index d70ede2ceb..1a5fba15c8 100644 --- a/packages/flutter/test/widgets/framework_test.dart +++ b/packages/flutter/test/widgets/framework_test.dart @@ -6,6 +6,8 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/widgets.dart'; +typedef ElementRebuildCallback = void Function(StatefulElement element); + class TestState extends State { @override Widget build(BuildContext context) => null; @@ -61,6 +63,358 @@ void main() { expect(keyA, isNot(equals(keyB))); }); + testWidgets('GlobalKey correct case 1 - can move global key from container widget to layoutbuilder', (WidgetTester tester) async { + final Key key = GlobalKey(debugLabel: 'correct'); + await tester.pumpWidget(Stack( + textDirection: TextDirection.ltr, + children: [ + Container( + key: const ValueKey(1), + child: SizedBox(key: key), + ), + LayoutBuilder( + key: const ValueKey(2), + builder: (BuildContext context, BoxConstraints constraints) { + return const Placeholder(); + }, + ), + ], + )); + + await tester.pumpWidget(Stack( + textDirection: TextDirection.ltr, + children: [ + Container( + key: const ValueKey(1), + child: const Placeholder(), + ), + LayoutBuilder( + key: const ValueKey(2), + builder: (BuildContext context, BoxConstraints constraints) { + return SizedBox(key: key); + }, + ), + ], + )); + }); + + testWidgets('GlobalKey correct case 2 - can move global key from layoutbuilder to container widget', (WidgetTester tester) async { + final Key key = GlobalKey(debugLabel: 'correct'); + await tester.pumpWidget(Stack( + textDirection: TextDirection.ltr, + children: [ + Container( + key: const ValueKey(1), + child: const Placeholder(), + ), + LayoutBuilder( + key: const ValueKey(2), + builder: (BuildContext context, BoxConstraints constraints) { + return SizedBox(key: key); + }, + ), + ], + )); + await tester.pumpWidget(Stack( + textDirection: TextDirection.ltr, + children: [ + Container( + key: const ValueKey(1), + child: SizedBox(key: key), + ), + LayoutBuilder( + key: const ValueKey(2), + builder: (BuildContext context, BoxConstraints constraints) { + return const Placeholder(); + }, + ), + ], + )); + }); + + testWidgets('GlobalKey correct case 3 - can deal with early rebuild in layoutbuilder - move backward', (WidgetTester tester) async { + const Key key1 = GlobalObjectKey('Text1'); + const Key key2 = GlobalObjectKey('Text2'); + Key rebuiltKeyOfSecondChildBeforeLayout; + Key rebuiltKeyOfFirstChildAfterLayout; + Key rebuiltKeyOfSecondChildAfterLayout; + await tester.pumpWidget( + LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return Column( + children: [ + const _Stateful( + child: Text( + 'Text1', + textDirection: TextDirection.ltr, + key: key1, + ), + ), + _Stateful( + child: const Text( + 'Text2', + textDirection: TextDirection.ltr, + key: key2, + ), + onElementRebuild: (StatefulElement element) { + // We don't want noise to override the result; + expect(rebuiltKeyOfSecondChildBeforeLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfSecondChildBeforeLayout = + statefulWidget.child.key; + }, + ), + ], + ); + }, + ) + ); + // Result will be written during first build and need to clear it to remove + // noise. + rebuiltKeyOfSecondChildBeforeLayout = null; + + final _StatefulState state = tester.firstState(find.byType(_Stateful).at(1)); + state.rebuild(); + // Reorders the items + await tester.pumpWidget( + LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return Column( + children: [ + _Stateful( + child: const Text( + 'Text2', + textDirection: TextDirection.ltr, + key: key2, + ), + onElementRebuild: (StatefulElement element) { + // Verifies the early rebuild happens before layout. + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + // We don't want noise to override the result; + expect(rebuiltKeyOfFirstChildAfterLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfFirstChildAfterLayout = statefulWidget.child.key; + }, + ), + _Stateful( + child: const Text( + 'Text1', + textDirection: TextDirection.ltr, + key: key1, + ), + onElementRebuild: (StatefulElement element) { + // Verifies the early rebuild happens before layout. + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + // We don't want noise to override the result; + expect(rebuiltKeyOfSecondChildAfterLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfSecondChildAfterLayout = statefulWidget.child.key; + }, + ), + ], + ); + }, + ) + ); + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + expect(rebuiltKeyOfFirstChildAfterLayout, key2); + expect(rebuiltKeyOfSecondChildAfterLayout, key1); + }); + + testWidgets('GlobalKey correct case 4 - can deal with early rebuild in layoutbuilder - move forward', (WidgetTester tester) async { + const Key key1 = GlobalObjectKey('Text1'); + const Key key2 = GlobalObjectKey('Text2'); + const Key key3 = GlobalObjectKey('Text3'); + Key rebuiltKeyOfSecondChildBeforeLayout; + Key rebuiltKeyOfSecondChildAfterLayout; + Key rebuiltKeyOfThirdChildAfterLayout; + await tester.pumpWidget( + LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return Column( + children: [ + const _Stateful( + child: Text( + 'Text1', + textDirection: TextDirection.ltr, + key: key1, + ), + ), + _Stateful( + child: const Text( + 'Text2', + textDirection: TextDirection.ltr, + key: key2, + ), + onElementRebuild: (StatefulElement element) { + // We don't want noise to override the result; + expect(rebuiltKeyOfSecondChildBeforeLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfSecondChildBeforeLayout = statefulWidget.child.key; + }, + ), + const _Stateful( + child: Text( + 'Text3', + textDirection: TextDirection.ltr, + key: key3, + ), + ), + ], + ); + }, + ) + ); + // Result will be written during first build and need to clear it to remove + // noise. + rebuiltKeyOfSecondChildBeforeLayout = null; + + final _StatefulState state = tester.firstState(find.byType(_Stateful).at(1)); + state.rebuild(); + // Reorders the items + await tester.pumpWidget( + LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return Column( + children: [ + const _Stateful( + child: Text( + 'Text1', + textDirection: TextDirection.ltr, + key: key1, + ), + ), + _Stateful( + child: const Text( + 'Text3', + textDirection: TextDirection.ltr, + key: key3, + ), + onElementRebuild: (StatefulElement element) { + // Verifies the early rebuild happens before layout. + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + // We don't want noise to override the result; + expect(rebuiltKeyOfSecondChildAfterLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfSecondChildAfterLayout = statefulWidget.child.key; + }, + ), + _Stateful( + child: const Text( + 'Text2', + textDirection: TextDirection.ltr, + key: key2, + ), + onElementRebuild: (StatefulElement element) { + // Verifies the early rebuild happens before layout. + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + // We don't want noise to override the result; + expect(rebuiltKeyOfThirdChildAfterLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfThirdChildAfterLayout = statefulWidget.child.key; + }, + ), + ], + ); + }, + ) + ); + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + expect(rebuiltKeyOfSecondChildAfterLayout, key3); + expect(rebuiltKeyOfThirdChildAfterLayout, key2); + }); + + testWidgets('GlobalKey correct case 5 - can deal with early rebuild in layoutbuilder - only one global key', (WidgetTester tester) async { + const Key key1 = GlobalObjectKey('Text1'); + Key rebuiltKeyOfSecondChildBeforeLayout; + Key rebuiltKeyOfThirdChildAfterLayout; + await tester.pumpWidget( + LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return Column( + children: [ + const _Stateful( + child: Text( + 'Text1', + textDirection: TextDirection.ltr, + ), + ), + _Stateful( + child: const Text( + 'Text2', + textDirection: TextDirection.ltr, + key: key1, + ), + onElementRebuild: (StatefulElement element) { + // We don't want noise to override the result; + expect(rebuiltKeyOfSecondChildBeforeLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfSecondChildBeforeLayout = statefulWidget.child.key; + }, + ), + const _Stateful( + child: Text( + 'Text3', + textDirection: TextDirection.ltr, + ), + ), + ], + ); + }, + ) + ); + // Result will be written during first build and need to clear it to remove + // noise. + rebuiltKeyOfSecondChildBeforeLayout = null; + + final _StatefulState state = tester.firstState(find.byType(_Stateful).at(1)); + state.rebuild(); + // Reorders the items + await tester.pumpWidget( + LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return Column( + children: [ + const _Stateful( + child: Text( + 'Text1', + textDirection: TextDirection.ltr, + ), + ), + _Stateful( + child: const Text( + 'Text3', + textDirection: TextDirection.ltr, + ), + onElementRebuild: (StatefulElement element) { + // Verifies the early rebuild happens before layout. + expect(rebuiltKeyOfSecondChildBeforeLayout, key1); + }, + ), + _Stateful( + child: const Text( + 'Text2', + textDirection: TextDirection.ltr, + key: key1, + ), + onElementRebuild: (StatefulElement element) { + // Verifies the early rebuild happens before layout. + expect(rebuiltKeyOfSecondChildBeforeLayout, key1); + // We don't want noise to override the result; + expect(rebuiltKeyOfThirdChildAfterLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfThirdChildAfterLayout = statefulWidget.child.key; + }, + ), + ], + ); + }, + ) + ); + expect(rebuiltKeyOfSecondChildBeforeLayout, key1); + expect(rebuiltKeyOfThirdChildAfterLayout, key1); + }); + testWidgets('GlobalKey duplication 1 - double appearance', (WidgetTester tester) async { final Key key = GlobalKey(debugLabel: 'problematic'); await tester.pumpWidget(Stack( @@ -501,7 +855,173 @@ void main() { ], )); FlutterError.onError = oldHandler; - expect(count, 2); + expect(count, 1); + }); + + testWidgets('GlobalKey duplication 18 - subtree build duplicate key with same type', (WidgetTester tester) async { + final Key key = GlobalKey(debugLabel: 'problematic'); + final Stack stack = Stack( + textDirection: TextDirection.ltr, + children: [ + const SwapKeyWidget(childKey: ValueKey(0)), + Container(key: const ValueKey(1)), + Container(key: key), + ], + ); + await tester.pumpWidget(stack); + final SwapKeyWidgetState state = tester.state(find.byType(SwapKeyWidget)); + state.swapKey(key); + await tester.pump(); + final dynamic exception = tester.takeException(); + expect(exception, isFlutterError); + expect( + exception.toString(), + equalsIgnoringHashCodes( + 'Duplicate GlobalKey detected in widget tree.\n' + 'The following GlobalKey was specified multiple times in the widget tree. This will lead ' + 'to parts of the widget tree being truncated unexpectedly, because the second time a key is seen, the ' + 'previous instance is moved to the new location. The key was:\n' + '- [GlobalKey#00000 problematic]\n' + 'This was determined by noticing that after the widget with the above global key was ' + 'moved out of its previous parent, that previous parent never updated during this frame, meaning that ' + 'it either did not update at all or updated before the widget was moved, in either case implying that ' + 'it still thinks that it should have a child with that global key.\n' + 'The specific parent that did not update after having one or more children forcibly ' + 'removed due to GlobalKey reparenting is:\n' + '- Stack(alignment: AlignmentDirectional.topStart, textDirection: ltr, fit: loose, ' + 'overflow: clip, renderObject: RenderStack#00000)\n' + 'A GlobalKey can only be specified on one widget at a time in the widget tree.' + ), + ); + }); + + testWidgets('GlobalKey duplication 19 - subtree build duplicate key with different types', (WidgetTester tester) async { + final Key key = GlobalKey(debugLabel: 'problematic'); + final Stack stack = Stack( + textDirection: TextDirection.ltr, + children: [ + const SwapKeyWidget(childKey: ValueKey(0)), + Container(key: const ValueKey(1)), + Container(child: SizedBox(key: key)), + ], + ); + await tester.pumpWidget(stack); + final SwapKeyWidgetState state = tester.state(find.byType(SwapKeyWidget)); + state.swapKey(key); + await tester.pump(); + final dynamic exception = tester.takeException(); + expect(exception, isFlutterError); + expect( + exception.toString(), + equalsIgnoringHashCodes( + 'Multiple widgets used the same GlobalKey.\n' + 'The key [GlobalKey#95367 problematic] was used by 2 widgets:\n' + ' SizedBox-[GlobalKey#00000 problematic]\n' + ' Container-[GlobalKey#00000 problematic]\n' + 'A GlobalKey can only be specified on one widget at a time in the widget tree.' + ), + ); + }); + + testWidgets('GlobalKey duplication 20 - real duplication with early rebuild in layoutbuilder will throw', (WidgetTester tester) async { + const Key key1 = GlobalObjectKey('Text1'); + const Key key2 = GlobalObjectKey('Text2'); + Key rebuiltKeyOfSecondChildBeforeLayout; + Key rebuiltKeyOfFirstChildAfterLayout; + Key rebuiltKeyOfSecondChildAfterLayout; + await tester.pumpWidget( + LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return Column( + children: [ + const _Stateful( + child: Text( + 'Text1', + textDirection: TextDirection.ltr, + key: key1, + ), + ), + _Stateful( + child: const Text( + 'Text2', + textDirection: TextDirection.ltr, + key: key2, + ), + onElementRebuild: (StatefulElement element) { + // We don't want noise to override the result; + expect(rebuiltKeyOfSecondChildBeforeLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfSecondChildBeforeLayout = statefulWidget.child.key; + }, + ), + ], + ); + }, + ) + ); + // Result will be written during first build and need to clear it to remove + // noise. + rebuiltKeyOfSecondChildBeforeLayout = null; + + final _StatefulState state = tester.firstState(find.byType(_Stateful).at(1)); + state.rebuild(); + + await tester.pumpWidget( + LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return Column( + children: [ + _Stateful( + child: const Text( + 'Text2', + textDirection: TextDirection.ltr, + key: key2, + ), + onElementRebuild: (StatefulElement element) { + // Verifies the early rebuild happens before layout. + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + // We don't want noise to override the result; + expect(rebuiltKeyOfFirstChildAfterLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfFirstChildAfterLayout = statefulWidget.child.key; + }, + ), + _Stateful( + child: const Text( + 'Text1', + textDirection: TextDirection.ltr, + key: key2, + ), + onElementRebuild: (StatefulElement element) { + // Verifies the early rebuild happens before layout. + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + // We don't want noise to override the result; + expect(rebuiltKeyOfSecondChildAfterLayout, isNull); + final _Stateful statefulWidget = element.widget as _Stateful; + rebuiltKeyOfSecondChildAfterLayout = statefulWidget.child.key; + }, + ), + ], + ); + }, + ) + ); + expect(rebuiltKeyOfSecondChildBeforeLayout, key2); + expect(rebuiltKeyOfFirstChildAfterLayout, key2); + expect(rebuiltKeyOfSecondChildAfterLayout, key2); + final dynamic exception = tester.takeException(); + expect(exception, isFlutterError); + expect( + exception.toString(), + equalsIgnoringHashCodes( + 'Multiple widgets used the same GlobalKey.\n' + 'The key [GlobalObjectKey String#00000] was used by multiple widgets. The ' + 'parents of those widgets were:\n' + '- _Stateful(state: _StatefulState#00000)\n' + '- _Stateful(state: _StatefulState#00000)\n' + 'A GlobalKey can only be specified on one widget at a time in the widget tree.' + ), + ); }); testWidgets('GlobalKey - dettach and re-attach child to different parents', (WidgetTester tester) async { @@ -758,9 +1278,6 @@ class NullChildElement extends Element { visitor(null); } - @override - void forgetChild(Element child) { } - @override void performRebuild() { } } @@ -772,9 +1289,6 @@ class DirtyElementWithCustomBuildOwner extends Element { final BuildOwner _owner; - @override - void forgetChild(Element child) {} - @override void performRebuild() {} @@ -823,3 +1337,66 @@ class DependentState extends State { deactivatedCount += 1; } } + +class SwapKeyWidget extends StatefulWidget { + const SwapKeyWidget({this.childKey}): super(); + + final Key childKey; + @override + SwapKeyWidgetState createState() => SwapKeyWidgetState(); +} + +class SwapKeyWidgetState extends State { + Key key; + + @override + void initState() { + super.initState(); + key = widget.childKey; + } + + void swapKey(Key newKey) { + setState(() { + key = newKey; + }); + } + + @override + Widget build(BuildContext context) { + return Container(key: key); + } +} + +class _Stateful extends StatefulWidget { + const _Stateful({Key key, this.child, this.onElementRebuild}) : super(key: key); + final Text child; + final ElementRebuildCallback onElementRebuild; + @override + State createState() => _StatefulState(); + + @override + StatefulElement createElement() => StatefulElementSpy(this); +} + +class _StatefulState extends State<_Stateful> { + void rebuild() => setState(() {}); + + @override + Widget build(BuildContext context) { + return widget.child; + } +} + +class StatefulElementSpy extends StatefulElement { + StatefulElementSpy(StatefulWidget widget) : super(widget); + + _Stateful get _statefulWidget => widget as _Stateful; + + @override + void rebuild() { + if (_statefulWidget.onElementRebuild != null) { + _statefulWidget.onElementRebuild(this); + } + super.rebuild(); + } +}