Incorporate gap space into main axis overflow flag #1173

2022-11-15T10:20:26-08:00

NickGerleman commented

2022-11-15 10:20:26 -08:00

(Migrated from github.com)

Summary:
In https://github.com/facebook/react-native/issues/35351 we see incorrect child item height when the flex-wrap is enabled, the cross-axis is to be stretched, and main-axis overflow is caused by gap.

In YGDistributeFreeSpaceSecondPass, if we do not have overflow (determined by flexBasisOverflows), we have stretch cross-alignment, and we reason that nothing can add to main axis dimensions, we know we're a single line and want to take full cross dimensions. and can set YGMeasureModeExactly which uses parent dimensions. Guessing an optimization?

If we do have overflow, then we set YGMeasureModeAtMost to find minimum possible cross-axis dimensions instead.

flexBasisOverflows incorporates both computed flex basis, and margin, so it is more generally a flag for whether we will wrap. So we should incorporate gap spacing into it. E.g. it is also used for whether we should the match main axis parent dimension of the overall container. This change does just that, and renames the flag to mainAxisOverflows.

We will want to cherry-pick the fix for this into RN 0.71 since we have not yet introduced the community to the incorrect behavior, and we expect a lot of usage of flex-gap.

Differential Revision: D41311424

Summary: In https://github.com/facebook/react-native/issues/35351 we see incorrect child item height when the flex-wrap is enabled, the cross-axis is to be stretched, and main-axis overflow is caused by gap. In YGDistributeFreeSpaceSecondPass, if we do not have overflow (determined by flexBasisOverflows), we have stretch cross-alignment, and we reason that nothing can add to main axis dimensions, we know we're a single line and want to take full cross dimensions. and can set YGMeasureModeExactly which uses parent dimensions. Guessing an optimization? If we do have overflow, then we set YGMeasureModeAtMost to find minimum possible cross-axis dimensions instead. `flexBasisOverflows` incorporates both computed flex basis, and margin, so it is more generally a flag for whether we will wrap. So we should incorporate gap spacing into it. E.g. it is also used for whether we should the match main axis parent dimension of the overall container. This change does just that, and renames the flag to `mainAxisOverflows`. We will want to cherry-pick the fix for this into RN 0.71 since we have not yet introduced the community to the incorrect behavior, and we expect a lot of usage of flex-gap. Differential Revision: D41311424

facebook-github-bot commented

2022-11-15 10:21:12 -08:00

(Migrated from github.com)

This pull request was exported from Phabricator. Differential Revision: D41311424

This pull request was **exported** from Phabricator. Differential Revision: [D41311424](https://www.internalfb.com/diff/D41311424)

NickGerleman (Migrated from github.com) reviewed 2022-11-15 10:28:12 -08:00

yoga/Yoga.cpp

NickGerleman (Migrated from github.com) commented

2022-11-15 10:28:12 -08:00

  float totalGap = childCount <= 1

```suggestion float totalGap = childCount <= 1 ```

NickGerleman (Migrated from github.com) reviewed 2022-11-15 10:28:44 -08:00

yoga/Yoga.cpp

NickGerleman (Migrated from github.com) commented

2022-11-15 10:28:44 -08:00

Both work I guess, one skips a bit of math.

jacobp100 (Migrated from github.com) reviewed 2022-11-15 11:24:01 -08:00

yoga/Yoga.cpp

jacobp100 (Migrated from github.com) commented

2022-11-15 11:24:01 -08:00

Because this function is only called once, should we change it to return the main axis size?

jacobp100 commented

2022-11-15 11:24:11 -08:00

(Migrated from github.com)

I think what you’ve done makes sense in the current code

I’m confused why we do all this calculation at all. Don’t we already know if the children wrapped at this point - and is that not the same thing?

I think what you’ve done makes sense in the current code I’m confused why we do all this calculation at all. Don’t we already know if the children wrapped at this point - and is that not the same thing?

NickGerleman (Migrated from github.com) reviewed 2022-11-15 11:26:15 -08:00

yoga/Yoga.cpp

NickGerleman (Migrated from github.com) commented

2022-11-15 11:26:14 -08:00

I was looking at that, but it isn't super clean, since the main purpose of the function is to mutate each node with its calculated basis. Imo it shouldn't be adding margin either, but that is already per child so doing it during the iteration makes sense to me.

NickGerleman commented

2022-11-15 11:29:38 -08:00

(Migrated from github.com)

I’m confused why we do all this calculation at all. Don’t we already know if the children wrapped at this point - and is that not the same thing?

I think this place is the first place we can check. This is directly after we calculate flex basis per-item, which allows us to know if wrapping will happen. Then later we break into specific lines, and that calculation is item-by-item adding leading gap.

> I’m confused why we do all this calculation at all. Don’t we already know if the children wrapped at this point - and is that not the same thing? I think this place is the first place we can check. This is directly after we calculate flex basis per-item, which allows us to know if wrapping will happen. Then later we break into specific lines, and that calculation is item-by-item adding leading gap.

👍 2

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1173