facebook / yoga

Yoga is an embeddable layout engine targeting web standards.
https://yogalayout.dev/
MIT License
17.32k stars 1.44k forks source link

Yoga does not respect automatic minimum size for flexbox items #1409

Open szmarczak opened 1 year ago

szmarczak commented 1 year ago

Description

Actual (all items are equal width):

image

Expected (1 and 2 should shrink):

image

React Native Version

0.72.4

Output of npx react-native info

> npx react-native info
info Fetching system and libraries information...
System:
  OS: Windows 10 10.0.22621
  CPU: "(12) x64 AMD Ryzen 5 5600 6-Core Processor              "
  Memory: 19.21 GB / 31.91 GB
Binaries:
  Node:
    version: 20.2.0
    path: C:\Program Files\nodejs\node.EXE
  Yarn: Not Found
  npm:
    version: 9.7.2
    path: C:\Program Files\nodejs\npm.CMD
  Watchman: Not Found
SDKs:
  Android SDK: Not Found
  Windows SDK: Not Found
IDEs:
  Android Studio: AI-222.4459.24.2221.9971841
  Visual Studio:
    - 17.5.33424.131 (Visual Studio Community 2022)
Languages:
  Java:
    version: 11.0.20.1
    path: C:\Program Files\Eclipse Adoptium\jdk-11.0.20.101-hotspot\bin\javac.EXE
  Ruby: Not Found
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.72.4
    wanted: 0.72.4
  react-native-windows: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: Not found
  newArchEnabled: Not found
iOS:
  hermesEnabled: Not found
  newArchEnabled: Not found

Steps to reproduce

React Native:

import { Text, View } from 'react-native';

const item = {
  flexGrow: 1,
  flexShrink: 1,
  flexBasis: '100%',
};

export default function App() {
  return (
    <View style={{ display: 'flex', flexDirection: 'row' }}>
      <View style={[{ backgroundColor: 'red' }, item]}><Text>1</Text></View>
      <View style={[{ backgroundColor: 'green' }, item]}><Text>2</Text></View>
      <View style={[{ backgroundColor: 'blue' }, item]}><Text style={{ minWidth: '100%' }}>33333333333333333333</Text></View>
    </View>
  );
}

Web equivalent:

<style>
body {
  background-color: black;
  color: white;
  font-weight: bold;
  font-size: 24px;
  margin: 0;
}

/* React Native */
/*
* {
  min-width: 0px;
  overflow-wrap: break-word;
}
*/

.f {
  display: flex;
  text-align: center;
}

.r, .g, .b {
  flex-grow: 1;
  flex-shrink: 1;
  flex-basis: 100%;
}

.r {
  background-color: hsl(0deg, 50%, 20%);
}

.g {
  background-color: hsl(90deg, 50%, 20%);
}

.b {
  background-color: hsl(45deg, 50%, 20%);
}

.txt {
  min-width: 100%;
}
</style>

<div class="f">
  <div class="r">1</div>
  <div class="g">2</div>
  <div class="b">
    <div class="txt">
      3333333333333
    </div>
  </div>
</div>

Snack, screenshot, or link to a repository

Repro has been provided above.

github-actions[bot] commented 1 year ago
:warning: Newer Version of React Native is Available!
:information_source: You are on a supported minor version, but it looks like there's a newer patch available - 0.72.5. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.
github-actions[bot] commented 1 year ago
:warning: Missing Reproducible Example
:information_source: We could not detect a reproducible example in your issue report. Please provide either:
  • If your bug is UI related: a Snack
  • If your bug is build/update related: use our Reproducer Template. A reproducer needs to be in a GitHub repository under your username.
szmarczak commented 1 year ago

Stupid bot didn't notice that repro has been provided.

cortinico commented 1 year ago

Stupid bot didn't notice that repro has been provided.

You should provide a link to a repro or a Snack, not a snippet.

NickGerleman commented 1 year ago

Interestingly react-native-web adds min-width and min-height to every view, to avoid overflowing. This behavior is consistent with what Yoga does, but seems to be out of spec.

From the CSS spec, min-width was originally 0 in CSS 2 and this is still the case for block or inline contexts but changed to auto in CSS 3. https://www.w3.org/TR/css-sizing-3/#min-size-properties

Flexbox says this should be the content-based minimum size. So in the correct case, we would be sizing the box according to that, then distribute the rest of the free space accordingly, like the expected example. https://www.w3.org/TR/css-flexbox-1/#min-size-auto

Yoga will incorporate max-content into flexBasis: 'auto', but doesn't incorporate a minimum size constraintt. From a comment in code:

// Deviations from standard:
//  * Section 4.5 of the spec indicates that all flex items have a default
//    minimum main size. For text blocks, for example, this is the width of the
//    widest word. Calculating the minimum width is expensive, so we forego it
//    and assume a default minimum main size of 0.

I think this might also depend on https://github.com/facebook/yoga/issues/1298 where right now Yoga doesn't actually have a way to ask for min-content in general.

//    The spec describes four different layout modes: "fill available", "max
//    content", "min content", and "fit content". Of these, we don't use "min
//    content" because we don't support default minimum main sizes (see above
//    for details). Each of our measure modes maps to a layout mode from the
//    spec (https://www.w3.org/TR/CSS3-sizing/#terms):
//      - MeasureMode::Undefined: max content
//      - MeasureMode::Exactly: fill available
//      - MeasureMode::AtMost: fit content

Arguably, if we want to build for conformance, this might not be the right choice. But it would maybe be worth seeing how much this trick is actually saving on performance in a real app, if it was an intentional design decision.

FYI @necolas, that we should maybe put min-width: 0/min-height: 0 into the RSD shim unless we decide to fix this. Bu

szmarczak commented 1 year ago

@NickGerleman Huge thanks for looking into this! Do you know of any workarounds?

NickGerleman commented 1 year ago

The workaround for consistency would be set an explicit minimum size, since Yoga will not determine one automatically. That's not really fixing the root issue, admittedly.

nicoburns commented 1 year ago

I was interested in what the performance impact of implementing "automatic minimum size" might be, so I've done a little informal investigation into the issue.

Methodology

Modifying Taffy's benchmarks, which include head-to-head comparison with Yoga via the rust bindings (yoga-rs) with various style combinations designed to stress the min-content sizing part of the algorithm. In particular, I tested a lot of trees that had width/height set to auto so that neither Taffy nor Yoga could easily hit the fast paths that can short-circuit measuring entire subtrees.

Findings / Conclusions

  1. Despite the fact that Taffy implements the "automatic minimum size" part of the spec (requested in this issue) while Yoga elides this feature in the name of performance, in practice Taffy is still faster than Yoga for most combinations of styles I tried. The exceptions I found are:

    • Cases where flex-basis is set but not width/height (but both Yoga and Taffy are very fast in these cases)
    • Cases where flex-direction: column and flex-wrap: wrap which Taffy really doesn't like. However, this case is explicitly called out in the spec as being "insanely expensive" to calculate correctly, and I don't think Taffy is implementing the fast-path exception from the spec for this case. So that's probably unrelated to "automatic minimum size".

      I should note that this only so consistently true as of a couple of recent performance fixes to Taffy (notably https://github.com/DioxusLabs/taffy/pull/556). And it's entirely possible that such performance wins can be found in Yoga too.

  2. However, if the primary concern is to avoid regressing performance compared to the existing released version(s) of Yoga, then I would be pretty confident that it is possible to implement this feature while maintaining that level of performance, although this may well require some effort to be put into optimising Yoga.

    Speaking of which:

  3. Performance of Flexbox implementations seems to be very sensitive to their caching implementations. We've now found tweaks to Taffy's caching logic that give orders of magnitude improvements to certain tree structures on 4 or 5 separate occasions. So that might be a good place to look if we want to improve Yoga's performance (but perhaps it might be better to hold on optimising until a compliant algorithm implementation has been developed so people don't end up depending on perf that isn't achievable within the spec). Yoga and Taffy are both using 9 cache slots, but Yoga uses a FIFO strategy when storing new cache entries, whereas Taffy assigns a slot based on the combination of "measure modes" (to use Yoga's terminology) used when requesting the layout.

Benchmark results

Taffy's standard benchmarks

These are Taffy's current standard benchmarks which we have committed to the repo. The idea of including them here is that they are useful for giving us an insight into what performance in easier / less degenerate cases than the above. Although arguably they measure a bit too easy of a scenario. The use the following style generation function:

fn create_style<R: Rng>(rng: &mut R) -> Style {
    fn gen_dimension<R: Rng>(rng: &mut R) -> Dimension {
        let switch: f32 = rng.gen_range(0.0..=1.0);
        if switch < 0.2 {
            Dimension::Auto
        } else if switch < 0.8 {
            Dimension::Length(rng.gen_range(0.0..500.0))
        } else {
            Dimension::Percent(rng.gen_range(0.0..1.0))
        }
    }
    Style {
        size: Size {
            width: gen_dimension(rng),
            height: gen_dimension(rng),
        },
        ..Default::default()
    }
}
Benchmark Node Count Depth Yoga ([ba27f9d]) Taffy ([be627e7f])
big trees (wide) 1,000 1 0.7339 ms 0.4855 ms
big trees (wide) 10,000 1 6.9652 ms 5.5412 ms
big trees (wide) 100,000 1 130.04 ms 62.017 ms
big trees (deep) 4,000 12 2.2466 ms 1.9278 ms
big trees (deep) 10,000 14 5.9559 ms 4.5248 ms

My conclusion from these numbers: although Taffy is a bit faster, the numbers are more similar than they are different (with the exception of the unrealistically large 100k node test).

Stress Test

The results below pertain to trees of nodes where styles were creating using the following function (this is using Taffy/Rust notation, but hopefully it should be pretty clear what it's doing):

fn create_style<R: Rng>(rng: &mut R) -> Style {
    let is_row = rng.gen::<f32>() < 0.5;
    Style {
        flex_direction: if is_row {
            FlexDirection::Row
        } else {
            FlexDirection::Column
        },
        flex_grow: 1.0,
        margin: Rect {
            left: LengthPercentageAuto::Length(10.0),
            right: LengthPercentageAuto::Length(10.0),
            top: LengthPercentageAuto::Length(10.0),
            bottom: LengthPercentageAuto::Length(10.0),
        },
        ..Default::default()
    }
}

these styles were picked for inclusion here because they were amongst the slowest results I could find for both libraries.

Benchmark Node Count Depth Yoga ([ba27f9d]) Taffy ([be627e7f])
big trees (wide) 1,000 1 1.0176 ms 0.6545 ms
big trees (wide) 10,000 1 11.002 ms 6.9952 ms
big trees (wide) 100,000 1 171.58 ms 80.602 ms
big trees (deep) 4,000 12 63.387 ms 10.146 ms
big trees (deep) 10,000 14 241.81 ms 29.160 ms

It was interesting (and surprising!) to me that Yoga actually seems to scale worse in this more difficult scenario (note in particular the numbers in the "deep" benchmarks), despite containing algorithm shortcuts designed to facilitate this kind of scalability. Whether that's because the algorithm shortcuts don't help much, or because there are other aspects of Taffy's implementation that compensate for it using a slower algorithm I'm not sure. But I figure that in many ways it doesn't matter too much, because either could be implemented in Yoga.


These benchmarks are definitely a bit artificial, and I'd love to run some numbers on some real-world views/trees, but hopefully they give us some idea of what might be possible :)

Let me know if there are any other style combinations you'd like me to benchmark.

NickGerleman commented 1 year ago

Thanks for the helpful context. Apart from the general "is it possible for this to be efficient", comparing Yoga to previous baseline is probably what we need to do here.

Re benchmarks, I definitely agree with the sentiment of what we have right now veering too far into synthetic. I've been at times tempted to see if we can dump a tree from a real-world app, and make a fixture out of it, so we can test laying out a real-world tree (or different real-world trees), along with incremental mutation and relayout. We do have a couple of ~10k node real-world UIs that have pretty different characteristics.

I've also noticed, at least for the case of YGBenchmark.c, we sometimes seem to spending more time counting how long it takes to allocate and free Yoga nodes, then the layout process itself. I was experimenting with moving yoga::Style to a heap allocated structure instead of an inline one, and IIRC the benchmarks showed something like a >10% difference, which was much less visible when isolating to just the layout step (which was slower due to less locality, but a lot less). I'm not aware of whether yoga-rs does much additional boxing on top of Yoga structures, but that might also be a variable to isolate.

For caching, beyond what Yoga does, we recently discovered RN Fabric can invalidate the layout cache much more often than it needs to. In RN there is a Yoga node per ShadowNode, and a series of bugs which leads to sometimes restoring previous ShadowNodes, containing pre-layout information, into new trees, on React mutation. In the most serious cases, this could lead to small UI changes having catastrophic effect to layout cache, and effectively removing most of the previous work. https://github.com/facebook/react-native/commit/ef438464fab7bc663bc664a641c0ead8e35cf8cd was one of the changes to help here, but there are more coming, and some discussion on the general model (RN Fabric heavily relying on Yoga private APIs is another thing I'd like to get us out of).

NickGerleman commented 10 months ago

Apart from cost of flex layout, I think part of the performance concern here is for laying out text.

I think if we wanted to implement this correctly, we could still shortcut min-content measurement in most cases I think.

If flex basis is based on content, we already have a max-content measurement. If we don’t need to shrink, and didn’t otherwise have an explicit flex basis, we know we are already at least min-content sized in the main axis.

NickGerleman commented 10 months ago

Downside, the elegant way to do this involves breaking every Yoga measure function already in existence, since now they need to know about min-content. Or adding a secondary function.