From fbf8174c87eb4cab85d0ebf43f672cb8493c63da Mon Sep 17 00:00:00 2001 From: Hixie Date: Mon, 5 Oct 2015 15:52:36 -0700 Subject: [PATCH] Fix Focus Focus.at() and company should be on Focus, not FocusState. _notifyDescendants() was using the wrong runtimeType. Let InheritedWidget update the descendants during build. When you setState() during build, assert that you're not markNeedsBuild()ing someone who isn't a descendant. Typo in Widget.toString(). --- examples/address_book/lib/main.dart | 14 ++- packages/flutter/lib/src/widgets/binding.dart | 19 +++- packages/flutter/lib/src/widgets/focus.dart | 96 +++++++++---------- .../flutter/lib/src/widgets/framework.dart | 67 +++++++++++-- packages/flutter/lib/src/widgets/input.dart | 6 +- .../unit/test/widget/build_scope_test.dart | 4 + packages/unit/test/widget/focus_test.dart | 63 ++++++++++++ 7 files changed, 195 insertions(+), 74 deletions(-) create mode 100644 packages/unit/test/widget/focus_test.dart diff --git a/examples/address_book/lib/main.dart b/examples/address_book/lib/main.dart index 2bca9be10fb..26c0e4aefaf 100644 --- a/examples/address_book/lib/main.dart +++ b/examples/address_book/lib/main.dart @@ -53,14 +53,12 @@ class AddressBookHome extends StatelessComponent { ); } - static final GlobalKey nameKey = new GlobalKey(); - static final GlobalKey phoneKey = new GlobalKey(); - static final GlobalKey emailKey = new GlobalKey(); - static final GlobalKey addressKey = new GlobalKey(); - static final GlobalKey ringtoneKey = new GlobalKey(); - static final GlobalKey noteKey = new GlobalKey(); - static final GlobalKey fillKey = new GlobalKey(); - static final GlobalKey emoticonKey = new GlobalKey(); + static final GlobalKey nameKey = new GlobalKey(label: 'name field'); + static final GlobalKey phoneKey = new GlobalKey(label: 'phone field'); + static final GlobalKey emailKey = new GlobalKey(label: 'email field'); + static final GlobalKey addressKey = new GlobalKey(label: 'address field'); + static final GlobalKey ringtoneKey = new GlobalKey(label: 'ringtone field'); + static final GlobalKey noteKey = new GlobalKey(label: 'note field'); Widget buildBody(BuildContext context) { return new Material( diff --git a/packages/flutter/lib/src/widgets/binding.dart b/packages/flutter/lib/src/widgets/binding.dart index 59056dfd4f8..2699254b6aa 100644 --- a/packages/flutter/lib/src/widgets/binding.dart +++ b/packages/flutter/lib/src/widgets/binding.dart @@ -41,7 +41,7 @@ class WidgetFlutterBinding extends FlutterBinding { Element.finalizeTree(); } - List _dirtyElements = new List(); + List _dirtyElements = []; /// Adds an element to the dirty elements list so that it will be rebuilt /// when buildDirtyElements is called. @@ -62,10 +62,19 @@ class WidgetFlutterBinding extends FlutterBinding { return; BuildableElement.lockState(() { _dirtyElements.sort((BuildableElement a, BuildableElement b) => a.depth - b.depth); - for (BuildableElement element in _dirtyElements) - element.rebuild(); + int dirtyCount = _dirtyElements.length; + int index = 0; + while (index < dirtyCount) { + _dirtyElements[index].rebuild(); + index += 1; + if (dirtyCount < _dirtyElements.length) { + _dirtyElements.sort((BuildableElement a, BuildableElement b) => a.depth - b.depth); + dirtyCount = _dirtyElements.length; + } + } + assert(!_dirtyElements.any((BuildableElement element) => element.dirty)); _dirtyElements.clear(); - }); + }, building: true); assert(_dirtyElements.isEmpty); } } @@ -76,7 +85,7 @@ void runApp(Widget app) { WidgetFlutterBinding.instance.renderViewElement.update( WidgetFlutterBinding.instance.describeApp(app) ); - }); + }, building: true); } void debugDumpApp() { diff --git a/packages/flutter/lib/src/widgets/focus.dart b/packages/flutter/lib/src/widgets/focus.dart index 478c77173ed..098fcf954e4 100644 --- a/packages/flutter/lib/src/widgets/focus.dart +++ b/packages/flutter/lib/src/widgets/focus.dart @@ -75,6 +75,52 @@ class Focus extends StatefulComponent { final bool autofocus; final Widget child; + static bool at(BuildContext context, Widget widget, { bool autofocus: true }) { + assert(widget != null); + assert(widget.key is GlobalKey); + _FocusScope focusScope = context.inheritedWidgetOfType(_FocusScope); + if (focusScope != null) { + if (autofocus) + focusScope._setFocusedWidgetIfUnset(widget.key); + return focusScope.scopeFocused && + focusScope.focusedScope == null && + focusScope.focusedWidget == widget.key; + } + return true; + } + + static bool _atScope(BuildContext context, Widget widget, { bool autofocus: true }) { + assert(widget != null); + _FocusScope focusScope = context.inheritedWidgetOfType(_FocusScope); + if (focusScope != null) { + if (autofocus) + focusScope._setFocusedScopeIfUnset(widget.key); + assert(widget.key != null); + return focusScope.scopeFocused && + focusScope.focusedScope == widget.key; + } + return true; + } + + // Don't call moveTo() from your build() function, it's intended to be called + // from event listeners, e.g. in response to a finger tap or tab key. + + static void moveTo(BuildContext context, Widget widget) { + assert(widget != null); + assert(widget.key is GlobalKey); + _FocusScope focusScope = context.inheritedWidgetOfType(_FocusScope); + if (focusScope != null) + focusScope.focusState._setFocusedWidget(widget.key); + } + + static void _moveScopeTo(BuildContext context, Focus component) { + assert(component != null); + assert(component.key != null); + _FocusScope focusScope = context.inheritedWidgetOfType(_FocusScope); + if (focusScope != null) + focusScope.focusState._setFocusedScope(component.key); + } + FocusState createState() => new FocusState(); } @@ -155,7 +201,7 @@ class FocusState extends State { void initState() { super.initState(); if (config.autofocus) - FocusState._moveScopeTo(context, config); + Focus._moveScopeTo(context, config); _updateWidgetRemovalListener(_focusedWidget); _updateScopeRemovalListener(_focusedScope); } @@ -169,56 +215,10 @@ class FocusState extends State { Widget build(BuildContext context) { return new _FocusScope( focusState: this, - scopeFocused: FocusState._atScope(context, config), + scopeFocused: Focus._atScope(context, config), focusedScope: _focusedScope == _noFocusedScope ? null : _focusedScope, focusedWidget: _focusedWidget, child: config.child ); } - - static bool at(BuildContext context, Widget widget, { bool autofocus: true }) { - assert(widget != null); - assert(widget.key is GlobalKey); - _FocusScope focusScope = context.inheritedWidgetOfType(_FocusScope); - if (focusScope != null) { - if (autofocus) - focusScope._setFocusedWidgetIfUnset(widget.key); - return focusScope.scopeFocused && - focusScope.focusedScope == null && - focusScope.focusedWidget == widget.key; - } - return true; - } - - static bool _atScope(BuildContext context, Widget widget, { bool autofocus: true }) { - assert(widget != null); - _FocusScope focusScope = context.inheritedWidgetOfType(_FocusScope); - if (focusScope != null) { - if (autofocus) - focusScope._setFocusedScopeIfUnset(widget.key); - assert(widget.key != null); - return focusScope.scopeFocused && - focusScope.focusedScope == widget.key; - } - return true; - } - - // Don't call moveTo() from your build() function, it's intended to be called - // from event listeners, e.g. in response to a finger tap or tab key. - - static void moveTo(BuildContext context, Widget widget) { - assert(widget != null); - assert(widget.key is GlobalKey); - _FocusScope focusScope = context.inheritedWidgetOfType(_FocusScope); - if (focusScope != null) - focusScope.focusState._setFocusedWidget(widget.key); - } - - static void _moveScopeTo(BuildContext context, Focus component) { - assert(component != null); - assert(component.key != null); - _FocusScope focusScope = context.inheritedWidgetOfType(_FocusScope); - if (focusScope != null) - focusScope.focusState._setFocusedScope(component.key); - } } diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index 05ce1e691ca..16c63fc4234 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -186,8 +186,8 @@ abstract class Widget { final List data = []; debugFillDescription(data); if (data.isEmpty) - return 'name'; - return 'name(${data.join("; ")})'; + return '$name'; + return '$name(${data.join("; ")})'; } void debugFillDescription(List description) { } @@ -550,7 +550,7 @@ abstract class Element implements BuildContext { /// Wrapper around visitChildren for BuildContext. void visitChildElements(void visitor(Element element)) { // don't allow visitChildElements() during build, since children aren't necessarily built yet - assert(BuildableElement._debugStateLockLevel == 0); + assert(!BuildableElement._debugStateLocked); visitChildren(visitor); } @@ -858,6 +858,8 @@ abstract class BuildableElement extends Element { assert(_child != null); } + static BuildableElement _debugCurrentBuildTarget; + /// Reinvokes the build() method of the StatelessComponent object (for /// stateless components) or the State object (for stateful components) and /// then updates the widget tree. @@ -874,6 +876,12 @@ abstract class BuildableElement extends Element { assert(_debugLifecycleState == _ElementLifecycle.active); assert(_debugStateLocked); assert(_debugSetAllowIgnoredCallsToMarkNeedsBuild(true)); + BuildableElement debugPreviousBuildTarget; + assert(() { + debugPreviousBuildTarget = _debugCurrentBuildTarget; + _debugCurrentBuildTarget = this; + return true; + }); Widget built; try { built = _builder(this); @@ -896,12 +904,19 @@ abstract class BuildableElement extends Element { built = new ErrorWidget(); _child = updateChild(null, built, slot); } + + assert(() { + assert(_debugCurrentBuildTarget == this); + _debugCurrentBuildTarget = debugPreviousBuildTarget; + return true; + }); } static BuildScheduler scheduleBuildFor; static int _debugStateLockLevel = 0; static bool get _debugStateLocked => _debugStateLockLevel > 0; + static bool _debugBuilding = false; /// Establishes a scope in which component build functions can run. /// @@ -913,13 +928,31 @@ abstract class BuildableElement extends Element { /// After unwinding the last build scope on the stack, the framework verifies /// that each global key is used at most once and notifies listeners about /// changes to global keys. - static void lockState(void callback()) { - _debugStateLockLevel += 1; + static void lockState(void callback(), { bool building: false }) { + assert(_debugStateLockLevel >= 0); + assert(() { + if (building) { + assert(!_debugBuilding); + assert(_debugCurrentBuildTarget == null); + _debugBuilding = true; + } + _debugStateLockLevel += 1; + return true; + }); try { callback(); } finally { - _debugStateLockLevel -= 1; + assert(() { + _debugStateLockLevel -= 1; + if (building) { + assert(_debugBuilding); + assert(_debugCurrentBuildTarget == null); + _debugBuilding = false; + } + return true; + }); } + assert(_debugStateLockLevel >= 0); } /// Marks the element as dirty and adds it to the global list of widgets to @@ -934,10 +967,23 @@ abstract class BuildableElement extends Element { if (!_active) return; assert(_debugLifecycleState == _ElementLifecycle.active); - assert(!_debugStateLocked || (_debugAllowIgnoredCallsToMarkNeedsBuild && dirty)); + assert(() { + if (_debugBuilding) { + bool foundTarget = false; + visitAncestorElements((Element element) { + if (element == _debugCurrentBuildTarget) { + foundTarget = true; + return false; + } + return true; + }); + if (foundTarget) + return true; + } + return !_debugStateLocked || (_debugAllowIgnoredCallsToMarkNeedsBuild && dirty); + }); if (dirty) return; - assert(!_debugStateLocked); _dirty = true; assert(scheduleBuildFor != null); scheduleBuildFor(this); @@ -1101,11 +1147,12 @@ class InheritedElement extends StatelessComponentElement { } void _notifyDescendants() { - final Type ourRuntimeType = runtimeType; + final Type ourRuntimeType = widget.runtimeType; void notifyChildren(Element child) { if (child._dependencies != null && - child._dependencies.contains(ourRuntimeType)) + child._dependencies.contains(ourRuntimeType)) { child.dependenciesChanged(); + } if (child.runtimeType != ourRuntimeType) child.visitChildren(notifyChildren); } diff --git a/packages/flutter/lib/src/widgets/input.dart b/packages/flutter/lib/src/widgets/input.dart index d892944b1db..5ec60a81e13 100644 --- a/packages/flutter/lib/src/widgets/input.dart +++ b/packages/flutter/lib/src/widgets/input.dart @@ -71,7 +71,7 @@ class InputState extends ScrollableState { Widget buildContent(BuildContext context) { ThemeData themeData = Theme.of(context); - bool focused = FocusState.at(context, config); + bool focused = Focus.at(context, config); if (focused && !_keyboardHandle.attached) { _keyboardHandle = keyboard.show(_editableValue.stub, config.keyboardType); @@ -122,11 +122,11 @@ class InputState extends ScrollableState { ) ), onPointerDown: (_) { - if (FocusState.at(context, config)) { + if (Focus.at(context, config)) { assert(_keyboardHandle.attached); _keyboardHandle.showByRequest(); } else { - FocusState.moveTo(context, config); + Focus.moveTo(context, config); // we'll get told to rebuild and we'll take care of the keyboard then } } diff --git a/packages/unit/test/widget/build_scope_test.dart b/packages/unit/test/widget/build_scope_test.dart index 927c1e46a9a..d54c4f534bc 100644 --- a/packages/unit/test/widget/build_scope_test.dart +++ b/packages/unit/test/widget/build_scope_test.dart @@ -66,6 +66,10 @@ class BadDisposeWidgetState extends State { void main() { dynamic cachedException; + // ** WARNING ** + // THIS TEST OVERRIDES THE NORMAL EXCEPTION HANDLING + // AND DOES NOT REPORT EXCEPTIONS FROM THE FRAMEWORK + setUp(() { assert(cachedException == null); debugWidgetsExceptionHandler = (String context, dynamic exception, StackTrace stack) { diff --git a/packages/unit/test/widget/focus_test.dart b/packages/unit/test/widget/focus_test.dart new file mode 100644 index 00000000000..8806e6e30b8 --- /dev/null +++ b/packages/unit/test/widget/focus_test.dart @@ -0,0 +1,63 @@ +import 'package:sky/widgets.dart'; +import 'package:test/test.dart'; + +import 'widget_tester.dart'; + +class TestFocusable extends StatelessComponent { + TestFocusable(this.no, this.yes, GlobalKey key) : super(key: key); + final String no; + final String yes; + Widget build(BuildContext context) { + bool focused = Focus.at(context, this); + return new GestureDetector( + onTap: () { Focus.moveTo(context, this); }, + child: new Text(focused ? yes : no) + ); + } +} + +void main() { + test('Can have multiple focused children and they update accordingly', () { + testWidgets((WidgetTester tester) { + GlobalKey keyA = new GlobalKey(); + GlobalKey keyB = new GlobalKey(); + tester.pumpWidget( + new Focus( + child: new Column([ + // reverse these when you fix https://github.com/flutter/engine/issues/1495 + new TestFocusable('b', 'B FOCUSED', keyB), + new TestFocusable('a', 'A FOCUSED', keyA), + ]) + ) + ); + expect(tester.findText('a'), isNull); + expect(tester.findText('A FOCUSED'), isNotNull); + expect(tester.findText('b'), isNotNull); + expect(tester.findText('B FOCUSED'), isNull); + tester.tap(tester.findText('A FOCUSED')); + tester.pump(); + expect(tester.findText('a'), isNull); + expect(tester.findText('A FOCUSED'), isNotNull); + expect(tester.findText('b'), isNotNull); + expect(tester.findText('B FOCUSED'), isNull); + tester.tap(tester.findText('A FOCUSED')); + tester.pump(); + expect(tester.findText('a'), isNull); + expect(tester.findText('A FOCUSED'), isNotNull); + expect(tester.findText('b'), isNotNull); + expect(tester.findText('B FOCUSED'), isNull); + tester.tap(tester.findText('b')); + tester.pump(); + expect(tester.findText('a'), isNotNull); + expect(tester.findText('A FOCUSED'), isNull); + expect(tester.findText('b'), isNull); + expect(tester.findText('B FOCUSED'), isNotNull); + tester.tap(tester.findText('a')); + tester.pump(); + expect(tester.findText('a'), isNull); + expect(tester.findText('A FOCUSED'), isNotNull); + expect(tester.findText('b'), isNotNull); + expect(tester.findText('B FOCUSED'), isNull); + }); + }); +}