CSS `gap` utility does not work correctly when `flex-direction: column-reverse` is applied.
<rdar://problem/77708991>
I created a minimal project to showcase the issue here. Notice the children element in the enclosing column-reverse layout are given no gap. https://bugs.webkit.org/show_bug.cgi?id=225278
I ran into the same issue. Here is a minimal reproduction example ```html <div> <span>First</span> <span>Second</span> <span>Third</span> </div> ``` ```css div { display: flex; flex-direction: column-reverse; gap: 20px; background-color: lightgrey; } span { background-color: lightblue; } ```
Created attachment 446041 [details] Patch
Comment on attachment 446041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446041&action=review Thanks for the patch. Looks great overall but you have to fix the issue about adding the gap after the items. That will also require more testing, either in the same file or in another one. > Source/WebCore/ChangeLog:7 > + You need to add here one or more paragraphs describing the fix. > Source/WebCore/rendering/RenderFlexibleBox.cpp:2045 > + mainAxisOffset -= justifyContentSpaceBetweenChildren(availableFreeSpace, distribution, children.size()) + gapBetweenItems; This is unconditionally adding the gap after the last child. Gap only make sense in between items. > LayoutTests/css3/flexbox/column-reverse-gap.html:2 > +<html> Nit: you don't need the <html> tag. > LayoutTests/css3/flexbox/column-reverse-gap.html:10 > + background-color: blue; I don't think you need min-height and background-color > LayoutTests/css3/flexbox/column-reverse-gap.html:15 > + } Same for those rules
Comment on attachment 446041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446041&action=review >> Source/WebCore/rendering/RenderFlexibleBox.cpp:2045 >> + mainAxisOffset -= justifyContentSpaceBetweenChildren(availableFreeSpace, distribution, children.size()) + gapBetweenItems; > > This is unconditionally adding the gap after the last child. Gap only make sense in between items. That's true and I could wrap adding a gap into "if" statement but I don't think it's really necessary. As far as I understood, all the measurements were already done before this method is called. Here we simply move the children around. Also notice that we first set the current child's location and then decrease mainAxisOffset for the next child. That means that since they're no children after the last child, this offset will just be ignored. Am I missing something here? >> LayoutTests/css3/flexbox/column-reverse-gap.html:2 >> +<html> > > Nit: you don't need the <html> tag. Just for my understanding, what is the point of not using standard tags like <hmtl>, <title>, <body>? Just to keep test files as minimal as possible? I see that they are sometimes used on some tests and sometimes they are not. >> LayoutTests/css3/flexbox/column-reverse-gap.html:10 >> + background-color: blue; > > I don't think you need min-height and background-color You're right, "min-height" is redundant here. The reason, I added "background-color" is for visual "debugging". I don't know if it's important or we only care about automatic tests but I found it helpful while I was working on it. >> LayoutTests/css3/flexbox/column-reverse-gap.html:15 >> + } > > Same for those rules It's possible to use just "height" instead of "min-height" here. But I need to know the children's height to predict their vertical offset. About "background-color" please see the previous comment.
> Just for my understanding, what is the point of not using standard tags like <hmtl>, <title>, <body>? Just to keep test files as minimal as possible? I see that they are sometimes used on some tests and sometimes they are not. I meant <html> and <head> of course :)
Comment on attachment 446041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446041&action=review >>> Source/WebCore/rendering/RenderFlexibleBox.cpp:2045 >>> + mainAxisOffset -= justifyContentSpaceBetweenChildren(availableFreeSpace, distribution, children.size()) + gapBetweenItems; >> >> This is unconditionally adding the gap after the last child. Gap only make sense in between items. > > That's true and I could wrap adding a gap into "if" statement but I don't think it's really necessary. As far as I understood, all the measurements were already done before this method is called. Here we simply move the children around. Also notice that we first set the current child's location and then decrease mainAxisOffset for the next child. That means that since they're no children after the last child, this offset will just be ignored. Am I missing something here? Right, currently it does not matter a lot whether or not modifying the mainAxisOffset because there are no further usages of the variable. However what I want to prevent is someone eventually innocently adding some extra code by the end of the method using an invalid mainAxisOffset. So yeah you can forget about adding extra tests (since right now you're not adding any visible wrong behaviour) but let's prevent the variable update from happening. >>> LayoutTests/css3/flexbox/column-reverse-gap.html:2 >>> +<html> >> >> Nit: you don't need the <html> tag. > > Just for my understanding, what is the point of not using standard tags like <hmtl>, <title>, <body>? Just to keep test files as minimal as possible? I see that they are sometimes used on some tests and sometimes they are not. It's basically redundant since your using DOCTYPE html >>> LayoutTests/css3/flexbox/column-reverse-gap.html:10 >>> + background-color: blue; >> >> I don't think you need min-height and background-color > > You're right, "min-height" is redundant here. > The reason, I added "background-color" is for visual "debugging". I don't know if it's important or we only care about automatic tests but I found it helpful while I was working on it. You can keep the background color. >>> LayoutTests/css3/flexbox/column-reverse-gap.html:15 >>> + } >> >> Same for those rules > > It's possible to use just "height" instead of "min-height" here. But I need to know the children's height to predict their vertical offset. About "background-color" please see the previous comment. Let's use height then. BTW have you considered adding the test to the WPT suite and then import it into WebKit? I think it does clearly qualify. I know that right now it's a bit of a burden so feel free to ignore this suggestion (or to do it in a follow up patch :-)).
Comment on attachment 446041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446041&action=review >>>> LayoutTests/css3/flexbox/column-reverse-gap.html:15 >>>> + } >>> >>> Same for those rules >> >> It's possible to use just "height" instead of "min-height" here. But I need to know the children's height to predict their vertical offset. About "background-color" please see the previous comment. > > Let's use height then. > > BTW have you considered adding the test to the WPT suite and then import it into WebKit? I think it does clearly qualify. I know that right now it's a bit of a burden so feel free to ignore this suggestion (or to do it in a follow up patch :-)). I haven't considered that just because I was not aware of WPT at all. Actually, it's my first ever patch to WebKit and I'm learning by doing. But thanks for the advice. Will do it... somehow :)
Created attachment 446196 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 446197 [details] Patch
Created attachment 446204 [details] Patch
Created attachment 446206 [details] Patch
I've opened a PR in the WPT repo (https://github.com/web-platform-tests/wpt/pull/31942) but how can I add it to the "See also" section?
Created attachment 446319 [details] Patch
(In reply to Vitaly Dyachkov from comment #15) > I've opened a PR in the WPT repo > (https://github.com/web-platform-tests/wpt/pull/31942) but how can I add it > to the "See also" section? That was really awesome. The changes in WPT were approved you just need to click on "Squash and merge" in github to get your first patch for WPT integrated :) I added it for you, maybe you need to be a committer to do that not sure...
Comment on attachment 446319 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446319&action=review Just fix the minor nit I mentioned and I'll give the r+. BTW as you are not committer you'd very likely should also set the cq? flag in the patch so that I could flip it to cq+ as well to get the patch landed > Source/WebCore/ChangeLog:11 > + Take gapBetweenItems into account during RenderFlexibleBox::layoutColumnReverse. Nit: this description goes in between the "Reviewed by" line and the "Test" line. Sorry about that but that's what the coding style says
(In reply to Sergio Villar Senin from comment #17) > That was really awesome. The changes in WPT were approved you just need to > click on "Squash and merge" in github to get your first patch for WPT > integrated :) I'm really excited! But seems like I can't merge it. Github says that only those with write access can do that. I just don't have the merge button.
Created attachment 446331 [details] Patch
Comment on attachment 446331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446331&action=review > Source/WebCore/ChangeLog:9 > + Take gapBetweenItems into account during RenderFlexibleBox::layoutColumnReverse. Sorry for adding another review loop, but this should be a higher level description of the change instead of a literal description of the code change. The point is to let others not specially familiar with the code graps an idea about what the patch fixes. I'd write something like: "Whenever flex-direction: column-reverse is specified flexbox does always compute the flex item sizes and positions ignoring the -reverse direction until the very end. After completing the computations we just need to swap offsets to get the reversed positions. The code was properly considering space between items added by content justification but it was not adding gaps. Fixed it by adding the gap size to the flex items' offsets" Feel free to adapt it, the key is that you have to explain what the problem was and how it was fixed.
Created attachment 446347 [details] Patch
Comment on attachment 446347 [details] Patch Thanks Vitaly for your patience! And congrats for the first WebKit and WPT patches!
(In reply to Sergio Villar Senin from comment #23) > Comment on attachment 446347 [details] > Patch > > Thanks Vitaly for your patience! And congrats for the first WebKit and WPT > patches! Thank you for your review. It really helped me to get through.
Committed r286654 (?): <https://commits.webkit.org/r286654> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446347 [details].
This fix should be live in iOS 15.4 Beta 1, and macOS 12.3 Beta 1.