-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Treat hidden IndexedStack children as offstage for test finder
#111479
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
Conversation
212f9c0 to
f12fca4
Compare
|
IMHO, IndexedStack says: https://api.flutter.dev/flutter/widgets/IndexedStack-class.html
But the offstage property, which IMHO is quite related to Offstage, says: https://api.flutter.dev/flutter/widgets/Offstage-class.html
So, I am worried whether making IndexedStack's inactive children offstage will violate its definition? |
|
Thanks for the feedback!
On the other hand, the documentation of
To me, that comment left the impression that using an
I think this may be a reasonable behavior for |
|
Aha that sounds pretty reasonable to me. So Flutter's "offstage" seems to at least has two meanings, one narrow and one broad. |
Piinks
left a comment
There was a problem hiding this 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?
Piinks
left a comment
There was a problem hiding this 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.
|
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! |
In c2dd76d, I have made this behavior opt-in. An |
|
I chatted with @goderbauer about this recently, and I think this is something we should add to #24722 |
|
Thanks for the explanation and for taking a look! |
|
Today I realize Flutter is inconsistent here: @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; |
An
IndexedStackonly 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
IndexedStackto 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 afinderin widget tests by default.This closes #111478.
Pre-launch Checklist
///).