Bug 133225 – [CSS Grid Layout] Support for align-self and align-items in grid layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

Both align-self and align-items should apply to the grid layout items.

[Javier Fernandez]

Created attachment 251634 [details]
Patch

[Darin Adler]

Comment on attachment 251634 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251634&action=review

> Source/WebCore/ChangeLog:9
> +        different wirting-modes and direction. Margins, borders and paddings are also

wirting

> Source/WebCore/rendering/RenderGrid.cpp:1289
> +    // A margin basically has three types: fixed, percentage, and auto (variable).
> +    // Auto and percentage margins simply become 0 when computing min/max width.
> +    // Fixed margins can be added in as is.
> +    Length marginLeft = child.style().marginLeft();
> +    Length marginRight = child.style().marginRight();
> +    LayoutUnit margin = 0;
> +    if (marginLeft.isFixed())
> +        margin += marginLeft.value();
> +    if (marginRight.isFixed())
> +        margin += marginRight.value();
> +    return margin;

How about writing this with helper functions?

    // A margin has three types: fixed, percentage, and auto (variable).
    // Auto and percentage margins become 0 when computing min/max width.
    // Fixed margins should be added in as is.

    static LayoutUnit fixedValue(const Length& length)
    {
        return length.isFixed() ? length.value() : 0;
    }

    static LayoutUnit marginWidthForChild(const RenderBox& child)
    {
        return fixedValues(child.style().marginLeft()) + fixedValue(child.style().marginRight());
    }

    static LayoutUnit marginHeightForChild(const RenderBox& child)
    {
        return fixedValue(child.style().marginTop()) + fixedValue(child.style().marginBottom());
    }

I think this is easier to read than the longer functions with the local variables. Inline as needed. Name the helper differently if you like.

> Source/WebCore/rendering/RenderGrid.cpp:1361
> +    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
> +    switch (RenderStyle::resolveAlignment(style(), child.style(), ItemPositionStretch)) {

Given this whole function ends up just deciding on start, end or centered, maybe we should write it that way with an enum rather than sprinkling in all the different function calls throughout. We should look into it again with an eye toward clarity when reading the function.

> Source/WebCore/rendering/RenderGrid.cpp:1363
> +        // If orthogonal writing-modes, this computes to 'start'.

Generally speaking these should be:

    return <boolean expression about whether to use start or end> ? startOfRowForChild(child) : endOfRowForChild(child);

Rather than this sequence of multiple return statements.

> Source/WebCore/rendering/RenderGrid.cpp:1373
> +        return startOfRowForChild(child);
> +    case ItemPositionSelfEnd:

Paragraphing here is strange. If you are including blank lines within cases, then please leave a blank line before the next case too.

> Source/WebCore/rendering/RenderGrid.cpp:1392
> +        // The alignment axis (column axis) and the inline axis are parallell in
> +        // orthogonal writing mode.
> +        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
> +        if (hasOrthogonalWritingMode)
> +            return startOfRowForChild(child);
> +
> +        // Otherwise this this is equivalent to 'start'.
> +        return startOfRowForChild(child);

This seems to be written out too long. I think a ? : would be much better. Also, this is startOfRowForChild in both places. Is that because of the FIXME? Lets not write it this strange way.

> Source/WebCore/rendering/RenderGrid.cpp:1401
> +        // The alignment axis (column axis) and the inline axis are parallell in
> +        // orthogonal writing mode.
> +        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
> +        if (hasOrthogonalWritingMode)
> +            return endOfRowForChild(child);
> +
> +        // Otherwise this this is equivalent to 'start'.
> +        return startOfRowForChild(child);

This seems to be written out too long. I think a ? : would be much better.

> Source/WebCore/rendering/RenderGrid.cpp:1406
> +        // Only used in flex layout, for other layout, it's equivalent to 'start'.
> +    case ItemPositionFlexStart:
> +    case ItemPositionStart:

Indentation of this comment is wrong. It should be moved left or put on the same line with FlexStart. Indented like this it seems to be part of the previous case.

> Source/WebCore/rendering/RenderGrid.cpp:1410
> +        // Only used in flex layout, for other layout, it's equivalent to 'end'.
> +    case ItemPositionFlexEnd:
> +    case ItemPositionEnd:

Ditto.

> Source/WebCore/rendering/RenderGrid.h:123
> +    LayoutUnit startOfRowForChild(const RenderBox& child) const;
> +    LayoutUnit endOfRowForChild(const RenderBox& child) const;
> +    LayoutUnit centeredRowPositionForChild(const RenderBox&) const;
> +    LayoutUnit rowPositionForChild(const RenderBox&) const;

Should either include or omit the argument name “child” consistently.

> Source/WebCore/rendering/RenderGrid.h:133
> +    bool allowedToStretchLogicalHeightForChild(const RenderBox& child) const;
> +    bool needToStretchChildLogicalHeight(const RenderBox&) const;
> +    LayoutUnit marginLogicalHeightForChild(const RenderBox&) const;
> +    LayoutUnit computeMarginLogicalHeightForChild(const RenderBox&) const;

Should either include or omit the argument name “child” consistently.

[Javier Fernandez]

Comment on attachment 251634 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251634&action=review

>> Source/WebCore/rendering/RenderGrid.cpp:1289
>> +    return margin;
> 
> How about writing this with helper functions?
> 
>     // A margin has three types: fixed, percentage, and auto (variable).
>     // Auto and percentage margins become 0 when computing min/max width.
>     // Fixed margins should be added in as is.
> 
>     static LayoutUnit fixedValue(const Length& length)
>     {
>         return length.isFixed() ? length.value() : 0;
>     }
> 
>     static LayoutUnit marginWidthForChild(const RenderBox& child)
>     {
>         return fixedValues(child.style().marginLeft()) + fixedValue(child.style().marginRight());
>     }
> 
>     static LayoutUnit marginHeightForChild(const RenderBox& child)
>     {
>         return fixedValue(child.style().marginTop()) + fixedValue(child.style().marginBottom());
>     }
> 
> I think this is easier to read than the longer functions with the local variables. Inline as needed. Name the helper differently if you like.

Yeah, that's definitively how I should have do it. 

However, I ended up removing that code because it seems that Flexbox stretching logic uses child.verticalMarginExtent() or child.horizontalMarginExtent() functions, which are precisely what I need to implement the Gird stretching logic too.

>> Source/WebCore/rendering/RenderGrid.cpp:1361
>> +    switch (RenderStyle::resolveAlignment(style(), child.style(), ItemPositionStretch)) {
> 
> Given this whole function ends up just deciding on start, end or centered, maybe we should write it that way with an enum rather than sprinkling in all the different function calls throughout. We should look into it again with an eye toward clarity when reading the function.

Even that it's not implemented yet, Baseline alignment would imply a different result. Anyway, do you mean translating ItemPosition to an internal enumeration with just START, END and CENTER values ? In that case, we would need a function to return the corresponding layout point.

>> Source/WebCore/rendering/RenderGrid.cpp:1363
>> +        // If orthogonal writing-modes, this computes to 'start'.
> 
> Generally speaking these should be:
> 
>     return <boolean expression about whether to use start or end> ? startOfRowForChild(child) : endOfRowForChild(child);
> 
> Rather than this sequence of multiple return statements.

Agree.

>> Source/WebCore/rendering/RenderGrid.cpp:1373
>> +    case ItemPositionSelfEnd:
> 
> Paragraphing here is strange. If you are including blank lines within cases, then please leave a blank line before the next case too.

I'll avoid paragraphing.

>> Source/WebCore/rendering/RenderGrid.cpp:1392
>> +        return startOfRowForChild(child);
> 
> This seems to be written out too long. I think a ? : would be much better. Also, this is startOfRowForChild in both places. Is that because of the FIXME? Lets not write it this strange way.

Sure.

>> Source/WebCore/rendering/RenderGrid.cpp:1401
>> +        return startOfRowForChild(child);
> 
> This seems to be written out too long. I think a ? : would be much better.

Done.

[Javier Fernandez]

Created attachment 251708 [details]
Patch

[Javier Fernandez]

Created attachment 251709 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 251709 [details]:

media/track/track-active-cues.html bug 144236 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 251709 [details]
Patch

Clearing flags on attachment: 251709

Committed r183370: <http://trac.webkit.org/changeset/183370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.