Skip to content
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

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

Closed
1 task done
rasjonell opened this issue May 19, 2023 · 11 comments
Closed
1 task done

Comments

@rasjonell
Copy link

rasjonell commented May 19, 2023

Report

Issues and Steps to Reproduce

I use yoga-layout asm bindings.
When I Create a container node where width is exact, but only set min-height, and enable flexWrap:

const container = Node.create()

container.setWidth(200)
container.setMinHeight(200)
container.setFlexWrap(WRAP_WRAP)
container.setFlexDirection(FLEX_DIRECTION_ROW)

and add child nodes so that it goes to the next line.

const node1 = Node.create()
const node2 = Node.create()

node1.setWidth(120)
node1.setHeight(120)
node2.setWidth(120)
node2.setHeight(120)

container.insertChild(node1, 0)
container.insertChild(node2, 0)

container.calculateLayout()
const height = container.getComputedHeight()

Expected Behavior

The computed height property should be 240(120 + 120)

Actual Behavior

The computed height property is 400.

Seems like the container's min-height property is applied to each wrapped row.

Notes

I tried both with and without web defaults.
This also happens on wrapped columns.

Please let me know if I'm missing something. Thanks!

@NextThread
Copy link

can I work on this?

@NickGerleman
Copy link
Contributor

can I work on this?

Contributions are welcome 🙂. Are you familiar with Yoga's system for test fixtures.

@NextThread
Copy link

can I work on this?

Contributions are welcome 🙂. Are you familiar with Yoga's system for test fixtures.

Yeah,
Okay thanks

@NextThread
Copy link

NextThread commented Aug 17, 2023

@rasjonell hey can you try this?

const childHeightSum = node1.getComputedHeight() + node2.getComputedHeight();
const minHeight = container.getStyle().minHeight;

if (childHeightSum < minHeight) {
  container.setHeight(minHeight);
} else {
  container.setHeight(childHeightSum);
}

const height = container.getComputedHeight();

@rasjonell
Copy link
Author

@rasjonell hey can you try this?

const childHeightSum = node1.getComputedHeight() + node2.getComputedHeight();
const minHeight = container.getStyle().minHeight;

if (childHeightSum < minHeight) {
  container.setHeight(minHeight);
} else {
  container.setHeight(childHeightSum);
}

const height = container.getComputedHeight();

This won't work. First I can't just rely on the sum of computed heights, as not all elements are necessarily wrapped, for example I may have a case like this:

┌─────────────────────────────────┐
│                                 │ Each box has size: 100 x 300
│                                 │ The sum of children heights would be: 1200
│  ┌──────┐  ┌──────┐  ┌──────┐   │ If it happens to be greater than the container's minHeight,
│  │      │  │      │  │      │   │ The final height would be 1200, instead 600
│  │   1  │  │   2  │  │   3  │   │
│  │      │  │      │  │      │   │
│  └──────┘  └──────┘  └──────┘   │
│                                 │
│                                 │
│  ┌──────┐                       │
│  │      │                       │
│  │  4   │                       │
│  │      │                       │
│  └──────┘                       │
│                                 │
│                                 │
└─────────────────────────────────┘

If I collect the sum of heights it won't represent the height of all the rows. I would have to also make some calculations to understand how many rows are there and then calculate the sum, which can be really expensive, considering every layout change would make the same calculation twice(once on yoga's side, once on my own).

Another potential issue with this approach is that I would lose my minHeigt option afterwards as we are setting container.setHeight()

@NextThread
Copy link

NextThread commented Aug 18, 2023

can we follow the below approach?

  • We'll iterate through the child nodes and determine if adding each child to the current row would exceed the
    minHeight property of the container.
  • If the addition of a child to the current row would exceed minHeight, we start a new row.
  • We keep track of the total height by summing up the heights of each row.
  • After iterating through all child nodes, we calculate the final height of the container by considering the total height and
    the minHeight property.
  • We then set the container's height to the calculated final height.
const childNodes = container.getChildNodes();
const childNodeCount = childNodes.length;

let currentRowHeight = 0;
let totalHeight = 0;
let numRows = 1;

for (let i = 0; i < childNodeCount; i++) {
  const childNode = childNodes[i];
  const childHeight = childNode.getComputedHeight();
  
  if (currentRowHeight + childHeight <= minHeight) {
    currentRowHeight += childHeight;
  } else {
    totalHeight += currentRowHeight;
    currentRowHeight = childHeight;
    numRows++;
  }
}

totalHeight += currentRowHeight;

const finalHeight = Math.max(minHeight, totalHeight);
container.setHeight(finalHeight);

@rasjonell
Copy link
Author

Yes, this workaround will fix it, however it's really expensive, considering my client would need to recalculate stuff that's already calculated on Yoga's side. Also I would lose the minHeight config as we are setting the height property.

@NickGerleman
Copy link
Contributor

@NextThread have you looked at fixing this inside of Yoga, instead of recommending workarounds?

@NextThread
Copy link

@NextThread have you looked at fixing this inside of Yoga, instead of recommending workarounds?

No, I'm a beginner so, before that I was trying to share my approach,

should I try?

@goolAdapter
Copy link

Hello, I meet this exactly issue in my work.and I’m trying to fix it.

@cattenblue
Copy link

cattenblue commented Dec 5, 2023

I encountered the same issue and had a doubt about why clamping to the min/max size specified on the container is needed for each line. So, I tried removing it for each line, and I add three test. Here is my submission #1493

NickGerleman added a commit to NickGerleman/react-native that referenced this issue Dec 12, 2023
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
NickGerleman added a commit to NickGerleman/yoga that referenced this issue Dec 12, 2023
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
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Dec 12, 2023
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
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Dec 12, 2023
…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
NickGerleman added a commit to NickGerleman/yoga that referenced this issue Dec 12, 2023
…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
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Dec 12, 2023
…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
NickGerleman added a commit to NickGerleman/yoga that referenced this issue Dec 12, 2023
…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
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Dec 16, 2023
…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
NickGerleman added a commit to NickGerleman/yoga that referenced this issue Dec 16, 2023
…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
NickGerleman added a commit to NickGerleman/yoga that referenced this issue Dec 16, 2023
…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
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Dec 16, 2023
…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
facebook-github-bot pushed a commit that referenced this issue Dec 16, 2023
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
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Dec 16, 2023
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
Othinn pushed a commit to Othinn/react-native that referenced this issue Jan 9, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment