fix Container min-height produces incorrect size when flex-wrap is enabled #1491

goolAdapter · 2023-12-01T03:16:21Z

try to fix #1300

facebook-github-bot · 2023-12-01T03:16:27Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

facebook-github-bot · 2023-12-01T04:06:04Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

NickGerleman · 2023-12-02T01:05:31Z

Thank you for the contribution! I will take a closer look soon. I know the existing math around here is kind of wrong (#1013 edited a similar place).

For safety, we have been wrapping some new fixes in an "errata" check, so that existing apps can opt into incorrect behavior to preserve layouts. When a change is imported, it will run through a series of screenshot tests internally, which can give signal of how likely something is to cause breaks, but if we are being cautious, we'd probably want to wrap this in one. By default, Yoga would still get the correct behavior.

NickGerleman · 2023-12-02T01:05:48Z

.vscode/launch.json

-      "preLaunchTask": "Build Unit Tests"
+      "preLaunchTask": "Build Unit Tests",
+      "args": [
+        "--gtest_filter=YogaTest.*" // Replace * with the name of your test case


I don't think all the tests start with this

NickGerleman · 2023-12-06T13:56:02Z

yoga/algorithm/CalculateLayout.cpp

+  const FloatOptional minLineHeight = yoga::resolveValue(
+      node->getStyle().minDimension(dimension(crossAxis)), crossAxisownerSize);
+  if (minLineHeight.isDefined()) {
+    if (totalLineHeight > minLineHeight.unwrap()) {
+      totalLineCrossDim = totalLineHeight;
+    } else {
+      totalLineCrossDim = minLineHeight.unwrap();
+    }
+  }


Lower down, we feed lineHeight to boundAxis to do something similar (but it also accounts for padding and border). How does the new logic differ?

Summary: Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Differential Revision: D52087013

Summary: Fixes facebook#1300 Fixes facebook#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook#1491 2. facebook#1493 3. facebook#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Differential Revision: D52087013

Summary: Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Differential Revision: D52087013

…41916) Summary: X-link: facebook/yoga#1513 Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Differential Revision: D52087013

…1513) Summary: X-link: facebook/react-native#41916 Fixes facebook#1300 Fixes facebook#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook#1491 2. facebook#1493 3. facebook#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Differential Revision: D52087013

…41916) Summary: X-link: facebook/yoga#1513 Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Differential Revision: D52087013

…1513) Summary: X-link: facebook/react-native#41916 Fixes facebook#1300 Fixes facebook#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook#1491 2. facebook#1493 3. facebook#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Differential Revision: D52087013

goolAdapter · 2023-12-13T07:39:33Z

I've seen someone else come up with a better proposal. I withdraw my request. Thank you

…41916) Summary: X-link: facebook/yoga#1513 Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Reviewed By: joevilches Differential Revision: D52087013

…1513) Summary: X-link: facebook/react-native#41916 Fixes facebook#1300 Fixes facebook#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook#1491 2. facebook#1493 3. facebook#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Reviewed By: joevilches Differential Revision: D52087013

…41916) Summary: X-link: facebook/yoga#1513 Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Reviewed By: joevilches Differential Revision: D52087013

Summary: X-link: facebook/yoga#1513 X-link: facebook/react-native#41916 Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Reviewed By: joevilches Differential Revision: D52087013 fbshipit-source-id: 8d95ad17e58c1fec1cceab9756413d0b3bd4cd8f

Summary: Pull Request resolved: #1513 X-link: facebook/react-native#41916 Fixes #1300 Fixes #1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. #1491 2. #1493 3. #1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Reviewed By: joevilches Differential Revision: D52087013 fbshipit-source-id: 8d95ad17e58c1fec1cceab9756413d0b3bd4cd8f

Summary: X-link: facebook/yoga#1513 Pull Request resolved: #41916 Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Reviewed By: joevilches Differential Revision: D52087013 fbshipit-source-id: 8d95ad17e58c1fec1cceab9756413d0b3bd4cd8f

…41916) Summary: X-link: facebook/yoga#1513 Pull Request resolved: facebook#41916 Fixes facebook/yoga#1300 Fixes facebook/yoga#1008 This fixes a smattering of issues related to both sizing and aligment of multi-line-containers: 1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container. 2. Per-line leads were sometimes incorrectly contributing to alignment within the line 3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint. Need to test how breaking this is, to see if it might need to go behind an errata. See related PRs: 1. facebook/yoga#1491 2. facebook/yoga#1493 3. facebook/yoga#1013 Changelog: [General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers Reviewed By: joevilches Differential Revision: D52087013 fbshipit-source-id: 8d95ad17e58c1fec1cceab9756413d0b3bd4cd8f

fix Container min-height produces incorrect size

c2855ac

goolAdapter changed the title ~~fix Container min-height produces incorrect size when flex-wrap is enabled #1300~~ fix Container min-height produces incorrect size when flex-wrap is enabled Dec 1, 2023

facebook-github-bot added CLA Signed Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 1, 2023

NickGerleman reviewed Dec 2, 2023

View reviewed changes

NickGerleman reviewed Dec 6, 2023

View reviewed changes

NickGerleman mentioned this pull request Dec 12, 2023

Fix sizing and alignment issues with multi-line containers facebook/react-native#41916

Closed

NickGerleman mentioned this pull request Dec 12, 2023

Fix sizing and alignment issues with multi-line containers #1513

Closed

goolAdapter closed this Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix Container min-height produces incorrect size when flex-wrap is enabled #1491

fix Container min-height produces incorrect size when flex-wrap is enabled #1491

goolAdapter commented Dec 1, 2023

facebook-github-bot commented Dec 1, 2023

facebook-github-bot commented Dec 1, 2023

NickGerleman commented Dec 2, 2023 •

edited

Loading

NickGerleman Dec 2, 2023

NickGerleman Dec 6, 2023

goolAdapter commented Dec 13, 2023

fix Container min-height produces incorrect size when flex-wrap is enabled #1491

fix Container min-height produces incorrect size when flex-wrap is enabled #1491

Conversation

goolAdapter commented Dec 1, 2023

facebook-github-bot commented Dec 1, 2023

Action Required

Process

facebook-github-bot commented Dec 1, 2023

NickGerleman commented Dec 2, 2023 • edited Loading

NickGerleman Dec 2, 2023

Choose a reason for hiding this comment

NickGerleman Dec 6, 2023

Choose a reason for hiding this comment

goolAdapter commented Dec 13, 2023

NickGerleman commented Dec 2, 2023 •

edited

Loading