[web] close input connection when window/iframe loses focus (flutter#166804)

Fixes flutter#155265

This includes 2 fixes:

* When the window/iframe loses focus, close the text input connection
instead of grabbing the focus again.
* Do not enable semantics using the placeholder when moving focus using
the "Tab" key.

Bonus: remove the no longer necessary `ViewFocusBinding.isEnabled`
(doesn't fix any issues, just a clean-up).

---------

Co-authored-by: Mouad Debbar <[email protected]>

Author: yjbanov

engine/src/flutter/lib/web_ui/lib/src/engine/platform_dispatcher/view_focus_binding.dart

@@ -5,20 +5,13 @@
 import 'dart:async';
 import 'dart:js_interop';
 
-import 'package:meta/meta.dart';
 import 'package:ui/src/engine.dart';
 import 'package:ui/ui.dart' as ui;
 
 /// Tracks the [FlutterView]s focus changes.
 final class ViewFocusBinding {
   ViewFocusBinding(this._viewManager, this._onViewFocusChange);
 
-  /// Whether [FlutterView] focus changes will be reported and performed.
-  ///
-  /// DO NOT rely on this bit as it will go away soon. You're warned :)!
-  @visibleForTesting
-  static bool isEnabled = true;
-
   final FlutterViewManager _viewManager;
   final ui.ViewFocusChangeCallback _onViewFocusChange;
 
@@ -46,9 +39,6 @@ final class ViewFocusBinding {
   }
 
   void changeViewFocus(int viewId, ui.ViewFocusState state) {
-    if (!isEnabled) {
-      return;
-    }
     final DomElement? viewElement = _viewManager[viewId]?.dom.rootElement;
 
     switch (state) {
@@ -97,10 +87,6 @@ final class ViewFocusBinding {
   });
 
   void _handleFocusChange(DomElement? focusedElement) {
-    if (!isEnabled) {
-      return;
-    }
-
     final int? viewId = _viewId(focusedElement);
     if (viewId == _lastViewId) {
       return;

engine/src/flutter/lib/web_ui/lib/src/engine/semantics/semantics_helper.dart

@@ -79,6 +79,15 @@ abstract class SemanticsEnabler {
   /// Or if the received [DomEvent] is suitable/enough for enabling the
   /// semantics. See [tryEnableSemantics].
   bool shouldEnableSemantics(DomEvent event) {
+    // Simply tabbing into the placeholder element should not cause semantics
+    // to be enabled. The user should actually click on the placeholder.
+    if (event.isA<DomKeyboardEvent>()) {
+      event as DomKeyboardEvent;
+      if (event.key == 'Tab') {
+        return true;
+      }
+    }
+
     if (!isWaitingToEnableSemantics) {
       // Forward to framework as normal.
       return true;

engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart

@@ -170,7 +170,12 @@ void _insertEditingElementInView(DomElement element, int viewId) {
     view != null,
     'Could not find View with id $viewId. This should never happen, please file a bug!',
   );
-  view!.dom.textEditingHost.append(element);
+  final host = view!.dom.textEditingHost;
+  // Do not cause DOM disturbance unless necessary. Doing superfluous DOM operations may seem
+  // harmless, but it actually causes focus changes that could break things.
+  if (!host.contains(element)) {
+    host.append(element);
+  }
 }
 
 /// Form that contains all the fields in the same AutofillGroup.
@@ -363,7 +368,11 @@ class EngineAutofillForm {
       mainTextEditingElement.style.pointerEvents = 'all';
     }
 
-    formElement.insertBefore(mainTextEditingElement, insertionReferenceNode);
+    // Do not cause DOM disturbance unless necessary. Doing superfluous DOM operations may seem
+    // harmless, but it actually causes focus changes that could break things.
+    if (!formElement.contains(mainTextEditingElement)) {
+      formElement.insertBefore(mainTextEditingElement, insertionReferenceNode);
+    }
     _insertEditingElementInView(formElement, viewId);
   }
 
@@ -1535,8 +1544,26 @@ abstract class DefaultTextEditingStrategy
     event as DomFocusEvent;
 
     final DomElement? willGainFocusElement = event.relatedTarget as DomElement?;
-    if (willGainFocusElement == null ||
-        _viewForElement(willGainFocusElement) == activeDomElementView) {
+    if (willGainFocusElement == null) {
+      // If the element to gain focus is reported as `null`, it means that the
+      // window/iframe that Flutter runs in is losing focus. In this case, the
+      // engine signals to the framework to close the text input connection,
+      // allowing the focus to move elsewhere.
+      //
+      // This is fixing https://github.com/flutter/flutter/issues/155265.
+      textEditing.sendTextConnectionClosedToFrameworkIfAny();
+    } else if (_viewForElement(willGainFocusElement) == activeDomElementView) {
+      // If the focus stays within the same FlutterView, ensure the focus stays
+      // on the input element.
+
+      // TODO(yjbanov): Make text input less grabby. See: https://github.com/flutter/flutter/issues/166857
+      // The motivation/reasoning behind this remains murky.
+      // It's unclear why, if the browser wants to remove focus from the input,
+      // we must insist that it stays on the element. This could lead to
+      // different elements/widgets fighting over who gets the focus, or resist
+      // to user's request to move focus elsewhere, which can be super-annoying
+      // UX. We should reevaluate what it is we're trying to do here. Perhaps
+      // there's a better way.
       moveFocusToActiveDomElement();
     }
   }

engine/src/flutter/lib/web_ui/test/common/spy.dart

@@ -11,7 +11,7 @@ import 'package:ui/ui.dart';
 
 /// Encapsulates the info of a platform message that was intercepted by
 /// [PlatformMessagesSpy].
-class PlatformMessage {
+final class PlatformMessage {
   PlatformMessage(this.channel, this.methodCall);
 
   /// The name of the channel on which the message was sent.
@@ -27,6 +27,19 @@ class PlatformMessage {
   dynamic get methodArguments => methodCall.arguments;
 }
 
+/// A message that carries a plain string.
+///
+/// Messages of this type are coded using [StringCodec].
+final class PlatformStringMessage {
+  PlatformStringMessage(this.channel, this.string);
+
+  /// The name of the channel on which the message was sent.
+  final String channel;
+
+  /// The value of the string passed in the message.
+  final String string;
+}
+
 /// Intercepts platform messages sent from the engine to the framework.
 ///
 /// It holds all intercepted platform messages in a [messages] list that can
@@ -37,16 +50,23 @@ class PlatformMessagesSpy {
 
   bool get _isActive => _callback != null;
 
-  /// List of intercepted messages since the last [setUp] call.
+  /// List of intercepted method calls since the last [setUp] call.
   final List<PlatformMessage> messages = <PlatformMessage>[];
 
+  /// List of intercepted string messages since the last [setUp] call.
+  final List<PlatformStringMessage> strings = <PlatformStringMessage>[];
+
   /// Start spying on platform messages.
   ///
   /// This is typically called inside a test's `setUp` callback.
   void setUp() {
     assert(!_isActive);
     _callback = (String channel, ByteData? data, PlatformMessageResponseCallback? callback) {
-      messages.add(PlatformMessage(channel, const JSONMethodCodec().decodeMethodCall(data)));
+      if (channel == 'flutter/lifecycle') {
+        strings.add(PlatformStringMessage(channel, const StringCodec().decodeMessage(data!)));
+      } else {
+        messages.add(PlatformMessage(channel, const JSONMethodCodec().decodeMethodCall(data)));
+      }
     };
 
     _backup = PlatformDispatcher.instance.onPlatformMessage;
@@ -62,6 +82,7 @@ class PlatformMessagesSpy {
     assert(PlatformDispatcher.instance.onPlatformMessage == _callback);
     _callback = null;
     messages.clear();
+    strings.clear();
     PlatformDispatcher.instance.onPlatformMessage = _backup;
   }
 }

engine/src/flutter/lib/web_ui/test/engine/platform_dispatcher/view_focus_binding_test.dart

@@ -17,15 +17,12 @@ void testMain() {
     late EnginePlatformDispatcher dispatcher;
 
     setUp(() {
-      ViewFocusBinding.isEnabled = true;
-
       dispatcher = EnginePlatformDispatcher.instance;
       dispatchedViewFocusEvents = <ui.ViewFocusEvent>[];
       dispatcher.onViewFocusChange = dispatchedViewFocusEvents.add;
     });
 
     tearDown(() {
-      ViewFocusBinding.isEnabled = false;
       EngineSemantics.instance.semanticsEnabled = false;
     });
 

engine/src/flutter/lib/web_ui/test/engine/semantics/semantics_helper_test.dart

@@ -64,6 +64,24 @@ void testMain() {
       }
     });
 
+    test('Tab key should not enable semantics', () async {
+      final testSemanticsEnabler = FakeSemanticsEnabler();
+
+      // Tab should not enable semantics
+      {
+        final event = createDomKeyboardEvent('keydown', <String, Object>{'key': 'Tab'});
+        expect(testSemanticsEnabler.shouldEnableSemantics(event), isTrue);
+        expect(testSemanticsEnabler.tryEnableSemanticsCallCount, 0);
+      }
+
+      // Enter key is allowed to try to enable semantics
+      {
+        final event = createDomKeyboardEvent('keydown', <String, Object>{'key': 'Enter'});
+        expect(testSemanticsEnabler.shouldEnableSemantics(event), isFalse);
+        expect(testSemanticsEnabler.tryEnableSemanticsCallCount, 1);
+      }
+    });
+
     test(
       'Relevant events targeting placeholder should not be forwarded to the framework',
       () async {
@@ -151,3 +169,26 @@ void testMain() {
     skip: isDesktop && ui_web.browser.browserEngine != ui_web.BrowserEngine.blink,
   );
 }
+
+class FakeSemanticsEnabler extends SemanticsEnabler {
+  @override
+  void dispose() {
+    throw UnimplementedError();
+  }
+
+  @override
+  bool get isWaitingToEnableSemantics => true;
+
+  @override
+  DomElement prepareAccessibilityPlaceholder() {
+    throw UnimplementedError();
+  }
+
+  int tryEnableSemanticsCallCount = 0;
+
+  @override
+  bool tryEnableSemantics(DomEvent event) {
+    tryEnableSemanticsCallCount += 1;
+    return false;
+  }
+}

engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart

@@ -585,6 +585,128 @@ Future<void> testMain() async {
       expect(editingStrategy.domElement!.style.width, '10px');
       expect(editingStrategy.domElement!.style.height, '10px');
     });
+
+    test('does not blur input when size is updated', () async {
+      final PlatformMessagesSpy spy = PlatformMessagesSpy();
+      spy.setUp();
+
+      editingStrategy!.enable(
+        multilineConfig,
+        onChange: trackEditingState,
+        onAction: trackInputAction,
+      );
+
+      expect(editingStrategy!.isEnabled, isTrue);
+
+      final inputElement = editingStrategy!.domElement!;
+      expect(domDocument.activeElement, inputElement);
+
+      int blurCount = 0;
+      final blurListener = createDomEventListener((_) {
+        blurCount++;
+      });
+      inputElement.addEventListener('blur', blurListener);
+
+      final sizeCompleter = Completer<void>();
+      testTextEditing.acceptCommand(
+        TextInputSetEditableSizeAndTransform(
+          geometry: EditableTextGeometry(
+            width: 240,
+            height: 60,
+            globalTransform: Matrix4.translationValues(11, 12, 0).storage,
+          ),
+        ),
+        sizeCompleter.complete,
+      );
+      await sizeCompleter.future;
+
+      expect(blurCount, isZero);
+      // `TextInputClient.onConnectionClosed` shouldn't have been called.
+      expect(spy.messages, isEmpty);
+
+      inputElement.removeEventListener('blur', blurListener);
+      spy.tearDown();
+    });
+
+    test('closes input connection when window/iframe loses focus', () async {
+      final PlatformMessagesSpy spy = PlatformMessagesSpy();
+      spy.setUp();
+
+      textEditing.configuration = singlelineConfig;
+
+      final showCompleter = Completer<void>();
+      textEditing.acceptCommand(const TextInputShow(), showCompleter.complete);
+      await showCompleter.future;
+
+      expect(textEditing.isEditing, isTrue);
+
+      expect(domDocument.activeElement, textEditing.strategy.domElement);
+
+      final event = createDomEvent('Event', 'blur');
+      editingStrategy!.handleBlur(event);
+
+      expect(spy.messages, hasLength(1));
+      expect(spy.messages[0].channel, 'flutter/textinput');
+      expect(spy.messages[0].methodName, 'TextInputClient.onConnectionClosed');
+
+      spy.tearDown();
+    });
+
+    test(
+      'keeps focus within window/iframe when the focus moves within the flutter view in Chrome but not Safari',
+      () async {
+        final PlatformMessagesSpy spy = PlatformMessagesSpy();
+        spy.setUp();
+
+        textEditing.configuration = singlelineConfig;
+
+        final showCompleter = Completer<void>();
+        textEditing.acceptCommand(const TextInputShow(), showCompleter.complete);
+        await showCompleter.future;
+
+        // The "setSizeAndTransform" message has to be here before we call
+        // checkInputEditingState, since on some platforms (e.g. Desktop Safari)
+        // we don't put the input element into the DOM until we get its correct
+        // dimensions from the framework.
+        final MethodCall setSizeAndTransform = configureSetSizeAndTransformMethodCall(
+          150,
+          50,
+          Matrix4.translationValues(10.0, 20.0, 30.0).storage.toList(),
+        );
+        textEditing.channel.handleTextInput(
+          codec.encodeMethodCall(setSizeAndTransform),
+          (ByteData? data) {},
+        );
+
+        expect(textEditing.isEditing, isTrue);
+
+        expect(domDocument.activeElement, textEditing.strategy.domElement);
+
+        final flutterView =
+            EnginePlatformDispatcher.instance.viewManager.findViewForElement(
+              textEditing.strategy.domElement,
+            )!;
+
+        flutterView.dom.rootElement.focusWithoutScroll();
+        expect(spy.messages, isEmpty);
+
+        if (isSafari) {
+          // In Safari the web engine does not respond to blur, so there's no
+          // expectation that the input element keep focus.
+          expect(domDocument.activeElement, flutterView.dom.rootElement);
+        } else if (isFirefox) {
+          // This is a mysterious behavior in Firefox. Even though the engine does
+          // call <input>.focus() the browser doesn't move focus to the target
+          // element. This only happens in the test harness. When testing
+          // manually, Firefox happily moves focus to the input element.
+          expect(domDocument.activeElement, flutterView.dom.rootElement);
+        } else {
+          expect(domDocument.activeElement, textEditing.strategy.domElement);
+        }
+
+        spy.tearDown();
+      },
+    );
   });
 
   group('$HybridTextEditing', () {