[0.71.0-rc.0] Gap, flexWrap & alignContent renders wrong size

[Titozzz]

Description

When using gap, flexWrap and alignContent, size is miscalculated, because gap isn't taken into account when computing the sizes, so if any wrapping happens because of gap, it is not computed properly

Please note that I have tried with and without legacyStretchBehavior enabled (by patching react-native).

Version

0.71.0-rc.0

Output of npx react-native info

System:
OS: macOS 12.6
CPU: (8) arm64 Apple M1
Memory: 155.78 MB / 16.00 GB
Shell: 5.8.1 - /bin/zsh
Binaries:
Node: 18.11.0 - /usr/local/bin/node
Yarn: 1.22.17 - /opt/homebrew/bin/yarn
npm: 8.19.2 - /usr/local/bin/npm
Watchman: 2022.10.10.00 - /opt/homebrew/bin/watchman
Managers:
CocoaPods: 1.11.3 - /opt/homebrew/bin/pod
SDKs:
iOS SDK:
Platforms: DriverKit 21.4, iOS 15.5, macOS 12.3, tvOS 15.4, watchOS 8.5
Android SDK: Not Found
IDEs:
Android Studio: 2021.3 AI-213.7172.25.2113.9123335
Xcode: 13.4/13F17a - /usr/bin/xcodebuild
Languages:
Java: 11.0.15 - /usr/bin/javac
npmPackages:
@react-native-community/cli: Not Found
react: 18.2.0 => 18.2.0
react-native: 0.71.0-rc.0 => 0.71.0-rc.0
react-native-macos: Not Found
npmGlobalPackages:
react-native: Not Found

Steps to reproduce

In order to reproduce you need to have:

• 1 container with flexWrap: ‘wrap’ & alignContent: ‘stretch’ & gap: someValue
• Children that wrap only because of the gap property (they would fit in the container otherwise)


When that happens the size calculated by alignContent: ’stretch’ doesn’t take into account the wrap, so each child will have the full container size instead of half

.

Example:

• Container of 300 x 300 direction 'row', flexWrap: 'wrap', alignContent: 'stretch'
• 5 children of width: 60, flexGrow: 1

Before adding gap: 1

After adding gap: 1

Note that if the child where already wrapping (example width: 61) this would not happen. It only happens if the wrapping is caused by gap

Snack, code example, screenshot, or link to a repository

Web equivalent:
https://jsfiddle.net/t8q6xevj/1/
Native Code used to test:

Feel free to alter gap of container / width of children

import React from 'react';
import {
  SafeAreaView,
  View,
} from 'react-native';

function App(): JSX.Element {
  return (
    <SafeAreaView style={{flexGrow: 1, alignItems: 'center', justifyContent: 'center'}}>
        <View
          style={{
            height: 300,
            width: 300,
            backgroundColor: 'blue',
            flexWrap: 'wrap',
            flexDirection: 'row',
            alignContent: 'stretch',
            gap: 1,
          }}>
          <View
            style={redChild}></View>
          <View
            style={greenChild}></View>
          <View
            style={redChild}></View>
          <View
            style={greenChild}></View>
          <View
            style={redChild}></View>
        </View>
    </SafeAreaView>
  );
}

const child = {
  flexGrow: 1,
  width: 60,
}

const redChild = {
  ...child,
  backgroundColor: 'red',
};
const greenChild = {
  ...child,
  backgroundColor: 'green',
}

export default App;

[kelset]

cc @necolas @NickGerleman

[NickGerleman]

Good report (and good we can fix before much usage). Gap should not cause the height to increase when it is the reason for wrapping. Will take a look and add this as a test. FYI @jacobp100

YGCalculateCollectFlexItemsRowValues possibly prepares something unexpected, or a later step is doing something wrong to set cross-axis dimensions. Should be able to debug this.

[jacobp100]

Since gap sets both columnGap and rowGap, I wonder if it happens when you only set columnGap. That's the property that causes it to overflow - but it could be rowGap that is actually the issue and it was just coincidental it manifested during the wrapping

[NickGerleman]

Still happens with just columnGap. I have a fixture working which causes a failing unit test.

Planning to dig in a bit, since I know running these UTs is currently a giant pain in OSS (I thought I would have added the CMake baed UTs a while back but have been awful about maintaining focus recently). But should be manually reproable as well.

[jacobp100]

If you post the branch name I can dig a bit too

[jacobp100]

Maybe it's worth adding a disclaimer about gap in the release notes too. Warn people it may have bugs, and that fixing those bugs later on may break their layouts

[jacobp100]

@intergalacticspacehighway FYI too

[kelset]

Maybe it's worth adding a disclaimer about gap in the release notes too. Warn people it may have bugs, and that fixing those bugs later on may break their layouts

This is actually something that I was talking about with @Titozzz: is there a way to maybe have the flex-gap stuff hidden behind some experimental flag so that folks know that fixes/issues/bugs and breaking changes might come at a later stage as things pop up?

[NickGerleman]

Debugging this, comparing wrap with gap vs wrap due to size overflow.

Step 8: "MULTI-LINE CONTENT ALIGNMENT" is normally what sets things up. Children should most recently have been called with cross axis YGMeasureModeAtMost which measures cross-axis size is at zero, but YGAlignStretch will set the cross-dimension based on available size divided by line count.

In the gap case, measured cross-axis dimensions are 300px at this step. In the non-gap case it is transiently this (I think it is the result of a different YGMeasureMode?). But it means at some previous step we are not measuring cross-axis correctly specifically in the gap case.

[NickGerleman]

Maybe it's worth adding a disclaimer about gap in the release notes too. Warn people it may have bugs, and that fixing those bugs later on may break their layouts

This is actually something that I was talking about with @Titozzz: is there a way to maybe have the flex-gap stuff hidden behind some experimental flag so that folks know that fixes/issues/bugs and breaking changes might come at a later stage as things pop up?

I don't think we should gate it. There are more conformance tests we could maybe add to have caught this (e.g. a plan was to generate more Yoga fixtures from WPT), but real usage is also what led to bug discovery here, and putting the feature behind a flag would prevent that.

I do want to gate future capabilities behind StrictLayout, where we want to explicitly allows instability to make conformance fixes while staging. But flex gap is already here, and we have told people they will get it.

[intergalacticspacehighway]

I think the issue could be on this statement where it doesn't account gap and childCrossSize gets set to availableInnerCrossDim i.e. 300px. Maybe we should account gap here as well.

[intergalacticspacehighway]

 currentRelativeChild->getGapForAxis(crossAxis, availableInnerCrossDim).isUndefined()

Adding this fixes the issue!

[NickGerleman]

Okay, so I think I'm understanding this?

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. I confirmed this is normally taken.

Probably worth checking the other usages of flexBasisOverflows in case there are other options taken in the non-overflow case that only accounts for basis. Or to see whether we should put our math there.

[NickGerleman]

Yeah, it looks like totalOuterFlexBasis, used for overflow logic, is more than just flex basis, it also includes margins already. It is also used for setting main axis measure mode to parent width instead of minimum size if we have overflow.

I think the right solution here is to rename this, and incorporate gap spacing into its checks.

[NickGerleman]

PR here: facebook/yoga#1173
New UT and previous ones are now passing.

[Titozzz]

I also think we should not gate it, but warn users that if it doesn't match the spec there will be breaking changes might be a good idea

[NickGerleman]

A general strategy for compatibility fixes has been:

1. Enable the fix everywhere if we think breaks are unlikely. I.e. does the change cause issues if we turn it on for an experiment group in a very large codebase?
2. Put the continuing set of the rest of the breaking fixes under StrictLayout.

We are starting off at zero usage of gap within the RN ecosystem, so we are probably safe to make fixes while the usage is building, but after, we could fold likely impacting changes into StrictLayout.

[NickGerleman]

It's also one of the reasons I want to gate new major additions behind StrictLayout, since it makes that guarantee explicit (unless you opt into a quirk).

[Titozzz]

Fixed by 1aa157b