Exclude focus for hidden children in an IndexedStack

[skimm3]

Fixes: https://github.com/flutter/flutter/issues/114213 & https://github.com/flutter/flutter/issues/148405

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• [x] I signed the CLA.
• [x] I listed at least one issue that this PR fixes in the description above.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] I added new tests to check the change I am making, or this PR is test-exempt.
• [x] I followed the breaking change policy and added Data Driven Fixes where supported.
• [x] All existing and new tests are passing.

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

[Piinks]

Adding @chunhtai for context on this resolving the go_router issue

[skimm3]

Hi @skimm3 welcome! Thanks for contributing. Is there a test that verifies this fixes the go_router issue you have linked as well?

Thanks! I have a test that verifies the bug and that passes with my fix. Not sure what to do with it since the bug will be fixed when the user is using a flutter version containing this fix and not depending on a go_router version.

Here is the code for the test:

    testWidgets('Hidden children in a StatefulShellRoute can not receive focus', (WidgetTester tester) async {
      final GlobalKey<NavigatorState> rootNavigatorKey =
          GlobalKey<NavigatorState>();

      final List<RouteBase> routes = <RouteBase>[
        StatefulShellRoute.indexedStack(
          builder: (BuildContext context, GoRouterState state,
                  StatefulNavigationShell navigationShell) =>
              navigationShell,
          branches: <StatefulShellBranch>[
            StatefulShellBranch(routes: <GoRoute>[
              GoRoute(
                path: '/a',
                builder: (BuildContext context, GoRouterState state) =>
                    const Focus(child: Text('Screen A')),
              ),
            ]),
            StatefulShellBranch(routes: <GoRoute>[
              GoRoute(
                path: '/b',
                builder: (BuildContext context, GoRouterState state) =>
                    const Text('Screen B'),
              ),
            ]),
          ],
        ),
      ];

      final GoRouter router = await createRouter(routes, tester,
          initialLocation: '/a', navigatorKey: rootNavigatorKey);
      
      final Element screenA = tester.element(
        find.text('Screen A', skipOffstage: false),
      );
      final FocusNode screenAFocusNode = Focus.of(screenA);
      screenAFocusNode.requestFocus();
      await tester.pump();

      expect(screenAFocusNode.hasFocus, true);

      router.go('/b');
      await tester.pumpAndSettle();

      screenAFocusNode.requestFocus();
      await tester.pump();

      expect(screenAFocusNode.hasFocus, false);
    });

[Piinks]

Not sure what to do with it since the bug will be fixed when the user is using a flutter version containing this fix and not depending on a go_router version.

Hmm good question. @chunhtai do you think we should add this test to go_router after this lands to prevent regressions there?

[chunhtai]

I think we can create a simple test with indexed stack to make sure it hides focus of child underneath since this is the root cause of the go_router issue

[auto-submit[bot]]

auto label is removed for flutter/flutter/159133, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

[victorsanni]

Currently taking a look at the google testing failures.

[Piinks]

This may be a breaking change if customers have been utilizing the focusability of children in the IndexedStack regardless of their visibility. An option to make this non-breaking would be to make the new behavior opt in through a flag on IndexedStack.

[goderbauer]

@skimm3 Do you have plans to come back to this PR to address the feedback given above (i.e. making this non-breaking by making the new behavior opt-in via a flag)?

[skimm3]

Hey @Piinks & @goderbauer! Pushed a new version with a flag that makes new functionality optional. This means this PR no longer fixes https://github.com/flutter/flutter/issues/148405 since go_router needs to use the new flag.

Called the flag maintainFocusability since hidden children in an IndexedStack is not tappable the same way a hidden child in Visibility is with maintainInteractivity set it. Verified this behavior with two tests.

[Piinks]

Rebasing here to start a new run for Google Testing

[flutter-dashboard[bot]]

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[justinmc]

Looks like there is one real failure in the Google tests, a "No element" error. Not sure if that rings a bell for anyone that has worked on this.

[Renzo-Olivares]

Rebased to get google testing running again. Did you still want to continue working on this pull request @skimm3 pending the google testing results? Thank you for your patience on this one.

[Renzo-Olivares]

Google testing is failing because a use-case relies on tapping a widget to make another widget wrapped with Visibility visible. Tapping on widget A, request focus on widget B which is wrapped in a Visibility with visible depending on the status of its focus. Because after this PR Visibility now includes an ExcludeFocus when maintainInteractivity is false the focus request does not work on widget B, so the widget never becomes visible.

I think adding a maintainFocusability flag to Visibility as pointed out by @skimm3 would be a good way forward considering this linked issue affects anyone using Visibility and not just IndexedStack.

[Piinks]

Removing from the Google testing queue since this appears to now be non breaking, it is in my re-review queue

[Renzo-Olivares]

Looks like the analyzer is failing, you can run dart format on stack_test.dart to fix this.

[Renzo-Olivares]

Rebasing to get google testing running again.

[Renzo-Olivares]

Rebasing again looks like google testing timed out.

[justinmc]

@Renzo-Olivares Can this merge?

[Renzo-Olivares]

@justinmc Just needs the doc suggestions from the latest review to be addressed.

[Renzo-Olivares]

Rebasing to get google testing running again.

[auto-submit[bot]]

autosubmit label was removed for flutter/flutter/159133, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

[Renzo-Olivares]

Trying another rebase to get Google testing going.