Open knopp opened 5 months ago
knopp
commented
5 months ago This is not a recent regression, but I think in our app we trigger rebuild in _ModalScopeState while the canRequestFocus property is set to false on the scope focus node, which leaves the focus scope node with descendantsAreFocusable permanently set to false. I think the primary problem is demonstrated in the example.
darshankawar
commented
5 months ago Below is the console error log after running the provided test:
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: true
Actual: <false>
When the exception was thrown, this was the stack:
#4 main.<anonymous closure> (file:///Users/dhs/Documents/NCFlutter/app_foo_stable/test/widget_test.dart:50:5)
<asynchronous suspension>
#5 testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:183:15)
<asynchronous suspension>
#6 TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1017:5)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)
This was caught by the test expectation on the following line:
file:///Users/dhs/Documents/NCFlutter/app_foo_stable/test/widget_test.dart line 50
The test description was:
FocusScope descendantsAreFocusable
════════════════════════════════════════════════════════════════════════════════════════════════════
Test failed. See exception logs above.
The test description was: FocusScope descendantsAreFocusableAbove occurs on latest master and stable.
LongCatIsLooong
commented
5 months ago I think using the Focus.withExternalFocusNode/FocusScope.withExternalFocusNode constructors would prevent the widget/element from modifying the node. Maybe the default constructors shouldn't take an external node?
knopp
commented
5 months ago 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:
On line 1071, _ModalScopeState temporarily sets canRequestFocus to false. Then, in unlucky scenario, when the FocusScope widget of _ModalScopeState gets rebuilt while canRequestFocus is set to false, the node gets stuck with descendantsAreFocusable == false forever, essentially breaking focus in the application.
So either _ModalScopeState (and everyone else setting canRequestFocus) must be aware that this can lead to disabling (but not reenabling) descendantsAreFocusable on the scope node, or the scope node should handle this better.
knopp
commented
5 months ago As for default constructor not taking a node, that would be a pretty major breaking change. Though TBH the withExternalFocusNode constructor was always a bit confusing to me considering that default constructor also takes (optional) focus node.
gspencergoog
commented
5 months ago (Sorry, I was OOO for a few weeks) The history here is that we would very much like to get rid of the node argument in the normal constructor, but it was too large of a breaking change to do. The added withExternalFocus constructors are an attempt to provide the right functionality without breaking things. If I had it to do over again, we would have not included node in the constructor.
In any case, if you are modifying properties of the focus node after you supply it to the FocusScope widget, you should use the withExternalFocus constructor. Otherwise, the FocusScope widget may end up overwriting them in ways you don't expect.
knopp
commented
5 months ago @gspencergoog, in this case it's not me modifying it, but modal route in the framework:
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 .withExternalFocusNode constructor? If there isn't any then maybe it would be worth considering the doing breaking change.
gspencergoog
commented
5 months ago Also, is there a valid use-case for specifying the node argument in default constructor rather than the .withExternalFocusNode constructor? If there isn't any then maybe it would be worth considering the doing breaking change.
If the only reason you're passing the focus node is to be able to call requestFocus from someplace else, then it's valid to pass it in the default constructor.
Should it be fixed to use withExternalFocusNode?
Yes, I think you're right. This line: https://github.com/flutter/flutter/blob/65e657799fb596bf39bdc1e62756fa1f9c416efe/packages/flutter/lib/src/widgets/routes.dart#L1051
should be fixed to use withExternalFocusNode.
I'll put together a PR.
b3nni97
commented
5 months ago Is this issue potentially related to issue #146844?
gspencergoog
commented
5 months ago Is this issue potentially related to issue https://github.com/flutter/flutter/issues/146844?
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
commented
5 months ago @knopp I created https://github.com/flutter/flutter/pull/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 focusScopeNode is never changed (is immutable), the FocusScope widget shouldn't overwrite the skipTraversal or canRequestFocus attributes anyhow.
knopp
commented
5 months ago @gspencergoog, I think something similar to https://github.com/flutter/flutter/issues/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 FocusScope widget which in turn disables descendantsAreFocusable on the node).
gspencergoog
commented
5 months ago @knopp Yeah, after I wrote that comment above, @b3nni97 mentioned that it fixed https://github.com/flutter/flutter/issues/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?
Happens with current main (3.22.0-16.0.pre.26).
Widget test to reproduce:
This break focus in
_ModalScopeStatewhich setscanRequestFocuson the scope node and then rebuilds widget, which in turns sets thedescendantsAreFocusableto false.@gspencergoog, any thoughts? The focus code is quite convoluted, in this case the
_FocusState.didUpdateWidgetactually overrides property onFocusScopeNode, which is quite surprising.