RESOLVED FIXED 206767
[css-flexbox] Implement row-gap and column-gap for flex layout
https://bugs.webkit.org/show_bug.cgi?id=206767
Summary [css-flexbox] Implement row-gap and column-gap for flex layout
David Grogan
Reported 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 .
Attachments
Patch (23.72 KB, patch)
2020-09-28 02:40 PDT, Sergio Villar Senin
no flags
Patch (24.27 KB, patch)
2020-09-30 05:25 PDT, Sergio Villar Senin
rego: review+
David Grogan
Comment 1 2020-01-24 12:52:08 PST
"We" meaning chromium.
Masataka Yakura
Comment 2 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.
Laurie
Comment 3 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?
Radar WebKit Bug Importer
Comment 4 2020-05-15 09:59:17 PDT
Ole Strøm
Comment 5 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"?
Sergio Villar Senin
Comment 6 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
Sergio Villar Senin
Comment 7 2020-09-28 02:40:39 PDT
Ole Strøm
Comment 8 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 🎉
Manuel Rego Casasnovas
Comment 9 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).
Sergio Villar Senin
Comment 10 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
Sergio Villar Senin
Comment 11 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.
Sergio Villar Senin
Comment 12 2020-09-30 05:25:04 PDT
Manuel Rego Casasnovas
Comment 13 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.
Sergio Villar Senin
Comment 14 2020-10-01 04:33:06 PDT
Note You need to log in before you can comment on or make changes to this bug.