Skip to content
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

Closed
tvolkert opened this issue Sep 16, 2023 · 7 comments · Fixed by #134954, #136773, #136894 or #136898
Closed

Regression: ability to call nextFocus() on a node to focus its descendant #134854

tvolkert opened this issue Sep 16, 2023 · 7 comments · Fixed by #134954, #136773, #136894 or #136898
Assignees
Labels
c: regression It was better in the past than it is now f: focus Focus traversal, gaining or losing focus found in release: 3.13 Found to occur in 3.13 found in release: 3.16 Found to occur in 3.16 framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list

Comments

@tvolkert
Copy link
Contributor

The following test case was broken by #130812

testWidgets("Requesting nextFocus on node focuses its descendant", (WidgetTester tester) async {
  for (bool canRequestFocus in <bool>{true, false}) {
    final FocusNode node1 = FocusNode();
    final FocusNode node2 = FocusNode();
    await tester.pumpWidget(
      Directionality(
        textDirection: TextDirection.ltr,
        child: FocusTraversalGroup(
          policy: ReadingOrderTraversalPolicy(),
          child: FocusScope(
            child: Focus(
              focusNode: node1,
              canRequestFocus: canRequestFocus,
              child: Focus(
                focusNode: node2,
                child: Container(),
              ),
            ),
          ),
        ),
      ),
    );

    final bool didFindNode = node1.nextFocus();
    await tester.pump();
    expect(didFindNode, isTrue);
    expect(node1.hasPrimaryFocus, isFalse);
    expect(node2.hasPrimaryFocus, isTrue);
  }
});

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

@tvolkert tvolkert added c: regression It was better in the past than it is now framework flutter/packages/flutter repository. See also f: labels. f: focus Focus traversal, gaining or losing focus P1 High-priority issues at the top of the work list labels Sep 16, 2023
@chunhtai chunhtai self-assigned this Sep 18, 2023
chunhtai added a commit that referenced this issue Sep 18, 2023
…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
@github-actions
Copy link

github-actions bot commented Oct 2, 2023

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 doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2023
Mairramer pushed a commit to Mairramer/flutter that referenced this issue Oct 10, 2023
…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
@tvolkert
Copy link
Contributor Author

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 focusRightEntry()

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)" : ""}'),
        ),
      ),
    );
  }
}

@tvolkert tvolkert reopened this Oct 14, 2023
@flutter-triage-bot flutter-triage-bot bot unlocked this conversation Oct 14, 2023
@tvolkert
Copy link
Contributor Author

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);
  });
}

@tvolkert tvolkert added found in release: 3.13 Found to occur in 3.13 found in release: 3.16 Found to occur in 3.16 labels Oct 15, 2023
@HansMuller
Copy link
Contributor

CC @gspencergoog

@chunhtai
Copy link
Contributor

It seems to me when calling outer2.nextFocus(); should focus outer2. we have similar test to check this behavior.

https://github.com/flutter/flutter/blame/cdc40b52260c57923192fcf9fc96718d0b07a44d/packages/flutter/test/widgets/focus_traversal_test.dart#L90

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

@chunhtai
Copy link
Contributor

oh sorry, I missed that the outer1 and outer2 skipTraversal:true. yes this is a bug

auto-submit bot pushed a commit that referenced this issue Oct 23, 2023
#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
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this issue Oct 31, 2023
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
Copy link

github-actions bot commented Nov 6, 2023

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 doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2023
chunhtai added a commit to chunhtai/flutter that referenced this issue Nov 7, 2023
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
XilaiZhang pushed a commit to XilaiZhang/flutter that referenced this issue Nov 7, 2023
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
auto-submit bot pushed a commit that referenced this issue Nov 7, 2023
…ode to focus its desc… (#138014)

�� (#136898)

�endant" (#136894)"

This reverts commit c2bd2c1.

fixes #134854

Context: #137071. Updated from [commit in chunhtai/flutter](chunhtai@adacf22)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: regression It was better in the past than it is now f: focus Focus traversal, gaining or losing focus found in release: 3.13 Found to occur in 3.13 found in release: 3.16 Found to occur in 3.16 framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list
Projects
None yet
3 participants