It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

Hmm, I see. I agree, it looks like the path of trying to fix the underlying bug has been explored, and it's appropriate to apply a workaround.

In that case what that point reduces to is: this should have a comment linking to the issue, namely #111478 .

And then the high-level question for this PR is how to balance:

• Showing these children as offstage is confusing, because they're not recognized as offstage by a test finder.
• Showing these children as onstage is confusing, because they're not onstage in that they're not rendered or hit-tested.

In other words, #111478 means that the behavior outside of this method is inconsistent, and so whichever way it picks will fail to match some of that behavior.

I don't feel informed enough about this area to have a view on which of those is least bad.

I wonder if it could make sense to add into this tree dump a little note about this mismatch? Perhaps by adding a child that's made with [DiagnosticsNode.message]. The note could convey the information either of "btw these offstage children won't match an offstage finder", or "btw most of these children aren't rendered or hit-tested".

Relatedly, it seems like knowing which child is the currently truly-onstage one will often be pretty important information. So if that isn't conveyed by the two different styles, it should be conveyed some other way.

this should have a comment linking to the issue, namely #111478

done

I wonder if it could make sense to add into this tree dump a little note about this mismatch? Perhaps by adding a child that's made with [DiagnosticsNode.message].

Yes I think so, that would be helpful

I don't feel informed enough about this area to have a view on which of those is least bad.

Just ping me when the decision is made :)

I agree with the previous post that this just moves the confusion to a different place and therefore I am leaning towards not making this fix. It is odd to "break" something that used to work just to make it consistent with something we know is broken.

We could explore how breaking making the real fix is again. Unfortunately, the previous attempt at fixing this didn't note how many google targets were effected by this. If it is just a few, we could potentially hand-fix them and write a migration guide. If someone posts a PR with the real fix, I can run the internal tests again.

That looks reasonable! @simolus3 hi could you please re-create the PR as suggested above? (I can do so but think it is impolite to "steal" others' code)

I've opened #123129 again.

Thanks. I am going to close this one in the meantime.