It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

CC @victorsanni from triage.

Previous fix deemed too breaking: #137874

So, just to get this straight:
This PR might break a bunch of internal Google code?
And I have to also handle infinity?

So, just to get this straight: This PR might break a bunch of internal Google code?

See this comment. I'm curious what the internal failures looked like.

And I have to also handle infinity?

To me _additionalConstraints.constrainHeight(height), makes more sense.

See this comment. I'm curious what the internal failures looked like.

That is exactly the comment I am referring to. Would really like to know too.

To me _additionalConstraints.constrainHeight(height), makes more sense.

I believe I tried that and got to the conclusion I didn't want to clamp the height and width, but rather just constrain max size.
Can't remember why right now... It will probably come back to me in due time.
But what I can say is that the test added in #72095 is passing with my max implementation, and fails using _additionalConstraints.constrainX.

To me _additionalConstraints.constrainHeight(height), makes more sense.

I looked into the internal test failures from the other PR, now I'm convinced that using min is correct here since _additionalConstraints is ignored if it doesn't satisfy the incoming constraints (_additionalConstraints.enforce(constraints)).

Looks like this change still breaks internal golden tests but most of these changes look like improvements so far. There are a couple of golden changes that I need to further investigate.

Hmm there's a legit internal failure. The problem is that, if the parent that has a set of tight constraints, a child RenderConstainedBox is a no-op.

For example, in a render tree like this:

RenderBox(   // A RenderBox that gives tight width constraints to its child. In the failing test it was a Column with `CrossAxisAlignment.stretch`.
  constraints: BoxConstraints.tight(width: x),
  child: RenderConstrainedBox(
    additionalConstraints: BoxConstraints(minWidth: 200, maxWidth: 250),
    child: ...
  ),
);

The RenderConstrainedBox here has absolutely no effect on the layout of this render tree, no matter what the value of x is, so in this case _additionalConstraints should be completely ignored in the intrinsics calculation.
@goderbauer @chunhtai I wonder if we should use computeDrySize to calculate the intrinsic height of children in a RenderFlex, when CrossAxisAlignment is .stretch?

I wonder if we should use computeDrySize to calculate the intrinsic height of children in a RenderFlex, when CrossAxisAlignment is .stretch

I think the current behavior is correct. If using intrinsic size, it is not stretching the children. The doc also mentioned the child need to fill the space
https://api.flutter.dev/flutter/rendering/CrossAxisAlignment.html

Do you have a link to the error?

To clarify: Are you confirming my PR implementation is correct, or the current production code?
And is @LongCatIsLooong expected to take the next step?
I'll add clarifying comments in the meantime.

would also need to rebase to pass the ci

Hi @lohnn looks like there is some formatting issue https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8720576242602713249/+/u/run_test.dart_for_analyze_shard_and_subshard_None/stdout?format=raw

I spoke with @goderbauer offline he pointed out in the getMaxIntrinsicHeight documentation it says:

The width argument may give a specific width to assume

So the incoming width should be interpreted as a set of tight width constraints. If that interpretation is true, the additionalConstraints should be ignored in the implementation here because it would be overruled by the incoming tight width constraints.

But at the same time there are places that the incoming height is interpreted as a maxWidth constraint, for example the RenderIntrinsicHeight implementation.
also in the same API docs:

The given width can be infinite, meaning that the intrinsic height in an unconstrained environment is being requested

which seems to imply that width should be interpreted as a set of loose width constraints [0, width].

So if the first interpretation is true, then the current RenderConstrainedBox doesn't have to be changed. We'll need to fix RenderIntrinsicHeight and everywhere else that's assuming the incoming width means [0, width] instead of [width, width].

So the incoming width should be interpreted as a set of tight width constraints.

Is this true? The doc doesn't suggest that.I thought the getIntrinsicHight is more of what is the height if you are given this width. the renderobject itself is free to impose additional constraint on top of the width. in fact, if we don't consider the additional constraint, won't we return a different height when we actually layout the renderobject with the given width?

This is also what I thought, and this is also causing real layout bugs where intrinsic (expected) sizes are not matching the actual size.

I totally agree that this is a real layout but, but applying this patch will unfortunately introduce real layout bugs too, which you can repro by just changing the CrossAxisAlignment to .stretch in your original example:

CustomScrollView(
      slivers: [
        SliverFillRemaining(
          hasScrollBody: false,
          child: Column(
            crossAxisAlignment: CrossAxisAlignment.stretch,
            children: const [
              SizedBox(
                width: 300,
                child: Text(
                  'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec commodo arcu nec varius ultricies. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia curae; Pellentesque porta auctor.',
                ),
              ),
            ],
          ),
        ),
      ],
    );

Of course it doesn't make a lot of sense when .stretch and SizedBox(width: 300) are used side-by-side like that in the code, but the SizedBox can be introduced by a reusable Widget that tries to establish a preferred width.

All right, I now see how we trade from one problem to another in this case.
Any ideas on how to propagate the CrossAxisAlignment constraints?