Growing element with flex-grow: 1 breaks layouting of other items

[ShawnCZek]

Hi,

This issue originates from our discussion regarding the complexity of large data sets: #653.

I noticed a bug with flexbox layouting in RmlUi in one of our documents. When one of the elements may grow and only that element specifies the flex property with the value of auto (or sets flex-grow to 1), the layouting yields unexpected results.

I am using the latest version of the library, 5.1. This problem is also reproducible on master.

Reproduction

I have created a separate branch with a sample. This was mainly designed to showcase the performance issues investigated in the discussion linked above, but it can also be used for this case: https://github.com/ShawnCZek/RmlUi/tree/sample_player_list.

The biggest culprits are the following styles:

#contents {
    display: flex;
    flex-direction: column;
}

#players {
    flex: auto;
}

Expected Result

The linked branch contains an HTML sample, too. The styling is not 1:1 due to differences within browsers, but RmlUi should show a similar result to Mozilla Firefox/Google Chrome. I am attaching such a comparison:

The input is overflowing, and the margin between the top text and the player list table is off.

Workarounds

There are possible workarounds that might help investigate this case.

Workaround n.1

If one of the other items sets a minimum height (or width for the row direction), it no longer stretches so unexpectedly:

#contents > div:first-child {
    min-height: 30dp;
}

This minimum size must be set directly for the item. If it is a property of its child(ren), the output is broken once again.

Workaround n.2

Other elements set the flex property:

#contents > div:first-child, #contents > div:last-child {
    flex: 1;
}

Elements no longer move up and down. However, the margin is ignored, and the last element completely overflows the body.

[mikke89]

Thanks a lot for typing out the issue, I'll take a closer look at it soon.

[mikke89]

Hey, I finally took a deeper look at this one. And you're completely right, this was indeed the lack of min-size content calculations. Or more precisely, we don't handle the automatic minimum size of flex items. In flex layout, the minimum size itself is modified to account for min-content size. Generally, it is quite expensive to calculate this. However, in this specific sample, we were already doing content-based sizing, so we already had that value readily available. It was a small change to fix it, so I went ahead with that.

In flex layout, this minimum size should be applied in all cases, not just during content-based sizing. However, I kept it to this single case for now, (1) since enabling it in all cases would be a large performance regression in several cases, and (2) we currently don't have a way to do min-content size in the width direction in any case, so this would only be possible in column layout for now.

I found another optimization. There are some conditions where we don't need to do the formatting to determine the cross axis size (width in this case). It simply has no effect on the end result under these specific conditions, see the commit (e9451c9) for details.

---

With those optimization, I'd also like to discuss some recommendations I would make for this layout. I believe the intended way is for the size of the header and footer to not scale flexibly, but rather to the size of their contents. This behavior could be made more explicit by setting them to flex: none, not allowing them to grow or shrink. This will involve content-based sizing, but in these cases that is relatively trivial. Further, I expect that the player list itself is intended to take up the rest of the available size. Using content based sizing here does not really do anything of value. Thus, this element should be set to flex: 1, allowing it to expand into the remaining space. That way, we avoid the unnecessary expensive formatting of the player list, while ensuring that the header and footer have their intended size, regardless of any automatic minimum size. I believe this should have worked also before the new changes.

Also, I suspect that the footer's align-self: flex-end is not the intended behavior. This property aligns the item in the cross axis (that is, horizontally in this case), but it looks like the intention was to make this align to the bottom. This property has no effect now, it only makes it harder to use the new cross-axis optimizations, resulting in an extra unnecessary content-based sizing step.

The new optimizations, along with the above changes to the sample, will ensure that only a single formatting step is done for each element during the flex layout formatting. However, the player list itself will do some extra formatting due to the scrollbar appearing, but that is a separate issue, and we can get around that too by implementing the scrollbar-gutter property as discussed, and also see some improvements by implementing an early-out from table layout.

[ShawnCZek]

First of all, thank you for looking into this problem so quickly. As there were possible workarounds that worked well for the described use case, the issue was not urgent for me. Nonetheless, this can be quite a common case, especially when coming from the world of web dev, so I am pleased to see this being worked on.

I have tried merging these fixes into the sample branch. Unsurprisingly, the issue described is indeed fixed.

Also, thank you for all the recommendations; they are definitely helpful and highly appreciated! However, this was only a sample project to reproduce the problem, including styles from my WIP project. As we are coming close to releasing it, I have already made quite some changes to the styles and data processing, as discussed in the thread linked above, to optimize all these cases and to tidy up the styles. So, for instance, the align-self was indeed incorrect; that is a nice catch!

[mikke89]

I'm glad to hear the changes are working well for you!

And I fully understand that this was a small sample demo. It was actually a really good example for me to play around with, since it exercised several aspects where it looked like we could optimize without too much effort, and I suspect in fairly common situations, so thanks for that!