From 7aec9b4602ee9d6b13c6c7a5f70458df079ae9c1 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 29 Jan 2020 14:33:01 -0800 Subject: [PATCH] Avoid calling didChangeDependences on a State that has dropped out of the tree (#49527) --- .../lib/src/widgets/focus_manager.dart | 2 +- .../flutter/lib/src/widgets/focus_scope.dart | 7 ++ .../flutter/lib/src/widgets/framework.dart | 21 +++++- .../flutter/test/widgets/framework_test.dart | 68 +++++++++++++++++++ 4 files changed, 95 insertions(+), 3 deletions(-) diff --git a/packages/flutter/lib/src/widgets/focus_manager.dart b/packages/flutter/lib/src/widgets/focus_manager.dart index fd78e74fd7..7fc642a0bf 100644 --- a/packages/flutter/lib/src/widgets/focus_manager.dart +++ b/packages/flutter/lib/src/widgets/focus_manager.dart @@ -657,7 +657,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// Has no effect on nodes that return true from [hasFocus], but false from /// [hasPrimaryFocus]. /// - /// if [focusPrevious] is true, then rather than losing all focus, the focus + /// If [focusPrevious] is true, then rather than losing all focus, the focus /// will be moved to the node that the [enclosingScope] thinks should have it, /// based on its history of nodes that were set as first focus on it using /// [FocusScopeNode.setFirstFocus]. diff --git a/packages/flutter/lib/src/widgets/focus_scope.dart b/packages/flutter/lib/src/widgets/focus_scope.dart index b9f60779af..5c357e4f32 100644 --- a/packages/flutter/lib/src/widgets/focus_scope.dart +++ b/packages/flutter/lib/src/widgets/focus_scope.dart @@ -398,6 +398,13 @@ class _FocusState extends State { @override void deactivate() { super.deactivate(); + // The focus node's location in the tree is no longer valid here. But + // we can't unfocus or remove the node from the tree because if the widget + // is moved to a different part of the tree (via global key) it should + // retain its focus state. That's why we temporarily park it on the root + // focus node (via reparent) until it either gets moved to a different part + // of the tree (via didChangeDependencies) or until it is disposed. + _focusAttachment?.reparent(); _didAutofocus = false; } diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index f63eb78a43..8c432c67ae 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -4437,7 +4437,13 @@ class StatefulElement extends ComponentElement { } @override - Widget build() => state.build(this); + Widget build() { + if (_didChangeDependencies) { + _state.didChangeDependencies(); + _didChangeDependencies = false; + } + return state.build(this); + } /// The [State] instance associated with this location in the tree. /// @@ -4617,10 +4623,21 @@ class StatefulElement extends ComponentElement { return super.dependOnInheritedElement(ancestor as InheritedElement, aspect: aspect); } + /// This controls whether we should call [State.didChangeDependencies] from + /// the start of [build], to avoid calls when the [State] will not get built. + /// This can happen when the widget has dropped out of the tree, but depends + /// on an [InheritedWidget] that is still in the tree. + /// + /// It is set initially to false, since [_firstBuild] makes the initial call + /// on the [state]. When it is true, [build] will call + /// `state.didChangeDependencies` and then sets it to false. Subsequent calls + /// to [didChangeDependencies] set it to true. + bool _didChangeDependencies = false; + @override void didChangeDependencies() { super.didChangeDependencies(); - _state.didChangeDependencies(); + _didChangeDependencies = true; } @override diff --git a/packages/flutter/test/widgets/framework_test.dart b/packages/flutter/test/widgets/framework_test.dart index e00068004d..d70ede2ceb 100644 --- a/packages/flutter/test/widgets/framework_test.dart +++ b/packages/flutter/test/widgets/framework_test.dart @@ -710,6 +710,35 @@ void main() { ); } }); + + testWidgets('didUpdateDependencies is not called on a State that never rebuilds', (WidgetTester tester) async { + final GlobalKey key = GlobalKey(); + + /// Initial build - should call didChangeDependencies, not deactivate + await tester.pumpWidget(Inherited(1, child: DependentStatefulWidget(key: key))); + final DependentState state = key.currentState; + expect(key.currentState, isNotNull); + expect(state.didChangeDependenciesCount, 1); + expect(state.deactivatedCount, 0); + + /// Rebuild with updated value - should call didChangeDependencies + await tester.pumpWidget(Inherited(2, child: DependentStatefulWidget(key: key))); + expect(key.currentState, isNotNull); + expect(state.didChangeDependenciesCount, 2); + expect(state.deactivatedCount, 0); + + // reparent it - should call deactivate and didChangeDependencies + await tester.pumpWidget(Inherited(3, child: SizedBox(child: DependentStatefulWidget(key: key)))); + expect(key.currentState, isNotNull); + expect(state.didChangeDependenciesCount, 3); + expect(state.deactivatedCount, 1); + + // Remove it - should call deactivate, but not didChangeDependencies + await tester.pumpWidget(const Inherited(4, child: SizedBox())); + expect(key.currentState, isNull); + expect(state.didChangeDependenciesCount, 3); + expect(state.deactivatedCount, 2); + }); } class NullChildTest extends Widget { @@ -755,3 +784,42 @@ class DirtyElementWithCustomBuildOwner extends Element { @override bool get dirty => true; } + +class Inherited extends InheritedWidget { + const Inherited(this.value, {Widget child, Key key}) : super(key: key, child: child); + + final int value; + + @override + bool updateShouldNotify(Inherited oldWidget) => oldWidget.value != value; +} + +class DependentStatefulWidget extends StatefulWidget { + const DependentStatefulWidget({Key key}) : super(key: key); + + @override + State createState() => DependentState(); +} + +class DependentState extends State { + int didChangeDependenciesCount = 0; + int deactivatedCount = 0; + + @override + void didChangeDependencies() { + super.didChangeDependencies(); + didChangeDependenciesCount += 1; + } + + @override + Widget build(BuildContext context) { + context.dependOnInheritedWidgetOfExactType(); + return const SizedBox(); + } + + @override + void deactivate() { + super.deactivate(); + deactivatedCount += 1; + } +}