Bug 206767 – [css-flexbox] Implement row-gap and column-gap for flex layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[David Grogan]

We're planning to make flex containers obey the row-gap and column-gap properties from https://drafts.csswg.org/css-align/#gap-flex .

[David Grogan]

"We" meaning chromium.

[Masataka Yakura]

That gap support in Flexbox has been supported in Firefox for quite some time. It's really useful for some horizontal list of links (e.g. ones in the header in https://webkit.org/blog/ ).


Although that was a nice addition the way CSSWG did was not easily feature detectable (just repurpose `gap` props apply to Flex and add no `flex-gap` properties so `@supports` didn't work...)... I'd love Chromium/WebKit support the feature around the same time.

[Laurie]

Chromium now supports this along with Firefox. It would be great to have webkit also support flex gap. Is this something that is being planned at all?

[Radar WebKit Bug Importer]

<rdar://problem/63277872>

[Ole Strøm]

I keep on really wanting to use this one when using flex.

It's so close to being a reality; https://caniuse.com/flexbox-gap - just Safari missing, really.

Perhaps this issue should be renamed to reflect the now landed property name of just "gap"?

[Sergio Villar Senin]

(In reply to Ole Strøm from comment #5)
> I keep on really wanting to use this one when using flex.
> 
> It's so close to being a reality; https://caniuse.com/flexbox-gap - just
> Safari missing, really.
> 
> Perhaps this issue should be renamed to reflect the now landed property name
> of just "gap"?

Be patient :). There are some good news coming soon

[Sergio Villar Senin]

Created attachment 409876 [details]
Patch

[Ole Strøm]

(In reply to Sergio Villar Senin from comment #6)
> Be patient :). There are some good news coming soon

INDEED 🎉

[Manuel Rego Casasnovas]

Comment on attachment 409876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409876&action=review

Some comments inline but this looks great, really excited to see this happening.

> Source/WebCore/rendering/FlexibleBoxAlgorithm.cpp:81
> +    if (sumHypotheticalMainSize > 0) {

Do we need this check or we could do something like "m_gapBetweenItems > 0"?

> Source/WebCore/rendering/FlexibleBoxAlgorithm.cpp:85
> +        ASSERT(sumFlexBaseSize >= m_gapBetweenItems);

Why >= here and > in the previous ASSERT?

> Source/WebCore/rendering/FlexibleBoxAlgorithm.cpp:87
> +    }

Nit: Not a strong opinion, but in other similar loops insead of adding the gap during the loop, you've added it later. Maybe we should follow the same approach in all of them and use something like "m_allItems.size() - 1 * m_gapBetweeItems".

> Source/WebCore/rendering/RenderFlexibleBox.cpp:123
> +    if (!isColumnFlow() && numItemsWithNormalLayout > 0) {

Why not "numItemsWithNormalLayout > 1"? Otherwise if it's "1" |inlineGapSize| would be 0.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:945
> +

Super-nit: I'd remove this empty line, as we're still computing the |remainingFreeSpace|.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:962
> +    if (!isColumnFlow())

Maybe add "&& numLines > 1", otherwise you don't need to call setLogicalHeight().

> LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/gap-017-expected.txt:3
> +PASS .button 1 

You need to fix the trailing space here due to a recent change (see r267640).

[Sergio Villar Senin]

Comment on attachment 409876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409876&action=review

Thanks for the review.

>> Source/WebCore/rendering/FlexibleBoxAlgorithm.cpp:81
>> +    if (sumHypotheticalMainSize > 0) {
> 
> Do we need this check or we could do something like "m_gapBetweenItems > 0"?

In theory you could have sumHypotheticalMainSize == 0 and m_gapBetweenItems > 0 so yeah I think the check is correct (you don't want to end up with a negative sumHypotheticalMainSize)

>> Source/WebCore/rendering/FlexibleBoxAlgorithm.cpp:85
>> +        ASSERT(sumFlexBaseSize >= m_gapBetweenItems);
> 
> Why >= here and > in the previous ASSERT?

Actually I think the ASSERTs are incorrect because at this point the sums might be negative due to negative margins. I'll check that.

>> Source/WebCore/rendering/FlexibleBoxAlgorithm.cpp:87
>> +    }
> 
> Nit: Not a strong opinion, but in other similar loops insead of adding the gap during the loop, you've added it later. Maybe we should follow the same approach in all of them and use something like "m_allItems.size() - 1 * m_gapBetweeItems".

Makes sense, thanks for the suggestion.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:123
>> +    if (!isColumnFlow() && numItemsWithNormalLayout > 0) {
> 
> Why not "numItemsWithNormalLayout > 1"? Otherwise if it's "1" |inlineGapSize| would be 0.

Right, good point. The condition was there just to avoid negatives but in the case of 1 the whole if block would be a no-op.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:962
>> +    if (!isColumnFlow())
> 
> Maybe add "&& numLines > 1", otherwise you don't need to call setLogicalHeight().

Good point

[Sergio Villar Senin]

Comment on attachment 409876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409876&action=review

>>> Source/WebCore/rendering/FlexibleBoxAlgorithm.cpp:87
>>> +    }
>> 
>> Nit: Not a strong opinion, but in other similar loops insead of adding the gap during the loop, you've added it later. Maybe we should follow the same approach in all of them and use something like "m_allItems.size() - 1 * m_gapBetweeItems".
> 
> Makes sense, thanks for the suggestion.

I remembered why  we cannot do that. We need to compute it on every iteration because there is a check at the beginning of the loop which depends on the value of sumHypotheticalMainSize.

[Sergio Villar Senin]

Created attachment 410109 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 410109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410109&action=review

r=me, please wait for green EWSs before landing

> Source/WebCore/rendering/FlexibleBoxAlgorithm.cpp:78
> +    if (!lineItems.isEmpty()) {

Maybe you can add "m_gapBetweenItems > 0" too, otherwise you don't need to do anything.

[Sergio Villar Senin]

Committed r267829: <https://trac.webkit.org/changeset/267829>