RESOLVED FIXED Bug 225278
[css-flexbox] `gap` does not work correctly when `flex-direction: column-reverse` is applied 
https://bugs.webkit.org/show_bug.cgi?id=225278
Summary [css-flexbox] `gap` does not work correctly when `flex-direction: column-reve...
peter
Reported 2021-05-02 03:01:46 PDT
CSS `gap` utility does not work correctly when `flex-direction: column-reverse` is applied.
Attachments
Patch (6.06 KB, patch)
2021-12-06 08:19 PST, Vitaly Dyachkov 		no flags
DetailsFormatted DiffDiff
Patch (6.50 KB, patch)
2021-12-07 10:08 PST, Vitaly Dyachkov 		no flags
DetailsFormatted DiffDiff
Patch (6.05 KB, patch)
2021-12-07 10:22 PST, Vitaly Dyachkov 		no flags
DetailsFormatted DiffDiff
Patch (6.04 KB, patch)
2021-12-07 11:07 PST, Vitaly Dyachkov 		no flags
DetailsFormatted DiffDiff
Patch (6.21 KB, patch)
2021-12-07 11:19 PST, Vitaly Dyachkov 		no flags
DetailsFormatted DiffDiff
Patch (6.76 KB, patch)
2021-12-08 00:08 PST, Vitaly Dyachkov 		no flags
DetailsFormatted DiffDiff
Patch (6.76 KB, patch)
2021-12-08 00:59 PST, Vitaly Dyachkov 		no flags
DetailsFormatted DiffDiff
Patch (7.13 KB, patch)
2021-12-08 04:41 PST, Vitaly Dyachkov 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-05-09 03:02:14 PDT
<rdar://problem/77708991>
parion
Comment 2 2021-06-04 10:23:21 PDT
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
bugmenot
Comment 3 2021-08-15 03:52:39 PDT
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; } ```
Vitaly Dyachkov
Comment 4 2021-12-06 08:19:32 PST
Created attachment 446041 [details] Patch
Sergio Villar Senin
Comment 5 2021-12-07 01:08:09 PST
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
Vitaly Dyachkov
Comment 6 2021-12-07 06:35:10 PST
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.
Vitaly Dyachkov
Comment 7 2021-12-07 06:49:44 PST
> 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 :)
Sergio Villar Senin
Comment 8 2021-12-07 08:18:09 PST
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 :-)).
Vitaly Dyachkov
Comment 9 2021-12-07 08:43:55 PST
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 :)
Vitaly Dyachkov
Comment 10 2021-12-07 10:08:19 PST
Created attachment 446196 [details] Patch
EWS Watchlist
Comment 11 2021-12-07 10:09:07 PST
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
Vitaly Dyachkov
Comment 12 2021-12-07 10:22:43 PST
Created attachment 446197 [details] Patch
Vitaly Dyachkov
Comment 13 2021-12-07 11:07:52 PST
Created attachment 446204 [details] Patch
Vitaly Dyachkov
Comment 14 2021-12-07 11:19:02 PST
Created attachment 446206 [details] Patch
Vitaly Dyachkov
Comment 15 2021-12-07 12:48:28 PST
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?
Vitaly Dyachkov
Comment 16 2021-12-08 00:08:31 PST
Created attachment 446319 [details] Patch
Sergio Villar Senin
Comment 17 2021-12-08 00:39:10 PST
(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...
Sergio Villar Senin
Comment 18 2021-12-08 00:44:31 PST
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
Vitaly Dyachkov
Comment 19 2021-12-08 00:48:15 PST
(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.
Vitaly Dyachkov
Comment 20 2021-12-08 00:59:30 PST
Created attachment 446331 [details] Patch
Sergio Villar Senin
Comment 21 2021-12-08 04:18:35 PST
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.
Vitaly Dyachkov
Comment 22 2021-12-08 04:41:14 PST
Created attachment 446347 [details] Patch
Sergio Villar Senin
Comment 23 2021-12-08 07:10:09 PST
Comment on attachment 446347 [details] Patch Thanks Vitaly for your patience! And congrats for the first WebKit and WPT patches!
Vitaly Dyachkov
Comment 24 2021-12-08 07:33:46 PST
(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.
EWS
Comment 25 2021-12-08 07:35:48 PST
Committed r286654 (?): <https://commits.webkit.org/r286654> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446347 [details].
Brent Fulgham
Comment 26 2022-02-04 09:29:58 PST
This fix should be live in iOS 15.4 Beta 1, and macOS 12.3 Beta 1.
Note You need to log in before you can comment on or make changes to this bug.