-
Notifications
You must be signed in to change notification settings - Fork 27.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regression: ability to call nextFocus() on a node to focus its descendant #134854
Comments
…134954) fixes #134854 ## 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 this 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]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
…lutter#134954) fixes flutter#134854 ## 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 this 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]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
Something's still not right with focus traversal, and I bisect it back to #130812 The problem that persists (as of bcc795a) is demonstrated in the following app. Specifically, see the comment in import 'package:flutter/widgets.dart';
void main() async {
runApp(const BugReport());
}
class BugReport extends StatefulWidget {
const BugReport({super.key});
@override
State<BugReport> createState() => _BugReportState();
}
class _BugReportState extends State<BugReport> {
late final FocusNode leftFocusNode;
late final FocusNode rightFocusNode;
void _onFocusChanged() {
debugPrint('Focus changed to ${FocusManager.instance.primaryFocus?.debugLabel}');
}
void focusRightEntry() {
setState(() {
/// The [Focus] widget tied to [rightFocusNode] is marked as [skipTraversal],
/// so this call shouldn't directly focus [rightFocusNode], but rather focus
/// its first descendant node.
///
/// Thus, in the logs, due to the [_onFocusChanged] handler, we should see
/// a message that "Focus changed to innerFocusNode::right". This was the
/// case before #130812, but after that change, we see a message that
/// "Focus changed to rightFocusNode", which shouldn't happen since this
/// triggers an explicit call to [FocusTraversalPolicy.next]
rightFocusNode.nextFocus();
});
}
@override
void initState() {
super.initState();
leftFocusNode = FocusNode(debugLabel: 'leftFocusNode');
rightFocusNode = FocusNode(debugLabel: 'rightFocusNode');
FocusManager.instance.addListener(_onFocusChanged);
}
@override
void dispose() {
FocusManager.instance.removeListener(_onFocusChanged);
leftFocusNode.dispose();
rightFocusNode.dispose();
super.dispose();
}
@override
Widget build(BuildContext context) {
return Localizations(
delegates: const [DefaultWidgetsLocalizations.delegate],
locale: const Locale('en'),
child: FocusTraversalGroup(
child: Row(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: [
FocusScope(
child: Focus(
focusNode: leftFocusNode,
skipTraversal: true,
child: Padding(
padding: const EdgeInsets.all(50),
child: FocusableArea(
autofocus: true,
onTap: focusRightEntry,
debugLabel: 'left',
text: 'Click to transfer focus',
),
),
),
),
FocusScope(
child: Focus(
focusNode: rightFocusNode,
skipTraversal: true,
child: const Padding(
padding: EdgeInsets.all(50),
child: FocusableArea(
debugLabel: 'right',
text: 'Should gain focus when you click the other one',
),
),
),
),
],
),
),
);
}
}
class FocusableArea extends StatefulWidget {
const FocusableArea({
super.key,
this.autofocus = false,
this.onTap,
this.debugLabel,
required this.text,
});
final bool autofocus;
final VoidCallback? onTap;
final String? debugLabel;
final String text;
@override
State<FocusableArea> createState() => _FocusableAreaState();
}
class _FocusableAreaState extends State<FocusableArea> {
late final FocusNode _focusNode;
bool _hasFocus = false;
void _handleFocusChanged(bool hasFocus) {
setState(() {
_hasFocus = hasFocus;
});
}
@override
void initState() {
super.initState();
_focusNode = FocusNode(debugLabel: 'innerFocusNode::${widget.debugLabel}');
}
@override
void dispose() {
_focusNode.dispose();
super.dispose();
}
@override
Widget build(BuildContext context) {
return Focus(
onFocusChange: _handleFocusChanged,
autofocus: widget.autofocus,
focusNode: _focusNode,
child: GestureDetector(
onTap: widget.onTap,
child: Padding(
padding: const EdgeInsets.fromLTRB(20, 10, 10, 10),
child: Text('${widget.text}${_hasFocus ? " (FOCUSED)" : ""}'),
),
),
);
}
} |
I've distilled the app above into a unit test that shows the problem. This unit test passed at commit 9c8ee4f and fails (and has failed since) at commit 4763be7 void main() {
testWidgets('calling', (WidgetTester tester) async {
final FocusNode outer1 = FocusNode(debugLabel: 'outer1', skipTraversal: true);
final FocusNode outer2 = FocusNode(debugLabel: 'outer2', skipTraversal: true);
final FocusNode inner1 = FocusNode(debugLabel: 'inner1', );
final FocusNode inner2 = FocusNode(debugLabel: 'inner2', );
addTearDown(() {
outer1.dispose();
outer2.dispose();
inner1.dispose();
inner2.dispose();
});
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: FocusTraversalGroup(
child: Row(
children: <Widget>[
FocusScope(
child: Focus(
focusNode: outer1,
child: Focus(
focusNode: inner1,
child: const SizedBox(width: 10, height: 10),
),
),
),
FocusScope(
child: Focus(
focusNode: outer2,
// This [Padding] widget is substantive to the test!
// In https://github.com/flutter/flutter/issues/134854,
// the regression wasn't seen (and this test passed) without
// the [Padding] widget in the tree.
child: Padding(
padding: const EdgeInsets.all(5),
child: Focus(
focusNode: inner2,
child: const SizedBox(width: 10, height: 10),
),
),
),
),
],
),
),
),
);
expect(FocusManager.instance.primaryFocus, isNull);
inner1.requestFocus();
await tester.pump();
expect(FocusManager.instance.primaryFocus, inner1);
outer2.nextFocus();
await tester.pump();
expect(FocusManager.instance.primaryFocus, inner2);
});
} |
It seems to me when calling The fact that remove the Padding will cause the result to change is a bug in the code. I will put up a pr to fix it |
oh sorry, I missed that the outer1 and outer2 skipTraversal:true. yes this is a bug |
#136898) �endant" (#136894)" This reverts commit c2bd2c1. fixes #134854 This is a straight reland, the internal test is testing a wrong behave. https://critique.corp.google.com/cl/575028981
flutter#136898) �endant" (flutter#136894)" This reverts commit c2bd2c1. fixes flutter#134854 This is a straight reland, the internal test is testing a wrong behave. https://critique.corp.google.com/cl/575028981
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
flutter#136898) �endant" (flutter#136894)" This reverts commit c2bd2c1. fixes flutter#134854 This is a straight reland, the internal test is testing a wrong behave. https://critique.corp.google.com/cl/575028981
flutter#136898) �endant" (flutter#136894)" This reverts commit c2bd2c1. fixes flutter#134854 This is a straight reland, the internal test is testing a wrong behave. https://critique.corp.google.com/cl/575028981
The following test case was broken by #130812
Before this change, callers could call
FocusNode.nextFocus()
to focus a descendant node. #130812 caused a regression whereby this no longer works.We should check in this test and get it passing again :-)
Marking as P1 because it's a regression, but feel free to drop down to P2 if I'm misreading this.
@chunhtai
The text was updated successfully, but these errors were encountered: