From b78aaca24ce7b87f61e6435d2c0b8af7b0a3e767 Mon Sep 17 00:00:00 2001 From: Juanjo Tugores Date: Fri, 1 Nov 2024 17:51:13 -0700 Subject: [PATCH] [web] Transfer focus to view rootElement on blur/remove. (flutter/engine#55045) The `safeBlur`/`safeRemove`/`safeRemoveSync` methods in the view manager should become the standard way to "blur" and "remove" elements within the web engine. Calling these method ensures the blur operation doesn't disrupt the framework's view focus management because these methods transfer the focus from the current element to its containing EngineFlutterView's `rootElement`, so focus never abandons the Flutter view unless the user wants to. This is a generalization of the former `DefaultTextEditingStrategy.scheduleFocusFlutterView`, which turns out is needed in anything that touches elements that may receive focus in the engine, not just text editing. ## Issue (Maybe) Part of https://github.com/flutter/flutter/issues/157387 (Opportunistically) Fixes https://github.com/flutter/flutter/issues/46638 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../lib/src/engine/semantics/semantics.dart | 4 +- .../lib/src/engine/semantics/text_field.dart | 11 +-- .../src/engine/text_editing/text_editing.dart | 27 +----- .../view_embedder/flutter_view_manager.dart | 83 ++++++++++++++++++- .../test/engine/semantics/semantics_test.dart | 5 +- .../engine/semantics/text_field_test.dart | 18 ++-- .../engine/surface/scene_builder_test.dart | 4 +- 7 files changed, 102 insertions(+), 50 deletions(-) diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics.dart index ea31b6356e..b3f59d1292 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -1987,7 +1987,9 @@ class SemanticsObject { void dispose() { assert(!_isDisposed); _isDisposed = true; - element.remove(); + + EnginePlatformDispatcher.instance.viewManager.safeRemoveSync(element); + _parent = null; semanticRole?.dispose(); semanticRole = null; diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/text_field.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/text_field.dart index 1ad9cecf83..0bf734b079 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/text_field.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/text_field.dart @@ -114,16 +114,7 @@ class SemanticsTextEditingStrategy extends DefaultTextEditingStrategy { } subscriptions.clear(); lastEditingState = null; - - // If the text element still has focus, remove focus from the editable - // element to cause the on-screen keyboard, if any, to hide (e.g. on iOS, - // Android). - // Otherwise, the keyboard stays on screen even when the user navigates to - // a different screen (e.g. by hitting the "back" button). - // Keep this consistent with how DefaultTextEditingStrategy does it. As of - // right now, the only difference is that semantic text fields do not - // participate in form autofill. - DefaultTextEditingStrategy.scheduleFocusFlutterView(activeDomElement, activeDomElementView); + EnginePlatformDispatcher.instance.viewManager.safeBlur(activeDomElement); domElement = null; activeTextField = null; _queuedStyle = null; diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart index e5082a09e2..edba5f73af 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart @@ -1411,9 +1411,9 @@ abstract class DefaultTextEditingStrategy with CompositionAwareMixin implements inputConfiguration.autofillGroup?.formElement != null) { _styleAutofillElements(activeDomElement, isOffScreen: true); inputConfiguration.autofillGroup?.storeForm(); - scheduleFocusFlutterView(activeDomElement, activeDomElementView); + EnginePlatformDispatcher.instance.viewManager.safeBlur(activeDomElement); } else { - scheduleFocusFlutterView(activeDomElement, activeDomElementView, removeElement: true); + EnginePlatformDispatcher.instance.viewManager.safeRemove(activeDomElement); } domElement = null; } @@ -1573,29 +1573,6 @@ abstract class DefaultTextEditingStrategy with CompositionAwareMixin implements void moveFocusToActiveDomElement() { activeDomElement.focusWithoutScroll(); } - - /// Move the focus to the given [EngineFlutterView] in the next timer event. - /// - /// The timer gives the engine the opportunity to focus on another element. - /// Shifting focus immediately can cause the keyboard to jump. - static void scheduleFocusFlutterView( - DomElement element, - EngineFlutterView? view, { - bool removeElement = false, - }) { - Timer(Duration.zero, () { - // If by the time the timer fired the focused element is no longer the - // editing element whose editing session was disabled, there's no need to - // move the focus, as it is likely that another widget already took the - // focus. - if (element == domDocument.activeElement) { - view?.dom.rootElement.focusWithoutScroll(); - } - if (removeElement) { - element.remove(); - } - }); - } } /// IOS/Safari behaviour for text editing. diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart index 685e940a53..16a16d5192 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart @@ -108,9 +108,86 @@ class FlutterViewManager { const String viewRootSelector = '${DomManager.flutterViewTagName}[${GlobalHtmlAttributes.flutterViewIdAttributeName}]'; final DomElement? viewRoot = element?.closest(viewRootSelector); - final String? viewIdAttribute = viewRoot?.getAttribute(GlobalHtmlAttributes.flutterViewIdAttributeName); - final int? viewId = viewIdAttribute == null ? null : int.parse(viewIdAttribute); - return viewId == null ? null : _viewData[viewId]; + if (viewRoot == null) { + // `element` is not inside any flutter view. + return null; + } + + final String? viewIdAttribute = viewRoot.getAttribute(GlobalHtmlAttributes.flutterViewIdAttributeName); + assert(viewIdAttribute != null, 'Located Flutter view is missing its id attribute.'); + + final int? viewId = int.tryParse(viewIdAttribute!); + assert(viewId != null, 'Flutter view id must be a valid int.'); + + return _viewData[viewId]; + } + + /// Blurs [element] by transferring its focus to its [EngineFlutterView] + /// `rootElement`. + /// + /// This operation is asynchronous, but happens as soon as possible + /// (see [Timer.run]). + Future safeBlur(DomElement element) { + return Future(() { + _transferFocusToViewRoot(element); + }); + } + + /// Removes [element] after transferring its focus to its [EngineFlutterView] + /// `rootElement`. + /// + /// This operation is asynchronous, but happens as soon as possible + /// (see [Timer.run]). + /// + /// There's a synchronous version of this method: [safeRemoveSync]. + Future safeRemove(DomElement element) { + return Future(() => safeRemoveSync(element)); + } + + /// Synchronously removes [element] after transferring its focus to its + /// [EngineFlutterView] `rootElement`. + /// + /// This is the synchronous version of [safeRemove]. + void safeRemoveSync(DomElement element) { + _transferFocusToViewRoot(element, removeElement: true); + } + + /// Blurs (removes focus) from [element] by transferring its focus to its + /// [EngineFlutterView] DOM's `rootElement` before (optionally) removing it. + /// + /// By default, blurring a focused `element` (or removing it from the DOM) + /// transfers its focus to the `body` element of the page. + /// + /// This method achieves two things when blurring/removing `element`: + /// + /// * Ensures the focus is preserved within the Flutter View when blurring + /// elements that are part of the internal DOM structure of the Flutter + /// app. + /// * Prevents the Flutter engine from reporting bogus "blur" events from the + /// Flutter View. + /// + /// When [removeElement] is true, `element` will be removed from the DOM after + /// its focus (or that of any of its children) is transferred to the root of + /// the view. + /// + /// See: https://jsfiddle.net/ditman/1e2swpno for a JS focus transfer demo. + void _transferFocusToViewRoot( + DomElement element, { + bool removeElement = false, + }) { + final DomElement? activeElement = domDocument.activeElement; + // If the element we're operating on is not active anymore (it can happen + // when this method is called asynchronously), OR the element that we want + // to remove *contains* the `activeElement`. + if (element == activeElement || removeElement && element.contains(activeElement)) { + // Transfer the browser focus to the `rootElement` of the + // [EngineFlutterView] that contains `element` + final EngineFlutterView? view = findViewForElement(element); + view?.dom.rootElement.focusWithoutScroll(); + } + if (removeElement) { + element.remove(); + } } void dispose() { diff --git a/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_test.dart b/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_test.dart index dd9cf23b9d..8a24eb38fb 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -25,6 +25,9 @@ EngineSemanticsOwner owner() => EnginePlatformDispatcher.instance.implicitView!. DomElement get platformViewsHost => EnginePlatformDispatcher.instance.implicitView!.dom.platformViewsHost; +DomElement get flutterViewRoot => + EnginePlatformDispatcher.instance.implicitView!.dom.rootElement; + void main() { internalBootstrapBrowserTest(() { return testMain; @@ -3567,7 +3570,7 @@ void _testRoute() { tester.apply(); expect(capturedActions, isEmpty); - expect(domDocument.activeElement, domDocument.body); + expect(domDocument.activeElement, flutterViewRoot); semantics().semanticsEnabled = false; }); diff --git a/engine/src/flutter/lib/web_ui/test/engine/semantics/text_field_test.dart b/engine/src/flutter/lib/web_ui/test/engine/semantics/text_field_test.dart index c93dc3bf4a..38cef56fa8 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/semantics/text_field_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/semantics/text_field_test.dart @@ -9,7 +9,6 @@ import 'package:test/bootstrap/browser.dart'; import 'package:test/test.dart'; import 'package:ui/src/engine.dart' hide window; import 'package:ui/ui.dart' as ui; -import 'package:ui/ui_web/src/ui_web.dart' as ui_web; import '../../common/test_initialization.dart'; import 'semantics_tester.dart'; @@ -24,8 +23,8 @@ final InputConfiguration multilineConfig = InputConfiguration( ); EngineSemantics semantics() => EngineSemantics.instance; -EngineSemanticsOwner owner() => - EnginePlatformDispatcher.instance.implicitView!.semantics; +EngineFlutterView get flutterView => EnginePlatformDispatcher.instance.implicitView!; +EngineSemanticsOwner owner() => flutterView.semantics; const MethodCodec codec = JSONMethodCodec(); @@ -88,6 +87,8 @@ void testMain() { tearDown(() { semantics().semanticsEnabled = false; + // Most tests in this file expect to start with nothing focused. + domDocument.activeElement?.blur(); }); test('renders a text field', () { @@ -156,8 +157,7 @@ void testMain() { expect( owner().semanticsHost.ownerDocument?.activeElement, isNot(textField)); - // TODO(yjbanov): https://github.com/flutter/flutter/issues/46638 - }, skip: ui_web.browser.browserEngine == ui_web.BrowserEngine.firefox); + }); test('Syncs semantic state from framework', () async { expect( @@ -226,7 +226,9 @@ void testMain() { await Future.delayed(Duration.zero); expect( owner().semanticsHost.ownerDocument?.activeElement, - EnginePlatformDispatcher.instance.implicitView!.dom.rootElement, + flutterView.dom.rootElement, + reason: 'Focus should be returned to the root element of the Flutter view ' + 'after housekeeping DOM operations (blur/remove)', ); // There was no user interaction with the element, @@ -367,7 +369,9 @@ void testMain() { await Future.delayed(Duration.zero); expect( owner().semanticsHost.ownerDocument?.activeElement, - EnginePlatformDispatcher.instance.implicitView!.dom.rootElement, + flutterView.dom.rootElement, + reason: 'Focus should be returned to the root element of the Flutter view ' + 'after housekeeping DOM operations (blur/remove)', ); }); diff --git a/engine/src/flutter/lib/web_ui/test/engine/surface/scene_builder_test.dart b/engine/src/flutter/lib/web_ui/test/engine/surface/scene_builder_test.dart index d2c47c17d8..a058051b40 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/surface/scene_builder_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/surface/scene_builder_test.dart @@ -13,7 +13,6 @@ import 'package:test/bootstrap/browser.dart'; import 'package:test/test.dart'; import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; -import 'package:ui/ui_web/src/ui_web.dart' as ui_web; import '../../common/matchers.dart'; import '../../common/rendering.dart'; @@ -182,8 +181,7 @@ void testMain() { expect(picture.buildCount, 1); expect(picture.updateCount, 0); expect(picture.applyPaintCount, 2); - }, // TODO(yjbanov): https://github.com/flutter/flutter/issues/46638 - skip: ui_web.browser.browserEngine == ui_web.BrowserEngine.firefox); + }); }); group('Compositing order', () {