[webkit-reviews] review denied: [Bug 225278] [css-flexbox] `gap` does not work correctly when `flex-direction: column-reverse` is applied : [Attachment 446041] Patch

Tue Dec 7 01:08:09 PST 2021

Sergio Villar Senin <svillar at igalia.com> has denied Vitaly Dyachkov
<obyknovenius at me.com>'s request for review:
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

Attachment 446041: Patch

https://bugs.webkit.org/attachment.cgi?id=446041&action=review

--- Comment #5 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

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