WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(24.27 KB, patch)
2020-09-30 05:25 PDT
,
Sergio Villar Senin
rego
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/63277872
>
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
Created
attachment 409876
[details]
Patch
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
Created
attachment 410109
[details]
Patch
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
Committed
r267829
: <
https://trac.webkit.org/changeset/267829
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug