Skip to content

Conversation

@simolus3
Copy link
Contributor

An IndexedStack only ever shows one of its children. Other children aren't rendered or hit-tested. In this sense, I think these other children could be considered off-stage.
This PR changes the behavior of IndexedStack to hide inactive children from an element visitor only interested in on-stage children. The main effect is that inactive children are no longer found through a finder in widget tests by default.

This closes #111478.

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.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Sep 13, 2022
@simolus3 simolus3 force-pushed the indexed-stack-hide-from-finder branch from 212f9c0 to f12fca4 Compare September 13, 2022 14:01
@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 13, 2022

IMHO, IndexedStack says: https://api.flutter.dev/flutter/widgets/IndexedStack-class.html

The stack is always as big as the largest child.

But the offstage property, which IMHO is quite related to Offstage, says: https://api.flutter.dev/flutter/widgets/Offstage-class.html

A widget that lays the child out as if it was in the tree, but ... without taking any room in the parent.

So, I am worried whether making IndexedStack's inactive children offstage will violate its definition?

@simolus3
Copy link
Contributor Author

Thanks for the feedback!

But the offstage property, which IMHO is quite related to Offstage

On the other hand, the documentation of debugVisitOnstageChildren explicitly mentions Overlay as well:

Classes like Offstage and Overlay override this method to hide their children.

To me, that comment left the impression that using an Offstage widget isn't a requirement for an element being considered offstage. That documentation comment further says:

For example, when you instruct the test framework to tap on a widget, by default the finder will look for onstage elements and ignore the offstage ones.

I think this may be a reasonable behavior for IndexedStacks as well. Do you think this might be worth an opt-in flag on IndexedStack instead?

@goderbauer goderbauer requested a review from Piinks September 13, 2022 22:05
@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 13, 2022

Aha that sounds pretty reasonable to me. So Flutter's "offstage" seems to at least has two meanings, one narrow and one broad.

@Piinks Piinks added the a: tests "flutter test", flutter_test, or one of our tests label Sep 14, 2022
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha that sounds pretty reasonable to me. So Flutter's "offstage" seems to at least has two meanings, one narrow and one broad.

This sounds like something that should definitely be clarified in the docs here if that is the case. It sounds like there are a couple of opportunities listed in the comments here where the docs are not very helpful or clear.

There are currently failing tests for this PR, can you take a look?

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 14, 2022

@Piinks This sounds like something that should definitely be clarified in the docs here if that is the case. It sounds like there are a couple of opportunities listed in the comments here where the docs are not very helpful or clear.

#111524 trys to explain a little bit :)

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am running this through additional testing to see if it breaks anything. I think this is a fair change, but there may be some customers broken by it.

@Piinks
Copy link
Contributor

Piinks commented Sep 20, 2022

This did break some customer tests that expect the current behavior.

@simolus3 is there another way we could achieve this? Or a non breaking way we could rework this?

Check out our breaking change policy, and let me know what your thoughts are if you would like to proceed with this change. Thanks!

@Piinks Piinks added the c: API break Backwards-incompatible API changes label Sep 20, 2022
@simolus3
Copy link
Contributor Author

Or a non breaking way we could rework this?

In c2dd76d, I have made this behavior opt-in. An IndexedStack will continue to behave like it did unless the hiddenChildrenAreOnstage field is disabled.

@Piinks
Copy link
Contributor

Piinks commented Oct 3, 2022

I chatted with @goderbauer about this recently, and I think this is something we should add to #24722
While your original proposal makes more sense, it is too breaking to change the API as it currently is. I don't know that the opt in flag approach is very beneficial here. I really appreciate you looking into this. I will add this to the list in the linked issue. At this time, I don't think we can accept this change.
Thank you for contributing!

@simolus3
Copy link
Contributor Author

simolus3 commented Oct 5, 2022

Thanks for the explanation and for taking a look!

@simolus3 simolus3 closed this Oct 5, 2022
@fzyzcjy
Copy link
Contributor

fzyzcjy commented Mar 10, 2023

Today I realize Flutter is inconsistent here: RenderIndexedStack.debugDescribeChildren says those children are offstage. Thus, when users printing the debug renderobject tree, it will be very surprising - because it is marked as offstage but it is not matched by a offstage finder!

  @override
  List<DiagnosticsNode> debugDescribeChildren() {
    final List<DiagnosticsNode> children = <DiagnosticsNode>[];
    int i = 0;
    RenderObject? child = firstChild;
    while (child != null) {
      children.add(child.toDiagnosticsNode(
        name: 'child ${i + 1}',
        style: i != index ? DiagnosticsTreeStyle.offstage : null,
      ));
      child = (child.parentData! as StackParentData).nextSibling;
      i += 1;
    }
    return children;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests c: API break Backwards-incompatible API changes framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Widget test] Make IndexedStack hide non-active children from finder

3 participants