Bug 160076 – [css-grid] Stretch alignment doesn't work for orthogonal flows

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

[Javier Fernandez]

We are progressively adding support on grid for orthogonal flows, however, stretching is not possible yet in these cases.

[Javier Fernandez]

Created attachment 284691 [details]
Patch

[Darin Adler]

Comment on attachment 284691 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:2190
> +    bool childHasAutoSizeInColumnAxis = isHorizontalMode ? childStyle.height().isAuto() : childStyle.width().isAuto();
> +    bool childHasAutoSizeInRowAxis = isHorizontalMode ? childStyle.width().isAuto() : childStyle.height().isAuto();
> +    bool allowedToStretchChildAlongColumnAxis = childHasAutoSizeInColumnAxis && !hasAutoMarginsInColumnAxis(child);
> +    bool allowedToStretchChildAlongRowAxis = childHasAutoSizeInRowAxis && !hasAutoMarginsInRowAxis(child);
> +    bool stretchingAlongRowAxis = childStyle.resolvedJustifySelf(gridStyle, selfAlignmentNormalBehavior).position() == ItemPositionStretch;
> +    bool stretchingAlongColumnAxis = childStyle.resolvedAlignSelf(gridStyle, selfAlignmentNormalBehavior).position() == ItemPositionStretch;

Inefficient to compute both axes and then only use the one that is needed below. Maybe these can be functions instead of booleans so we can call only the one that is needed.

Also inefficient to compute stretchingAlongAxis when allowedToStretchChildAlongAxis is already false. Early return style would work for that.

> Source/WebCore/rendering/RenderGrid.cpp:2203
> +    if (allowedToStretchChildBlockSize) {
> +        LayoutUnit stretchedLogicalHeight = availableAlignmentSpaceForChildBeforeStretching(overrideContainingBlockContentSizeForChild(child, childBlockDirection).value(), child);
> +        LayoutUnit desiredLogicalHeight = child.constrainLogicalHeightByMinMax(stretchedLogicalHeight, LayoutUnit(-1));
> +        child.setOverrideLogicalContentHeight(desiredLogicalHeight - child.borderAndPaddingLogicalHeight());
> +        if (desiredLogicalHeight != child.logicalHeight()) {
> +            // FIXME: Can avoid laying out here in some cases. See https://webkit.org/b/87905.
> +            child.setLogicalHeight(LayoutUnit());
> +            child.setNeedsLayout();
>          }
>      }

Could be redone to use early exit instead of nesting the having he main function flow go into two nested if statements.

[Javier Fernandez]

Comment on attachment 284691 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:2190
>> +    bool stretchingAlongColumnAxis = childStyle.resolvedAlignSelf(gridStyle, selfAlignmentNormalBehavior).position() == ItemPositionStretch;
> 
> Inefficient to compute both axes and then only use the one that is needed below. Maybe these can be functions instead of booleans so we can call only the one that is needed.
> 
> Also inefficient to compute stretchingAlongAxis when allowedToStretchChildAlongAxis is already false. Early return style would work for that.

I understand, I was aiming for a clearer code but it's true we can improve in the points you made. 
Thanks for the review, I'll apply the suggested chances in the patch for landing.

>> Source/WebCore/rendering/RenderGrid.cpp:2203
>>      }
> 
> Could be redone to use early exit instead of nesting the having he main function flow go into two nested if statements.

Yeah, I'll do that.

[Javier Fernandez]

Created attachment 286611 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286611 [details]
Patch

Clearing flags on attachment: 286611

Committed r204734: <http://trac.webkit.org/changeset/204734>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.