Exclude focus for child when it is hidden in Visibility if maintainInteractivity is not set, add possibility to set maintainInteractivity in IndexedStack #159133

skimm3 · 2024-11-19T08:22:54Z

Fixes: #114213 and is a prerequisite for fixing: #148405

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

Piinks · 2024-11-22T18:05:31Z

Adding @chunhtai for context on this resolving the go_router issue

Piinks

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

chunhtai · 2024-11-22T18:11:22Z

packages/flutter/lib/src/widgets/basic.dart

-      return Visibility.maintain(
-        visible: i == index,
-        child: children[i],
+      return ExcludeFocus(


This feels something should be added to Visibility widget.

Also it seems it should not use Visibility.maintain since the maintain by default allows interactivity and semantics and animation unecessarily it should use

Visibility( visible: i == index, maintainSize: true, maintainState: true, child: children[i] )

Thanks for the feedback! Fixed the issue in Visibility widget instead. The new solution makes Visibility ignore focus when child is hidden for all combinations of maintain-parameters. I'm a bit unsure if it should be ignored when maintainInteractivity is true, what do you think?

Removed maintain interactivity and semantics from indexed stack and added a test. Could not remove maintainAnimation since that is needed if maintainSize is set.

It should ignore when maintainInteractivity is true since focus is part of interaction

skimm3 · 2024-11-24T10:22:53Z

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 · 2024-11-25T16:44:27Z

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 · 2024-11-25T17:56:33Z

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

packages/flutter/test/widgets/stack_test.dart

chunhtai

LGTM, thanks for fixing the issue!!

Piinks

LGTM too! 🎊 Thanks!

auto-submit · 2024-12-09T21:38:57Z

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 · 2024-12-09T21:46:48Z

Currently taking a look at the google testing failures.

Piinks · 2024-12-09T21:50:28Z

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 · 2025-01-07T23:22:08Z

@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 · 2025-01-08T12:35:49Z

Hey @Piinks & @goderbauer!
Pushed a new version with a flag that makes new functionality optional. This means this PR no longer fixes #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

Thank you for the update, and for your patience over the holidays. 🙏

Piinks · 2025-01-14T23:51:12Z

packages/flutter/lib/src/widgets/basic.dart

@@ -4134,6 +4135,11 @@ class IndexedStack extends StatelessWidget {
  /// See [Stack.fit] for more information.
  final StackFit sizing;

+  /// If hidden [children] should be able to receive focus.


Suggested change

/// If hidden [children] should be able to receive focus.

/// Whether or not [children] in the stack that are not visible should be able to receive focus.

Piinks · 2025-01-14T23:51:43Z

packages/flutter/lib/src/widgets/basic.dart

-      return Visibility.maintain(visible: i == index, child: children[i]);
+      return Visibility(
+        visible: i == index,
+        maintainInteractivity: maintainFocusability,


Maybe we can match with the same name for consistency? :)

Suggested change

maintainInteractivity: maintainFocusability,

maintainInteractivity: maintainInteractivity,

Piinks · 2025-01-14T23:52:35Z

packages/flutter/lib/src/widgets/visibility.dart

@@ -245,7 +246,10 @@ class Visibility extends StatelessWidget {

  @override
  Widget build(BuildContext context) {
-    Widget result = child;
+    Widget result = ExcludeFocus(
+      excluding: !visible && !maintainInteractivity,


This makes me wonder if this PR should be split into two changes, since this changes the behavior of the Visibility widget.

Can you share some context for why this change was added? Is there a separate problem with Visibility this is solving?

Thanks for the great feedback, resolved the other comments.

Reason for adding the change to Visibility:
In the first iteration of this PR I added the ExcludeFocus widget to IndexedStack. That solved the original problem but I got feedback from chunhtai that the fix should probably be in Visibility, since the root cause of the bug was that Visibility did not handle focus correctly for hidden children. I then moved my implementation there to make the fix more general and not only cover IndexedStack but other widgets that might have the same issue.

On splitting PR into two:
I could do that but the bug would only be solved after both PR lands, it seems easier to keep it in one. I'm also unsure how to practically split them. One way would be to file a new issue that describes the bug in Visibility in which hidden children can receive focus if maintainInteractivity is false. After that issue is fixed I could go back to this and "only" add the flag in IndexedStack.

Yeah I see what you mean. My hesitation is that this change to visibility does not just have to do with maintainInteractivity, visible is also part of it. We haven't seen any regressions in the tests though, so I think it is ok.

packages/flutter/lib/src/widgets/basic.dart

Piinks · 2025-01-31T22:55:36Z

packages/flutter/lib/src/widgets/basic.dart

@@ -4134,6 +4135,11 @@ class IndexedStack extends StatelessWidget {
  /// See [Stack.fit] for more information.
  final StackFit sizing;

+  /// Wether or not [children] in the stack that are not visible should be able to receive focus.


Suggested change

/// Wether or not [children] in the stack that are not visible should be able to receive focus.

/// Whether or not [children] in the stack that are not visible should be able to receive focus.

If that was my typo from an earlier review, my bad. 😅

it has been a while since my last review. but may I know what's the use case that people would want the interactivity on the hidden children?

The reason for adding it was to not make a breaking change since that is the current behavior. I would argue that since it is a bug and not expected behavior it is fine to not be backwards compatible in this case but that's for you to decide since this is my first contribution and I'm not really sure on how you go about these things.

Removing the flag would fix the bug in go_router directly instead of having to wait for go_router to use the flutter version with this fix and then add the flag. But I also understand that breaking changes are not wanted :)

ah ok, I saw the previous comment as well, I think this is probably due to a lot of internal test failures. yeah making this opt in for now sgtm, can you add a todo at this line explain we should remove this at some day and this flag is for backward compatibility

Piinks · 2025-01-31T22:57:38Z

packages/flutter/lib/src/widgets/visibility.dart

@@ -245,7 +246,10 @@ class Visibility extends StatelessWidget {

  @override
  Widget build(BuildContext context) {
-    Widget result = child;
+    Widget result = ExcludeFocus(
+      excluding: !visible && !maintainInteractivity,


Yeah I see what you mean. My hesitation is that this change to visibility does not just have to do with maintainInteractivity, visible is also part of it. We haven't seen any regressions in the tests though, so I think it is ok.

chunhtai

Sorry if my comments have been addressed before, it has been a long time

chunhtai · 2025-02-13T21:17:16Z

packages/flutter/lib/src/widgets/basic.dart

@@ -4134,6 +4135,11 @@ class IndexedStack extends StatelessWidget {
  /// See [Stack.fit] for more information.
  final StackFit sizing;

+  /// Wether or not [children] in the stack that are not visible should be able to receive focus.


it has been a while since my last review. but may I know what's the use case that people would want the interactivity on the hidden children?

chunhtai · 2025-02-13T21:19:41Z

packages/flutter/lib/src/widgets/basic.dart

+        maintainInteractivity: maintainInteractivity,
+        maintainSize: true,
+        maintainState: true,
+        maintainAnimation: true,


This is abit surprising, why do we want to keep the animation for if they are not visible?

It's due to an assertion in Visibility that requires maintainAnimation to be set if maintainSize or maintainState is true.

thanks for explanation sgtm

Piinks · 2025-02-24T20:23:02Z

Rebasing here to start a new run for Google Testing

…emantics in indexedstack, add tests

Piinks · 2025-02-26T00:15:08Z

packages/flutter/lib/src/widgets/visibility.dart

@@ -245,7 +246,7 @@ class Visibility extends StatelessWidget {

  @override
  Widget build(BuildContext context) {
-    Widget result = child;
+    Widget result = ExcludeFocus(excluding: !visible && !maintainInteractivity, child: child);


It looks like this change is breaking, I have looked into the google testing failure and it does break expected existing behavior there. Does this need to factor in whether or not it is visible?

If it does not factor in visible it will exclude focus even if the child is visible which I do not think is wanted.

To not break any existing behavior we could add a new flag, maintainFocusability, that decides wether we should exclude focus for hidden children. Another solution is to revert my changes to Visibility and add ExcludeFocus in IndexedStack instead. That however is more of a band aid fix since the problem was that hidden children in Visibility can receive focus. I also remember @chunhtai suggested against the the second option.

Btw, thanks a lot for being patient and helping me out!

flutter-dashboard · 2025-03-18T17:07:30Z

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.

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 19, 2024

Piinks self-requested a review November 19, 2024 23:28

victorsanni self-requested a review November 19, 2024 23:29

Piinks requested a review from chunhtai November 22, 2024 18:05

Piinks added f: routes Navigator, Router, and related APIs. f: focus Focus traversal, gaining or losing focus p: go_router The go_router package labels Nov 22, 2024

Piinks reviewed Nov 22, 2024

View reviewed changes

chunhtai reviewed Nov 22, 2024

View reviewed changes

skimm3 force-pushed the exclude-focus-for-hidden-children-in-indexed-stack branch from bc8c4fe to 025bee5 Compare November 24, 2024 10:04

github-actions bot removed f: routes Navigator, Router, and related APIs. f: focus Focus traversal, gaining or losing focus labels Nov 24, 2024

victorsanni reviewed Nov 25, 2024

View reviewed changes

packages/flutter/test/widgets/stack_test.dart Outdated Show resolved Hide resolved

skimm3 force-pushed the exclude-focus-for-hidden-children-in-indexed-stack branch 2 times, most recently from bfc5c95 to 651d851 Compare November 29, 2024 06:41

skimm3 requested a review from chunhtai November 29, 2024 06:48

skimm3 changed the title ~~Exclude focus for hidden children in an IndexedStack~~ Exclude focus for child when it is hidden in Visibility if maintainInteractivity is not set Nov 29, 2024

chunhtai approved these changes Dec 9, 2024

View reviewed changes

chunhtai requested review from victorsanni and Piinks December 9, 2024 19:59

victorsanni approved these changes Dec 9, 2024

View reviewed changes

Piinks approved these changes Dec 9, 2024

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 9, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 9, 2024

skimm3 force-pushed the exclude-focus-for-hidden-children-in-indexed-stack branch from 651d851 to 49b0f60 Compare January 8, 2025 12:30

skimm3 requested a review from Piinks January 8, 2025 12:36

Piinks reviewed Jan 14, 2025

View reviewed changes

skimm3 force-pushed the exclude-focus-for-hidden-children-in-indexed-stack branch from 49b0f60 to 73432bb Compare January 18, 2025 10:00

chunhtai self-requested a review January 21, 2025 16:47

victorsanni reviewed Jan 21, 2025

View reviewed changes

packages/flutter/lib/src/widgets/basic.dart Outdated Show resolved Hide resolved

Piinks reviewed Jan 31, 2025

View reviewed changes

Piinks self-assigned this Feb 6, 2025

chunhtai reviewed Feb 13, 2025

View reviewed changes

skimm3 force-pushed the exclude-focus-for-hidden-children-in-indexed-stack branch 2 times, most recently from 6801038 to e64e210 Compare February 15, 2025 07:47

skimm3 changed the title ~~Exclude focus for child when it is hidden in Visibility if maintainInteractivity is not set~~ Exclude focus for child when it is hidden in Visibility if maintainInteractivity is not set, add possibility to set maintainInteractivity in IndexedStack Feb 15, 2025

skimm3 force-pushed the exclude-focus-for-hidden-children-in-indexed-stack branch from 6c7ace8 to 1776a01 Compare February 15, 2025 20:47

skimm3 added 10 commits February 24, 2025 14:23

Exclude focus for hidden children in an IndexedStack

137b61e

move exclude focus to visibility, do not maintain interactivity and s…

2c42266

…emantics in indexedstack, add tests

do not exclude focus when maintainInteractivity is set

8c06837

add missing trailing comma

2cca47f

add maintainFocusability flag to IndexedStack

e7ae155

resolve pr feedback

15e91cc

simplify maintainInteractivity docs

11de6e6

add todo to maintainInteractivity flag

c795061

format todo comment

221173b

fix formatting

24bacae

Piinks force-pushed the exclude-focus-for-hidden-children-in-indexed-stack branch from 1776a01 to 24bacae Compare February 24, 2025 20:23

Piinks reviewed Feb 26, 2025

View reviewed changes

	/// If hidden [children] should be able to receive focus.
	/// Whether or not [children] in the stack that are not visible should be able to receive focus.

	maintainInteractivity: maintainFocusability,
	maintainInteractivity: maintainInteractivity,

	/// Wether or not [children] in the stack that are not visible should be able to receive focus.
	/// Whether or not [children] in the stack that are not visible should be able to receive focus.

Exclude focus for child when it is hidden in Visibility if maintainInteractivity is not set, add possibility to set maintainInteractivity in IndexedStack #159133

Are you sure you want to change the base?

Exclude focus for child when it is hidden in Visibility if maintainInteractivity is not set, add possibility to set maintainInteractivity in IndexedStack #159133

Conversation

skimm3 commented Nov 19, 2024 • edited Loading

Pre-launch Checklist

Piinks commented Nov 22, 2024

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

skimm3 commented Nov 24, 2024

Piinks commented Nov 25, 2024

chunhtai commented Nov 25, 2024

chunhtai left a comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

auto-submit bot commented Dec 9, 2024

victorsanni commented Dec 9, 2024

Piinks commented Dec 9, 2024

goderbauer commented Jan 7, 2025

skimm3 commented Jan 8, 2025

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Mar 18, 2025

skimm3 commented Nov 19, 2024 •

edited

Loading

chunhtai Nov 25, 2024 •

edited

Loading

chunhtai Feb 14, 2025 •

edited

Loading