[0.71.0-rc.3] Redundant margin when using either rowGap or gap

[mobily]

Description

gap/rowGap adds an unwanted gutter (space) to the last layout element.

Version

0.71.0-rc.3

Output of npx react-native info

System:
    OS: macOS 13.0
    CPU: (10) arm64 Apple M1 Max
    Memory: 159.22 MB / 64.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.14.0 - /usr/local/bin/node
    Yarn: 1.22.18 - /usr/local/bin/yarn
    npm: 8.9.0 - /opt/homebrew/bin/npm
    Watchman: 2022.09.05.00 - /opt/homebrew/bin/watchman
  Managers:
    CocoaPods: Not Found
  SDKs:
    iOS SDK:
      Platforms: DriverKit 21.4, iOS 16.0, macOS 12.3, tvOS 16.0, watchOS 9.0
    Android SDK:
      API Levels: 29, 30, 31, 32
      Build Tools: 29.0.2, 29.0.3, 30.0.2, 30.0.3, 31.0.0, 32.0.0, 32.1.0
      System Images: android-32 | Google APIs ARM 64 v8a
      Android NDK: Not Found
  IDEs:
    Android Studio: 2021.1 AI-211.7628.21.2111.8193401
    Xcode: 14.0.1/14A400 - /usr/bin/xcodebuild
  Languages:
    Java: 11.0.14.1 - /usr/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: 18.2.0 => 18.2.0
    react-native: 0.71.0-rc.3 => 0.71.0-rc.3
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

Steps to reproduce

The code example below is self-explanatory.

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

<View
  style={{
    gap: 16,
    backgroundColor: 'red',
  }}
>
  <View style={{ backgroundColor: 'yellow' }}>
    <Text>test1</Text>
  </View>
  <View style={{ backgroundColor: 'yellow' }}>
    <Text>test2</Text>
  </View>
  <View style={{ backgroundColor: 'yellow' }}>
    <Text>test3</Text>
  </View>
</View>

[NickGerleman]

I can try to reproduce soon. cc @jacobp100 @intergalacticspacehighway in case either of you can get to it first.

This one is kind of surprising to me given the test coverage.

[mobily]

@NickGerleman let me know if you need more details on this from me (reproducing this issue should be pretty straightforward)

[jacobp100]

I guess a test we miss is this:

<div id="column_gap_inflexible" style="flex-direction: row; column-gap: 10px;">
  <div><div style="width: 20px; height: 20px;"></div></div>
  <div><div style="width: 20px; height: 20px;"></div></div>
  <div><div style="width: 20px; height: 20px;"></div></div>
</div>

Where the direct children have to resolve their children to calculate their size

I also wonder if this bug still produces if you remove the extra <View>s:-

<View
  style={{
    gap: 16,
    backgroundColor: 'red',
  }}
>
  <Text style={{ backgroundColor: 'yellow' }}>test1</Text>
  <Text style={{ backgroundColor: 'yellow' }}>test2</Text>
  <Text style={{ backgroundColor: 'yellow' }}>test3</Text>
</View>

@mobily ?

[intergalacticspacehighway]

Adding below check might fix it.

Issue is that it's increasing the main axis size of the view. I think we should check for the last child here.

We can check if it's the last child by const bool isLastChild = i == collectedFlexItemsValues.endOfLineIndex - 1;

So it's like before it was calculating betweenMainDim from the remainingFreeSpace (which would become 0 when it's last child) and now we're initializing it with a gap. So, I think we should not add it if it's the last child.

Looking if it needs to be fixed somewhere else.

[mobily]

@jacobp100 here's the screenshot after removing extra View components

[intergalacticspacehighway]

const bool isLastChild = i == collectedFlexItemsValues.endOfLineIndex - 1;
betweenMainDim -= isLastChild ? gap : 0;

This should fix it. I need to do a buck setup to run tests, did it a long ago, will try again 😅

[NickGerleman]

const bool isLastChild = i == collectedFlexItemsValues.endOfLineIndex - 1;
betweenMainDim -= isLastChild ? gap : 0;

This should fix it. I need to do a buck setup to run tests, did it a long ago, will try again 😅

The Buck setup in OSS is no longer a thing (still need to add a CMake build for the GTest UTs instead), but I can run the tests internally if you make a PR.

[intergalacticspacehighway]

Thanks, @NickGerleman created a PR.

Buck setup

Yeah, I tried running it. Looks like I'll have to downgrade the java version to 8 😅. Let me know if something fails.

[cortinico]

I need to do a buck setup to run tests, did it a long ago, will try again 😅

I would suggest to just send a PR and let the CI run Buck tests in OSS for you.
Sadly Buck OSS in lacking M1 support so it's going to be challenging to run in a standalone environment as @NickGerleman suggested. We're looking into improving this in the future.

[intergalacticspacehighway]

@cortinico got it. I sent it to the Yoga repo facebook/yoga#1188. Should I send it here as well?

[cortinico]

@cortinico got it. I sent it to the Yoga repo facebook/yoga#1188. Should I send it here as well?

Sorry I thought you were talking about Buck tests for React Native and not yoga. I've just triggered the CI for Yoga so you can iterate there 👍

[NickGerleman]

Fix should be ported to 0.71-stable now.