Bug 206767 - [css-flexbox] Implement row-gap and column-gap for flex layout
Summary: [css-flexbox] Implement row-gap and column-gap for flex layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2020-01-24 12:51 PST by David Grogan
Modified: 2021-09-11 11:00 PDT (History)
30 users (show)

See Also:


Attachments
Patch (23.72 KB, patch)
2020-09-28 02:40 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (24.27 KB, patch)
2020-09-30 05:25 PDT, Sergio Villar Senin
rego: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2020-01-24 12:51:42 PST
We're planning to make flex containers obey the row-gap and column-gap properties from https://drafts.csswg.org/css-align/#gap-flex .
Comment 1 David Grogan 2020-01-24 12:52:08 PST
"We" meaning chromium.
Comment 2 Masataka Yakura 2020-01-26 11:44:04 PST
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.
Comment 3 Laurie 2020-05-10 02:42:44 PDT
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?
Comment 4 Radar WebKit Bug Importer 2020-05-15 09:59:17 PDT
<rdar://problem/63277872>
Comment 5 Ole Strøm 2020-09-24 06:36:22 PDT
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"?
Comment 6 Sergio Villar Senin 2020-09-24 07:08:00 PDT
(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
Comment 7 Sergio Villar Senin 2020-09-28 02:40:39 PDT
Created attachment 409876 [details]
Patch
Comment 8 Ole Strøm 2020-09-28 03:00:23 PDT
(In reply to Sergio Villar Senin from comment #6)
> Be patient :). There are some good news coming soon

INDEED 🎉
Comment 9 Manuel Rego Casasnovas 2020-09-29 03:26:43 PDT
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).
Comment 10 Sergio Villar Senin 2020-09-30 04:42:37 PDT
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
Comment 11 Sergio Villar Senin 2020-09-30 04:50:31 PDT
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.
Comment 12 Sergio Villar Senin 2020-09-30 05:25:04 PDT
Created attachment 410109 [details]
Patch
Comment 13 Manuel Rego Casasnovas 2020-09-30 05:49:15 PDT
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.
Comment 14 Sergio Villar Senin 2020-10-01 04:33:06 PDT
Committed r267829: <https://trac.webkit.org/changeset/267829>