[Focus] defer autofocus resolution to `_applyFocusChange` #85562

LongCatIsLooong · 2021-06-29T18:57:07Z

Fixes: #84845.
also fixes a potential crash in FocusState.didUpdateWdiget.

when FocusScopeNode.autofocus is called, the FocusScopeNode may have a
focusedChild that's about to be detached.

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

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

Fixes: flutter#84845. also fixes a potential crash in FocusState.didUpdateWdiget. when `FocusScopeNode.autofocus` is called, the FocusScopeNode may have a focusedChild that's about to be detached.

LongCatIsLooong

On a different note, have we tried using LinkedHashList for storing FocusNode._ancestors and/or FocusScopeNode._focusedChildren?

LongCatIsLooong · 2021-06-29T18:59:02Z

packages/flutter/lib/src/widgets/focus_manager.dart

+    assert(_manager != null);
+    assert(_focusDebug('Autofocus scheduled for $node: scope $this'));
+    _manager?._pendingAutofocuses.add(_Autofocus(scope: this, autofocusNode: node));
+    _manager?._markNeedsUpdate();
  }

  @override
  void _doRequestFocus({required bool findFirstFocus}) {


This method still does the same thing. Just trying to avoid runtime type checks like primaryFocus is FocusScopeNode.

LongCatIsLooong · 2021-06-29T19:02:40Z

packages/flutter/lib/src/widgets/focus_manager.dart

@@ -1756,6 +1759,13 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
  void _applyFocusChange() {
    _haveScheduledUpdate = false;
    final FocusNode? previousFocus = _primaryFocus;
+    for (final _Autofocus autofocus in _pendingAutofocuses) {
+      if (identical(autofocus.scope._manager, this)) {


This isn't going to work well if a subtree involved changes manager after calling FocusScopeNode.autofocus since we keep the information in the manager. FocusManager has a public constructor but I guess most of the time we only use the singleton from bindings.

LongCatIsLooong · 2021-06-29T19:04:04Z

packages/flutter/lib/src/widgets/focus_scope.dart

@@ -657,7 +657,8 @@ class _FocusState extends State<Focus> {
      focusNode.descendantsAreFocusable = widget.descendantsAreFocusable;
    } else {
      _focusAttachment!.detach();
-      focusNode.removeListener(_handleFocusChanged);


This getter crashes if we supply an external FocusNode in the old widget and do not specify one in the new widget.

xu-baolin · 2021-06-30T09:28:22Z

packages/flutter/lib/src/widgets/focus_manager.dart

+
+    assert(_manager != null);
+    assert(_focusDebug('Autofocus scheduled for $node: scope $this'));
+    _manager?._pendingAutofocuses.add(_Autofocus(scope: this, autofocusNode: node));


Should remove the node if it was disposed?

The node and the scope? I think they're supposed to be owned by widgets so FocusManager shouldn't call dispose on FocusNodes.

Yes, that's correct. The owner of the FocusNode has the responsibility to dispose of it.

I mean the node was disposed before applying the autofocus.

Ah I see. But I'm not sure if there's a good way to check if a FocusNode is already disposed. It seems fine to keep a disposed FocusNode around? In _applyFocusChange if either of these 2 nodes are detached from the tree then we throw the entry away anyways.

If a node is disposed then it's guaranteed to be detached (reattaching disposed nodes back to the tree shouldn't be allowed). In _applyFocusChange we do check if any of the nodes are already detached and discard them.

That reminds me we didn't have tests to verify the first come first served behavior. I'll add that in a bit.

If we scroll the list ,the primary focus node will be changed. A little strange I think.

That's only true if you have autofocus set to true on more than one widget, which, as I said above, isn't well defined (mainly because of situations like this where things are built later).

xu-baolin · 2021-06-30T19:11:21Z

packages/flutter/lib/src/widgets/focus_manager.dart

@@ -1756,6 +1759,13 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
  void _applyFocusChange() {
    _haveScheduledUpdate = false;
    final FocusNode? previousFocus = _primaryFocus;
+    for (final _Autofocus autofocus in _pendingAutofocuses) {
+      if (identical(autofocus.scope._manager, this)) {


@LongCatIsLooong did you mean this line? For the test I posted this is true .

Ohh right. I thought detaching removes the manager.

Maybe we can do this.

After looking at FocusNode.detach, I think the idea is the widget tree (or whatever that the focus tree is going to attach to & and manage focus for) is responsible for calling "reparent" and "detach" so each focus node has the right parent and manager. So I think these checks should be sufficient.

@xu-baolin you mean this is true for something in the first row? I'd expect those elements to have already been disposed of (along with their focus nodes)?

The node is disposed but the scope node not.

if (identical(autofocus.scope._manager, this) && autofocus.autofocusNode._parent!= null)

LongCatIsLooong · 2021-07-19T23:41:07Z

@xu-baolin I added some additional checks to make sure the scope is attached to the focus tree.

xu-baolin · 2021-07-20T01:29:52Z

packages/flutter/test/widgets/focus_manager_test.dart

+        ),
+      );
+
+      // _applyFocusChanges will be called before persistentCallbacks,


'_applyFocusChanges' should be '_applyFocusChange'.

I remember the microTask should be called after the persistentCallbacks in a frame, right? Or do you mean before the next frame?

The microtask queue is also drained before the persistent callbacks.

xu-baolin

LGTM

gspencergoog · 2021-07-29T14:58:50Z

@LongCatIsLooong Are you still planning to commit this?

Removing trailing whitespaces

LongCatIsLooong · 2021-08-02T20:17:25Z

@gspencergoog yeah but there appears to be an analyzer problem that I believe has nothing to do with the PR. Does the approach look good to you?

xu-baolin · 2021-08-09T01:45:51Z

@LongCatIsLooong Can we reland #82671 now？#84845 (comment)

LongCatIsLooong added 2 commits June 29, 2021 11:49

[Focus] defer autofocus resolution to _applyFocusChange

8e966d6

Fixes: flutter#84845. also fixes a potential crash in FocusState.didUpdateWdiget. when `FocusScopeNode.autofocus` is called, the FocusScopeNode may have a focusedChild that's about to be detached.

Merge remote-tracking branch 'upstream/master' into auto-focus

9514d17

LongCatIsLooong requested review from gspencergoog and xu-baolin June 29, 2021 18:57

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 29, 2021

google-cla bot added the cla: yes label Jun 29, 2021

LongCatIsLooong commented Jun 29, 2021

View reviewed changes

xu-baolin reviewed Jun 30, 2021

View reviewed changes

Piinks added the f: focus Focus traversal, gaining or losing focus label Jun 30, 2021

code comments and one more test

55d8d93

LongCatIsLooong force-pushed the auto-focus branch from cd9e0f5 to 55d8d93 Compare July 2, 2021 19:03

xu-baolin mentioned this pull request Jul 16, 2021

move autofocus process to postFrame phase #85219

Closed

Merge remote-tracking branch 'upstream/master' into auto-focus

e7f809e

LongCatIsLooong requested a review from xu-baolin July 19, 2021 23:39

xu-baolin reviewed Jul 20, 2021

View reviewed changes

Add reparent test. Move logic to apply

e5e233b

LongCatIsLooong force-pushed the auto-focus branch from 80b60f8 to e5e233b Compare July 20, 2021 02:31

xu-baolin approved these changes Jul 20, 2021

View reviewed changes

LongCatIsLooong added 3 commits July 31, 2021 14:59

Merge branch 'master' into auto-focus

4329693

Update focus_manager.dart

9a2af8d

Removing trailing whitespaces

Update focus_manager.dart

a83fb4b

Merge remote-tracking branch 'upstream/master' into auto-focus

54b2025

LongCatIsLooong force-pushed the auto-focus branch from e94c8ea to 54b2025 Compare August 3, 2021 21:10

LongCatIsLooong added the waiting for tree to go green label Aug 4, 2021

fluttergithubbot merged commit 0f4e1db into flutter:master Aug 4, 2021

LongCatIsLooong deleted the auto-focus branch August 4, 2021 19:35

[Focus] defer autofocus resolution to _applyFocusChange #85562

[Focus] defer autofocus resolution to _applyFocusChange #85562

Uh oh!

Conversation

LongCatIsLooong commented Jun 29, 2021

Pre-launch Checklist

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu-baolin Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Jul 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu-baolin left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Jul 29, 2021

Uh oh!

LongCatIsLooong commented Aug 2, 2021

Uh oh!

xu-baolin commented Aug 9, 2021

Uh oh!

Uh oh!

[Focus] defer autofocus resolution to `_applyFocusChange` #85562

[Focus] defer autofocus resolution to `_applyFocusChange` #85562

LongCatIsLooong Jun 29, 2021 •

edited

Loading

xu-baolin Jul 3, 2021 •

edited

Loading