Constrain dimensions passed down in intrinsic protocol within RenderConstrainedBox #137874
Conversation
5fa3bd6 to
ef62b31
Compare
|
(Trying to see how/what this will break in google's internal repository, will report back) |
|
@goderbauer how's the progress? :) |
|
I had some time to look at this a little closer. I attempted exactly the same fix some time ago in #71880 and shortly after that figured out that doing just that wasn't enough as we have to consider infinity separately (#72095). Looks like the latter is missing from this PR. Having said all that, my change was later reverted in #72102 because it broke too much stuff within Google that was relying on the old broken behavior and we never found the time/resources to clean that up. I finally managed to kick off a test run within google to see what the current situation is, but I don't have high hopes that anything has changed there. I'll keep you posted on here, though. |
|
As suspected, this is failing over 200 tests within google. Some of the screenshot tests look actually broken now where more content gets cut off and new overflow warnings are produced. This would have to be investigated more closely before we can land this fix. Unfortunately (and I know this can be frustrating to hear), this is all internal code that I cannot share publicly. Furthermore, people with access to the code currently do not have the bandwidth to investigate this. |
|
Came across this as part of #140613. It would be nice to be able to add hyperlinks via RichText in the responsive web port I am working on for my apps—looking forward to a fix. |
|
We could add a flag that controls whether or not the additional constraints are taken into account when doing intrinsic dimensions, basically turn this bug into a feature. |
|
I like the idea of turning this into a feature by having a flag to chose the behavior you want. @Albert221 @callius Would that work for your use cases? Would you be interested in implementing that? The new implementation will also have to address the infinity use case (see #72095). |
|
Flag as in a |
|
I am going to close this since as-is this will be too breaking. If somebody wants to improve this with the suggestions made above: Please open a new PR. |
Does this was implemented ? |
RenderConstrainedBoxwas ignoring theadditionalConstraintswhile calling the child'sget{Min,Max}Intrinsic{Width,Height}. This was in my understanding wrong. The intrinsic dimensions are often used to make some layout calculations. But these dimensions can't be imaginary. They must be real. If we call for minimum intrinsic height given some width, then the widget should be able to lay itself out within tight constraints for the same width and height. This was not the case for widgets that we wrap inSizedBoxorConstrainedBox, because they were ignoring the constraints when passing down the known dimension when calling for intrinsic other dimension. This PR fixes that.Fixes #137546.
Below are screenshots of before and after, given the code from the issue above.
Pre-launch Checklist
///).