[webkit-reviews] review granted: [Bug 58453] Absolute positioned elements in a relative positioned CSS3 Flexbox fail to display properly : [Attachment 144189] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 58453: Absolute positioned elements in a relative positioned CSS3 Flexbox
fail to display properly
https://bugs.webkit.org/show_bug.cgi?id=58453

Attachment 144189: Patch
https://bugs.webkit.org/attachment.cgi?id=144189&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144189&action=review


You might want to find Hyatt on IRC to give this a quick look.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:949
> +void
RenderFlexibleBox::prepareChildForPositionedLayoutHandlingRowReverse(RenderBox*
child, LayoutUnit mainAxisOffset, LayoutUnit crossAxisOffset)
> +{
> +    LayoutUnit inlinePosition = isColumnFlow() ? crossAxisOffset :
mainAxisOffset;
> +    if (style()->flexDirection() == FlowRowReverse)

Can we just pass an enum to prepareChildForPositionedLayout to flip for
row-reverse?  It could default to NoFlipForRowReverse.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:995
> +	       child->layoutIfNeeded();

Do you know why this is needed?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1174
> +	       // FIXME: Should this check be in adjustAlignmentForChild? It
doesn't seem to affect any testcases.
> +	       if (!shouldFlexAlignApplyToChild(child))
> +		   continue;

I suspect the static inline and static block position don't matter when
top/right/bottom/left are set. RenderBox::computeInlineStaticDistance bails if
logicalLeft or logicalRight are set and the same happens in
computeBlockStaticDistance.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1276
> -		   child->repaintDuringLayoutIfMoved(oldRect);
> +	       adjustAlignmentForChild(child, newOffset - originalOffset);

This is much better.

> LayoutTests/css3/flexbox/align-absolute-child.html:146
> +    <!-- FIXME: Should positioned flex items stretch to their container? -->


I don't think they should, but it would be nice to be clear about this in the
spec.