Fix focus management for text fields #51009

tugorez · 2024-02-27T18:18:22Z

Fix focus management for text fields

This PR:

Refactors the DOM focus function to take options
Removes the timers sorrounding the activeDomElement.focus() so that the input elements get focused as immediate as possible.
Prevents the default on pointerdown in a Flutter View and schedules a requestViewFocusChange to claim focus in that view after some time. This gives 2 the opportunity to focus the right <input /> or <textarea /> element. This helps focus correctly transition from one input element to another (without jumping to a flutter view in between).
Deactivating a TextField doesn't blur the focused element anymore, it insteads schedules for later a call to move the focus to the flutter view if nothing inside it claimed focus first.
Prevents scroll in all the focus calls (this should help with the view jumping when focusing one text field after another).

Sample apps

Full screen mode: https://tugorez.com/flutter_focus_web
Embedded mode: https://tugorez.com/flutter_focus_web?embedded

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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

When Safari auto-completes a field, it synthesizes an event that quacks like a DomKeyboardEvent, but it lacks the `shiftKey` property. This change makes the property nullable, and adjusts its use. (This only affects debug mode builds)

ditman · 2024-06-19T00:25:16Z

This may break autofill forms. If you look at line 1425, we used to only blur the element. But now we are also removing it.

I've re-deployed a small testing app with a form to test autocomplete, here:

Without PR: https://dit-tests.web.app/before/
With PR: https://dit-tests.web.app/after/

I think this now works similarly across all browsers, PTAL @mdebbar.

ditman · 2024-06-19T00:26:23Z

lib/web_ui/lib/src/engine/dom.dart

+  external JSBoolean? get _shiftKey;
+  bool? get shiftKey => _shiftKey?.toDart;


This change is unrelated to this PR, I can move it to a separate PR to keep this one small.

ditman · 2024-06-19T00:28:09Z

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

+    if (this is! SafariDesktopTextEditingStrategy) {
+      // handleBlur causes Safari to reopen autofill dialogs after autofill,
+      // so we don't attach the listener there.
+      subscriptions.add(DomSubscription(activeDomElement, 'blur', handleBlur));
+    }


the handleBlur function was causing trouble with autocomplete in Safari (refocusing the field). Is this maybe not needed anymore?

mdebbar

LGTM!

…150582) flutter/engine@dd37cef...dda82d9 2024-06-20 tarrinneal@gmail.com Update StandardMessageCodec.readValue to be @nullable (flutter/engine#53473) 2024-06-20 bruno.leroux@gmail.com [Web] Fix extra new line when inputAction is not newline for a multilâ�¦ (flutter/engine#53453) 2024-06-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 366eb1b4b308 to be6b533e07e7 (1 revision) (flutter/engine#53492) 2024-06-20 tugorez@users.noreply.github.com Fix focus management for text fields (flutter/engine#51009) 2024-06-20 skia-flutter-autoroll@skia.org Roll Dart SDK from cbd933e707e7 to 366eb1b4b308 (1 revision) (flutter/engine#53487) 2024-06-20 skia-flutter-autoroll@skia.org Roll Skia from 17626ca22729 to 4471ee07e223 (1 revision) (flutter/engine#53486) 2024-06-20 skia-flutter-autoroll@skia.org Roll Dart SDK from e403519a4436 to cbd933e707e7 (1 revision) (flutter/engine#53485) 2024-06-20 skia-flutter-autoroll@skia.org Roll Skia from 4acebac47ea5 to 17626ca22729 (1 revision) (flutter/engine#53484) 2024-06-20 skia-flutter-autoroll@skia.org Roll Skia from 199e1a49b091 to 4acebac47ea5 (1 revision) (flutter/engine#53482) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jiahaog · 2024-06-21T12:52:09Z

Reason for revert: This causes b/348598454. We're getting test failures with stack traces like the following after this PR:

Cannot get renderObject of inactive element.
In order for an element to have a valid renderObject, it must be active, which means it is part of the tree.
Instead, this element is in the _ElementLifecycle.inactive state.
If you called this method from a State object, consider guarding it with State.mounted.
The findRenderObject() method was called for the following element:
  FocusScope
package:flutter/src/widgets/framework.dart 4725:9                   <fn>
package:flutter/src/widgets/framework.dart 4737:14                  findRenderObject
package:flutter/src/widgets/focus_manager.dart 842:33               get rect
package:flutter/src/widgets/focus_traversal.dart 1108:20            new
package:flutter/src/widgets/focus_traversal.dart 1353:49            <fn>
package:flutter/src/widgets/focus_traversal.dart 1353:75            sortDescendants
package:flutter/src/widgets/focus_traversal.dart 466:55             _sortAllDescendants
package:flutter/src/widgets/focus_traversal.dart 309:82             [_findInitialFocus]
package:flutter/src/widgets/focus_traversal.dart 277:12             findFirstFocus
package:flutter/src/widgets/view.dart 216:33                        didChangeViewFocus
package:flutter/src/widgets/binding.dart 971:15                     handleViewFocusChanged
package:flutter_test/src/window.dart 194:25                         [_handleViewFocusChanged]
...

which seems related to the focus changes in this PR.

This reverts commit 87a632b.

Reverts: #51009 Initiated by: jiahaog Reason for reverting: This causes b/348598454. We're getting test failures with stack traces like the following after this PR: ``` Cannot get renderObject of inactive element. In order for an element to have a valid renderObject, it must be active, which means it is part of the tree. Instead, this element is in the _ElementLifecycle.inactive state. If you called this method from a State object, consider guarding ``` Original PR Author: tugorez Reviewed By: {mdebbar} This change reverts the following previous change: Fix focus management for text fields This PR: 1. Refactors the DOM `focus` function to take [options][1] 2. Removes the timers sorrounding the `activeDomElement.focus()` so that the input elements get focused as immediate as possible. 3. Prevents the default on pointerdown in a Flutter View and schedules a `requestViewFocusChange` to claim focus in that view after some time. This gives `2` the opportunity to focus the right `<input />` or `<textarea />` element. This helps focus correctly transition from one input element to another (without jumping to a flutter view in between). 4. Deactivating a `TextField` doesn't blur the focused element anymore, it insteads schedules for later a call to move the focus to the flutter view if nothing inside it claimed focus first. 5. Prevents scroll in all the focus calls (this should help with the view jumping when focusing one text field after another). ## Sample apps 1. Full screen mode: https://tugorez.com/flutter_focus_web 2. Embedded mode: https://tugorez.com/flutter_focus_web?embedded [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

tugorez · 2024-06-21T21:27:34Z

b/348598454.

@gspencergoog is there something we're missing on the framework?

tugorez · 2024-06-21T23:45:47Z

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

@@ -16,7 +16,7 @@ final class ViewFocusBinding {
  ///
  /// DO NOT rely on this bit as it will go away soon. You're warned :)!
  @visibleForTesting
-  static bool isEnabled = false;
+  static bool isEnabled = true;


@ditman we might want to reland these changes but with this one set to false. Not sure what's breaking on the framework side but don't think the rest of these changes is related.

Will take a look!

Reland posted: #53537 /cc @tugorez

This reverts commit cf3ac2d (flutter#53502).

Relands: [**Fix focus management for text fields** (#51009)](#51009) by: * Reverting commit: cf3ac2d (#53502). * Keeping the new `ViewFocusBinding` disabled, as [suggested](#51009 (comment)). [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…lutter#150582) flutter/engine@dd37cef...dda82d9 2024-06-20 tarrinneal@gmail.com Update StandardMessageCodec.readValue to be @nullable (flutter/engine#53473) 2024-06-20 bruno.leroux@gmail.com [Web] Fix extra new line when inputAction is not newline for a multilâ�¦ (flutter/engine#53453) 2024-06-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 366eb1b4b308 to be6b533e07e7 (1 revision) (flutter/engine#53492) 2024-06-20 tugorez@users.noreply.github.com Fix focus management for text fields (flutter/engine#51009) 2024-06-20 skia-flutter-autoroll@skia.org Roll Dart SDK from cbd933e707e7 to 366eb1b4b308 (1 revision) (flutter/engine#53487) 2024-06-20 skia-flutter-autoroll@skia.org Roll Skia from 17626ca22729 to 4471ee07e223 (1 revision) (flutter/engine#53486) 2024-06-20 skia-flutter-autoroll@skia.org Roll Dart SDK from e403519a4436 to cbd933e707e7 (1 revision) (flutter/engine#53485) 2024-06-20 skia-flutter-autoroll@skia.org Roll Skia from 4acebac47ea5 to 17626ca22729 (1 revision) (flutter/engine#53484) 2024-06-20 skia-flutter-autoroll@skia.org Roll Skia from 199e1a49b091 to 4acebac47ea5 (1 revision) (flutter/engine#53482) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added the platform-web Code specifically for the web engine label Feb 27, 2024

tugorez closed this Mar 27, 2024

tugorez reopened this Mar 28, 2024

tugorez closed this Apr 23, 2024

tugorez reopened this Apr 23, 2024

tugorez and others added 22 commits May 17, 2024 11:26

Make sure the elements can't be reached by keyboard

d2e4ccc

Move the focus to the <flutter-view /> instead of blur.

ea09d6e

Add listener to the subscription (otherwise never gets cleaned up).

edab1dc

Delay the input removal

8bbc206

Prevent default on pointer down on flutter views

ed2f851

Format

78521ec

Add a test that checks focus goes from one input to another

72d3956

Remove the blur listeners

7a6c596

Enable view focus binding

11b7dae

Disable view focus binding.

df75938

Add ios awaits

edfdacf

Remove safari desktop delay

7daf2a1

Remove spaces

7e811da

Bring blur handlers back

c23c944

Format

08ab966

Add mising blur handler

8c99cb4

Remove blur events again lol

153eee4

Prevent scroll on focus

e8cf8e8

Make the linter happy

5cf50ac

Disable view focus

6f85439

Formatting

e931fbc

Refactor focus active dom element

d70596d

tugorez requested review from mdebbar and ditman May 20, 2024 17:31

tugorez marked this pull request as ready for review May 20, 2024 17:32

This comment was marked as resolved.

Sign in to view

ditman reviewed Jun 15, 2024

View reviewed changes

lib/web_ui/lib/src/engine/text_editing/text_editing.dart Show resolved Hide resolved

ditman added 2 commits June 18, 2024 16:43

Do not attach a blur handler on Safari

e35705e

Makes shiftKey in DomKeyboardEvent nullable.

39c5c8d

When Safari auto-completes a field, it synthesizes an event that quacks like a DomKeyboardEvent, but it lacks the `shiftKey` property. This change makes the property nullable, and adjusts its use. (This only affects debug mode builds)

ditman reviewed Jun 19, 2024

View reviewed changes

mdebbar approved these changes Jun 20, 2024

View reviewed changes

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 20, 2024

auto-submit bot merged commit 87a632b into flutter:main Jun 20, 2024
29 checks passed

engine-flutter-autoroll mentioned this pull request Jun 20, 2024

Roll Flutter Engine from dd37cefd4a94 to 87a632b43536 (6 revisions) flutter/flutter#150574

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 20, 2024

87a632b43 Fix focus management for text fields (flutter/engine#51009)

d1a7cf4

engine-flutter-autoroll mentioned this pull request Jun 20, 2024

Roll Flutter Engine from dd37cefd4a94 to d72135133b9a (8 revisions) flutter/flutter#150580

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 20, 2024

87a632b43 Fix focus management for text fields (flutter/engine#51009)

ad789df

engine-flutter-autoroll mentioned this pull request Jun 20, 2024

Roll Flutter Engine from dd37cefd4a94 to dda82d905f37 (9 revisions) flutter/flutter#150582

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 20, 2024

87a632b43 Fix focus management for text fields (flutter/engine#51009)

d59a26b

jiahaog added the revert Label used to revert changes in a closed and merged pull request. label Jun 21, 2024

auto-submit bot pushed a commit that referenced this pull request Jun 21, 2024

Revert "Fix focus management for text fields (#51009)"

ee1a7bd

This reverts commit 87a632b.

auto-submit bot mentioned this pull request Jun 21, 2024

Reverts "Fix focus management for text fields (#51009)" #53502

Merged

8 tasks

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jun 21, 2024

tugorez commented Jun 21, 2024

View reviewed changes

ditman added a commit to ditman/flutter-engine that referenced this pull request Jun 24, 2024

Reland "Fix focus management for text fields (flutter#51009)"

1740d73

This reverts commit cf3ac2d (flutter#53502).

ditman mentioned this pull request Jun 24, 2024

[web] Reland "Fix focus management for text fields (#51009)" #53537

Merged

8 tasks

yjbanov mentioned this pull request Jul 17, 2024

Make sure focus works with text fields flutter/flutter#145362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix focus management for text fields #51009

Fix focus management for text fields #51009

tugorez commented Feb 27, 2024 •

edited

Loading

This comment was marked as resolved.

ditman commented Jun 19, 2024

ditman Jun 19, 2024

ditman Jun 19, 2024

mdebbar left a comment

jiahaog commented Jun 21, 2024 •

edited

Loading

tugorez commented Jun 21, 2024

tugorez Jun 21, 2024

ditman Jun 22, 2024

ditman Jun 24, 2024

		external JSBoolean? get _shiftKey;
		bool? get shiftKey => _shiftKey?.toDart;

Fix focus management for text fields #51009

Fix focus management for text fields #51009

Conversation

tugorez commented Feb 27, 2024 • edited Loading

Sample apps

Pre-launch Checklist

This comment was marked as resolved.

ditman commented Jun 19, 2024

ditman Jun 19, 2024

Choose a reason for hiding this comment

ditman Jun 19, 2024

Choose a reason for hiding this comment

mdebbar left a comment

Choose a reason for hiding this comment

jiahaog commented Jun 21, 2024 • edited Loading

tugorez commented Jun 21, 2024

tugorez Jun 21, 2024

Choose a reason for hiding this comment

ditman Jun 22, 2024

Choose a reason for hiding this comment

ditman Jun 24, 2024

Choose a reason for hiding this comment

tugorez commented Feb 27, 2024 •

edited

Loading

jiahaog commented Jun 21, 2024 •

edited

Loading