Unverified Commit 835c7fab authored by Todd Volkert's avatar Todd Volkert Committed by GitHub

Fix FocusManager constructor (#75894)

The FocusManager constructor was registering global event handlers
on the shared RawKeyboard instance and the global pointer router.
This posed a few problems: (1) there was no way to unregister these
handlers, and (2) instantiating a second FocusManager would overwrite
the existing focus manager's RawKeyboard handler. This was manifesting
in unexpected ways, such as the fact that constructing a second
BuildOwner (for a parallel tree, for instance) was obliterating the
event handler for the main BuildOwner's focus manager, thus messing
with focus.

This change separates those global event registrations into a
dedicated method, registerGlobalHandlers(), and overrides dispose()
to properly unregister those handlers.
parent db038a1d
...@@ -33,7 +33,6 @@ class Rectangle extends StatelessWidget { ...@@ -33,7 +33,6 @@ class Rectangle extends StatelessWidget {
double? value; double? value;
RenderObjectToWidgetElement<RenderBox>? element; RenderObjectToWidgetElement<RenderBox>? element;
BuildOwner owner = BuildOwner();
void attachWidgetTreeToRenderTree(RenderProxyBox container) { void attachWidgetTreeToRenderTree(RenderProxyBox container) {
element = RenderObjectToWidgetAdapter<RenderBox>( element = RenderObjectToWidgetAdapter<RenderBox>(
container: container, container: container,
...@@ -74,7 +73,7 @@ void attachWidgetTreeToRenderTree(RenderProxyBox container) { ...@@ -74,7 +73,7 @@ void attachWidgetTreeToRenderTree(RenderProxyBox container) {
), ),
), ),
), ),
).attachToRenderTree(owner, element); ).attachToRenderTree(WidgetsBinding.instance!.buildOwner!, element);
} }
Duration? timeBase; Duration? timeBase;
...@@ -87,7 +86,7 @@ void rotate(Duration timeStamp) { ...@@ -87,7 +86,7 @@ void rotate(Duration timeStamp) {
transformBox.setIdentity(); transformBox.setIdentity();
transformBox.rotateZ(delta); transformBox.rotateZ(delta);
owner.buildScope(element!); WidgetsBinding.instance!.buildOwner!.buildScope(element!);
} }
void main() { void main() {
......
...@@ -1439,12 +1439,39 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -1439,12 +1439,39 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
/// This constructor is rarely called directly. To access the [FocusManager], /// This constructor is rarely called directly. To access the [FocusManager],
/// consider using the [FocusManager.instance] accessor instead (which gets it /// consider using the [FocusManager.instance] accessor instead (which gets it
/// from the [WidgetsBinding] singleton). /// from the [WidgetsBinding] singleton).
///
/// This newly constructed focus manager does not have the necessary event
/// handlers registered to allow it to manage focus. To register those event
/// handlers, callers must call [registerGlobalHandlers]. See the
/// documentation in that method for caveats to watch out for.
FocusManager() { FocusManager() {
rootScope._manager = this; rootScope._manager = this;
}
/// Registers global input event handlers that are needed to manage focus.
///
/// This sets the [RawKeyboard.keyEventHandler] for the shared instance of
/// [RawKeyboard] and adds a route to the global entry in the gesture routing
/// table. As such, only one [FocusManager] instance should register its
/// global handlers.
///
/// When this focus manager is no longer needed, calling [dispose] on it will
/// unregister these handlers.
void registerGlobalHandlers() {
assert(RawKeyboard.instance.keyEventHandler == null);
RawKeyboard.instance.keyEventHandler = _handleRawKeyEvent; RawKeyboard.instance.keyEventHandler = _handleRawKeyEvent;
GestureBinding.instance!.pointerRouter.addGlobalRoute(_handlePointerEvent); GestureBinding.instance!.pointerRouter.addGlobalRoute(_handlePointerEvent);
} }
@override
void dispose() {
if (RawKeyboard.instance.keyEventHandler == _handleRawKeyEvent) {
RawKeyboard.instance.keyEventHandler = null;
GestureBinding.instance!.pointerRouter.removeGlobalRoute(_handlePointerEvent);
}
super.dispose();
}
/// Provides convenient access to the current [FocusManager] singleton from /// Provides convenient access to the current [FocusManager] singleton from
/// the [WidgetsBinding] instance. /// the [WidgetsBinding] instance.
static FocusManager get instance => WidgetsBinding.instance!.focusManager; static FocusManager get instance => WidgetsBinding.instance!.focusManager;
......
...@@ -2327,7 +2327,7 @@ abstract class BuildContext { ...@@ -2327,7 +2327,7 @@ abstract class BuildContext {
/// Size measureWidget(Widget widget) { /// Size measureWidget(Widget widget) {
/// final PipelineOwner pipelineOwner = PipelineOwner(); /// final PipelineOwner pipelineOwner = PipelineOwner();
/// final MeasurementView rootView = pipelineOwner.rootNode = MeasurementView(); /// final MeasurementView rootView = pipelineOwner.rootNode = MeasurementView();
/// final BuildOwner buildOwner = BuildOwner(focusManager: FailingFocusManager()); /// final BuildOwner buildOwner = BuildOwner(focusManager: FocusManager());
/// final RenderObjectToWidgetElement<RenderBox> element = RenderObjectToWidgetAdapter<RenderBox>( /// final RenderObjectToWidgetElement<RenderBox> element = RenderObjectToWidgetAdapter<RenderBox>(
/// container: rootView, /// container: rootView,
/// debugShortDescription: '[root]', /// debugShortDescription: '[root]',
...@@ -2344,17 +2344,6 @@ abstract class BuildContext { ...@@ -2344,17 +2344,6 @@ abstract class BuildContext {
/// } /// }
/// } /// }
/// ///
/// // The default FocusManager, when created, modifies some static properties
/// // that we don't want to modify, which is why we use a failing implementation
/// // here.
/// class FailingFocusManager implements FocusManager {
/// @override
/// dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
///
/// @override
/// String toString({ DiagnosticLevel minLevel = DiagnosticLevel.info }) => 'FailingFocusManager';
/// }
///
/// class MeasurementView extends RenderBox with RenderObjectWithChildMixin<RenderBox> { /// class MeasurementView extends RenderBox with RenderObjectWithChildMixin<RenderBox> {
/// @override /// @override
/// void performLayout() { /// void performLayout() {
...@@ -2370,8 +2359,14 @@ abstract class BuildContext { ...@@ -2370,8 +2359,14 @@ abstract class BuildContext {
/// {@end-tool} /// {@end-tool}
class BuildOwner { class BuildOwner {
/// Creates an object that manages widgets. /// Creates an object that manages widgets.
///
/// If the `focusManager` argument is not specified or is null, this will
/// construct a new [FocusManager] and register its global input handlers
/// via [FocusManager.registerGlobalHandlers], which will modify static
/// state. Callers wishing to avoid altering this state can explicitly pass
/// a focus manager here.
BuildOwner({ this.onBuildScheduled, FocusManager? focusManager }) : BuildOwner({ this.onBuildScheduled, FocusManager? focusManager }) :
focusManager = focusManager ?? FocusManager(); focusManager = focusManager ?? (FocusManager()..registerGlobalHandlers());
/// Called on each build pass when the first buildable element is marked /// Called on each build pass when the first buildable element is marked
/// dirty. /// dirty.
...@@ -2402,6 +2397,12 @@ class BuildOwner { ...@@ -2402,6 +2397,12 @@ class BuildOwner {
/// the [FocusScopeNode] for a given [BuildContext]. /// the [FocusScopeNode] for a given [BuildContext].
/// ///
/// See [FocusManager] for more details. /// See [FocusManager] for more details.
///
/// This field will default to a [FocusManager] that has registered its
/// global input handlers via [FocusManager.registerGlobalHandlers]. Callers
/// wishing to avoid registering those handlers (and modifying the associated
/// static state) can explicitly pass a focus manager to the [new BuildOwner]
/// constructor.
FocusManager focusManager; FocusManager focusManager;
/// Adds an element to the dirty elements list so that it will be rebuilt /// Adds an element to the dirty elements list so that it will be rebuilt
......
...@@ -1490,7 +1490,7 @@ void main() { ...@@ -1490,7 +1490,7 @@ void main() {
final int pointerRouterCount = GestureBinding.instance!.pointerRouter.debugGlobalRouteCount; final int pointerRouterCount = GestureBinding.instance!.pointerRouter.debugGlobalRouteCount;
final RawKeyEventHandler? rawKeyEventHandler = RawKeyboard.instance.keyEventHandler; final RawKeyEventHandler? rawKeyEventHandler = RawKeyboard.instance.keyEventHandler;
expect(rawKeyEventHandler, isNotNull); expect(rawKeyEventHandler, isNotNull);
BuildOwner(focusManager: _FakeFocusManager()); BuildOwner(focusManager: FocusManager());
expect(GestureBinding.instance!.pointerRouter.debugGlobalRouteCount, pointerRouterCount); expect(GestureBinding.instance!.pointerRouter.debugGlobalRouteCount, pointerRouterCount);
expect(RawKeyboard.instance.keyEventHandler, same(rawKeyEventHandler)); expect(RawKeyboard.instance.keyEventHandler, same(rawKeyEventHandler));
}); });
...@@ -1516,18 +1516,6 @@ void main() { ...@@ -1516,18 +1516,6 @@ void main() {
}); });
} }
class _FakeFocusManager implements FocusManager {
@override
dynamic noSuchMethod(Invocation invocation) {
return super.noSuchMethod(invocation);
}
@override
String toString({ DiagnosticLevel minLevel = DiagnosticLevel.info }) {
return '_FakeFocusManager';
}
}
class _WidgetWithNoVisitChildren extends StatelessWidget { class _WidgetWithNoVisitChildren extends StatelessWidget {
const _WidgetWithNoVisitChildren(this.child, { Key? key }) : const _WidgetWithNoVisitChildren(this.child, { Key? key }) :
super(key: key); super(key: key);
......
...@@ -54,7 +54,7 @@ class OffscreenWidgetTree { ...@@ -54,7 +54,7 @@ class OffscreenWidgetTree {
} }
final RenderView renderView = OffscreenRenderView(); final RenderView renderView = OffscreenRenderView();
final BuildOwner buildOwner = BuildOwner(); final BuildOwner buildOwner = BuildOwner(focusManager: FocusManager());
final PipelineOwner pipelineOwner = PipelineOwner(); final PipelineOwner pipelineOwner = PipelineOwner();
RenderObjectToWidgetElement<RenderBox>? root; RenderObjectToWidgetElement<RenderBox>? root;
......
...@@ -897,7 +897,8 @@ abstract class TestWidgetsFlutterBinding extends BindingBase ...@@ -897,7 +897,8 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
FlutterError.demangleStackTrace = _oldStackTraceDemangler; FlutterError.demangleStackTrace = _oldStackTraceDemangler;
_pendingExceptionDetails = null; _pendingExceptionDetails = null;
_parentZone = null; _parentZone = null;
buildOwner!.focusManager = FocusManager(); buildOwner!.focusManager.dispose();
buildOwner!.focusManager = FocusManager()..registerGlobalHandlers();
// Disabling the warning because @visibleForTesting doesn't take the testing // Disabling the warning because @visibleForTesting doesn't take the testing
// framework itself into account, but we don't want it visible outside of // framework itself into account, but we don't want it visible outside of
// tests. // tests.
......
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