Repository navigation
FocusScopeNode.descendantsAreFocusable set to false after disabling and enabling canRequestFocus #147256
Description
Happens with current main (3.22.0-16.0.pre.26).
Widget test to reproduce:
testWidgets('FocusScope descendantsAreFocusable', (WidgetTester tester) async {
final focusScopeNode = FocusScopeNode();
await tester.pumpWidget(
FocusScope(
node: focusScopeNode,
child: const SizedBox.shrink(),
),
);
expect(focusScopeNode.descendantsAreFocusable, isTrue);
focusScopeNode.canRequestFocus = false;
expect(focusScopeNode.descendantsAreFocusable, isFalse);
focusScopeNode.canRequestFocus = true;
expect(focusScopeNode.descendantsAreFocusable, isTrue);
// Disabling canRequestFocus and updating the widget will permanently disable descendantsAreFocusable.
focusScopeNode.canRequestFocus = false;
// After this pump, setting canRequestFocus back will not restore
// descendantsAreFocusable to true.
await tester.pumpWidget(
FocusScope(
node: focusScopeNode,
child: const SizedBox.shrink(),
),
);
focusScopeNode.canRequestFocus = true;
// Test failure:
// This should be back to true, but in FocusNode state didUpdateWidget the
// descendantsAreFocusable on the focusScopeNode got set to false.
expect(focusScopeNode.descendantsAreFocusable, isTrue);
});This break focus in _ModalScopeState which sets canRequestFocus on the scope node and then rebuilds widget, which in turns sets the descendantsAreFocusable to false.
@gspencergoog, any thoughts? The focus code is quite convoluted, in this case the _FocusState.didUpdateWidget actually overrides property on FocusScopeNode, which is quite surprising.
Activity
knopp commentedon Apr 23, 2024
This is not a recent regression, but I think in our app we trigger rebuild in
_ModalScopeStatewhile thecanRequestFocusproperty is set tofalseon the scope focus node, which leaves the focus scope node withdescendantsAreFocusablepermanently set tofalse. I think the primary problem is demonstrated in the example.darshankawar commentedon Apr 24, 2024
Below is the console error log after running the provided test:
Above occurs on latest master and stable.
stable, master flutter doctor -v
LongCatIsLooong commentedon Apr 24, 2024
I think using the
Focus.withExternalFocusNode/FocusScope.withExternalFocusNodeconstructors would prevent the widget/element from modifying the node. Maybe the default constructors shouldn't take an external node?knopp commentedon Apr 24, 2024
My issue here is that it modifies completely unrelated property but doesn't set it back. And the behavior depends on whether the widget gets rebuilt or not, which is quite confusing. This actually causes a bug in modal scope:
flutter/packages/flutter/lib/src/widgets/routes.dart
Lines 1069 to 1075 in 65e6577
On line 1071,
_ModalScopeStatetemporarily setscanRequestFocusto false. Then, in unlucky scenario, when theFocusScopewidget of_ModalScopeStategets rebuilt whilecanRequestFocusis set to false, the node gets stuck withdescendantsAreFocusable == falseforever, essentially breaking focus in the application.So either
_ModalScopeState(and everyone else settingcanRequestFocus) must be aware that this can lead to disabling (but not reenabling)descendantsAreFocusableon the scope node, or the scope node should handle this better.knopp commentedon Apr 24, 2024
As for default constructor not taking a node, that would be a pretty major breaking change. Though TBH the
withExternalFocusNodeconstructor was always a bit confusing to me considering that default constructor also takes (optional) focus node.gspencergoog commentedon Apr 25, 2024
(Sorry, I was OOO for a few weeks) The history here is that we would very much like to get rid of the
nodeargument in the normal constructor, but it was too large of a breaking change to do. The addedwithExternalFocusconstructors are an attempt to provide the right functionality without breaking things. If I had it to do over again, we would have not includednodein the constructor.In any case, if you are modifying properties of the focus node after you supply it to the
FocusScopewidget, you should use thewithExternalFocusconstructor. Otherwise, theFocusScopewidget may end up overwriting them in ways you don't expect.knopp commentedon Apr 25, 2024
@gspencergoog, in this case it's not me modifying it, but modal route in the framework:
flutter/packages/flutter/lib/src/widgets/routes.dart
Line 1071 in 65e6577
Should it be fixed to use
withExternalFocusNode?Also, is there a valid use-case for specifying the node argument in default constructor rather than the
.withExternalFocusNodeconstructor? If there isn't any then maybe it would be worth considering the doing breaking change.gspencergoog commentedon Apr 25, 2024
If the only reason you're passing the focus node is to be able to call
requestFocusfrom someplace else, then it's valid to pass it in the default constructor.Yes, I think you're right. This line:
flutter/packages/flutter/lib/src/widgets/routes.dart
Line 1051 in 65e6577
should be fixed to use
withExternalFocusNode.I'll put together a PR.
b3nni97 commentedon Apr 25, 2024
Is this issue potentially related to issue #146844?
gspencergoog commentedon Apr 25, 2024
Potentially, since the focus node's focusability is predicated on whether or not it's in a user gesture (like a back swipe). It depends on what the root cause is, though.
gspencergoog commentedon Apr 25, 2024
@knopp I created #147390 , but I was unable to find a case to test where it made a difference. It is still the right way to do things, but since the route's
focusScopeNodeis never changed (is immutable), theFocusScopewidget shouldn't overwrite theskipTraversalorcanRequestFocusattributes anyhow.[-]FocusScopeNode.descendantsAreFocusable struck to false after disabling and enabling canRequestFocus[/-][+]`FocusScopeNode.descendantsAreFocusable` struck to false after disabling and enabling `canRequestFocus`[/+][-]`FocusScopeNode.descendantsAreFocusable` struck to false after disabling and enabling `canRequestFocus`[/-][+]`FocusScopeNode.descendantsAreFocusable` still set to false after disabling and enabling `canRequestFocus`[/+][-]`FocusScopeNode.descendantsAreFocusable` still set to false after disabling and enabling `canRequestFocus`[/-][+]`FocusScopeNode.descendantsAreFocusable` set to false after disabling and enabling `canRequestFocus`[/+]knopp commentedon Apr 26, 2024
@gspencergoog, I think something similar to #146844 should work? For us the problem also manifested when using the backswipe gesture on iOS. I'm guessing something there triggers the rebuild in the modal scope state (which updates the
FocusScopewidget which in turn disablesdescendantsAreFocusableon the node).gspencergoog commentedon Apr 26, 2024
@knopp Yeah, after I wrote that comment above, @b3nni97 mentioned that it fixed #146844, so it clearly is doing something that I should be able to test. Was this regression case only visible on iOS, or did it happen more generally?
Refactor route focus node creation (#147390)
Refactor route focus node creation (flutter#147390)