Bug 225278

Summary:

[css-flexbox] `gap` does not work correctly when `flex-direction: column-reverse` is applied

Product:

WebKit

Reporter:

peter <peponeupauer>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, bugmenot, changseok, clopez, esprehn+autocc, ews-watchlist, glenn, griffinwiebel, kondapallykalyan, obyknovenius, pdr, simon.fraser, svillar, webkit-bug-importer, youennf, zalan

Priority:

Keywords:

InRadar

Version:

Safari 14

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://github.com/web-platform-tests/wpt/pull/31942

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (6.50 KB, patch) 2021-12-07 10:08 PST, Vitaly Dyachkov	no flags	Details Formatted Diff Diff
Patch (6.05 KB, patch) 2021-12-07 10:22 PST, Vitaly Dyachkov	no flags	Details Formatted Diff Diff
Patch (6.04 KB, patch) 2021-12-07 11:07 PST, Vitaly Dyachkov	no flags	Details Formatted Diff Diff
Patch (6.21 KB, patch) 2021-12-07 11:19 PST, Vitaly Dyachkov	no flags	Details Formatted Diff Diff
Patch (6.76 KB, patch) 2021-12-08 00:08 PST, Vitaly Dyachkov	no flags	Details Formatted Diff Diff
Patch (6.76 KB, patch) 2021-12-08 00:59 PST, Vitaly Dyachkov	no flags	Details Formatted Diff Diff
Patch (7.13 KB, patch) 2021-12-08 04:41 PST, Vitaly Dyachkov	no flags	Details Formatted Diff Diff
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.