| Summary: | [css-flexbox] Implement row-gap and column-gap for flex layout | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Grogan <dgrogan> | ||||||
| Component: | Layout and Rendering | Assignee: | Sergio Villar Senin <svillar> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andrea, ap, bfulgham, bradbice, changseok, clopez, dianeberlusconi, eddie.chen, eoconnor, esprehn+autocc, ews-watchlist, glenn, jarilittlenen, kondapallykalyan, kyle.bavender, lpoulter1984, mail, mcoker, mehmetgelisin, me, me, michaelcpuckett, mjs, moritz.mahringer, myakura.web, olestr, pdr, rego, simon.fraser, svillar, webkit-bug-importer, zalan | ||||||
| Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
David Grogan
2020-01-24 12:51:42 PST
"We" meaning chromium. 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. 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? 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"? (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 Created attachment 409876 [details]
Patch
(In reply to Sergio Villar Senin from comment #6) > Be patient :). There are some good news coming soon INDEED 🎉 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 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 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. Created attachment 410109 [details]
Patch
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. Committed r267829: <https://trac.webkit.org/changeset/267829> |