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

Tue Dec 7 08:18:09 PST 2021

https://bugs.webkit.org/show_bug.cgi?id=225278

--- Comment #8 from Sergio Villar Senin <svillar at igalia.com> ---
Comment on attachment 446041
  --> https://bugs.webkit.org/attachment.cgi?id=446041
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 :-)).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211207/487e43df/attachment.htm>