Bug 94237 – Fix cross-direction stretch for replaced elements in row flexbox

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

[Shezan Baig]

Created attachment 158875 [details]
test case

When the flexbox has "-webkit-flex-direction" set to column or row, iframe children do not take up the entire width/height of the parent flexbox.  See attached test case.  This works with divs.

The issue is: the inHorizontalBox, inVerticalBox and stretching flags in RenderBox::computeLogicalHeight and RenderBox::computeLogicalWidth only consider the deprecated flexbox.  It should also consider the non-deprecated flexbox.

[Shezan Baig]

Correction: the first sentence in comment 0 should read: "When the flexbox has -webkit-align-items: stretch, ..."

[Ojan Vafai]

This seems to work for me on Chrome 22.0.1229.2 (Official Build 150678) dev. What are you testing on? Can you attach a screenshot of what you're seeing?

[Shezan Baig]

Created attachment 158878 [details]
screen capture on canary and beta

Hmm I haven't tried on dev, but the attachment fails for me on both canary and beta.  See the attached screenshot.  On the left is "Version 23.0.1237.0 canary", on the right is "21.0.1180.79 beta-m".  I would expect the green iframe to take up the entire width.

[Ojan Vafai]

Ugh. I'm just an idiot. Misread the testcase. Yes, definitely a bug.

[Ojan Vafai]

Created attachment 158880 [details]
test-case with iframe and div

[Ojan Vafai]

(In reply to comment #0)
> The issue is: the inHorizontalBox, inVerticalBox and stretching flags in RenderBox::computeLogicalHeight and RenderBox::computeLogicalWidth only consider the deprecated flexbox.  It should also consider the non-deprecated flexbox.

That shouldn't matter since we set the overrideLogicalContentHeight/Width, which makes us not hit the replaced codepath in these two functions. It's not obvious to me at first glance where the bug is.

[Shezan Baig]

We actually don't set the override width/height, because:

for height:
RenderFlexibleBox::applyStretchAlignmentToChild skips that logic because computeLogicalHeight() always returns the intrinsic height of the iframe, which is 150px

for width:
RenderFlexibleBox::applyStretchAlignmentToChild skips that logic because isMultiline returns false

[Tony Chang]

It sounds like we don't stretch in the cross direction because iframes have an intrinsic size.  I think this is also why images and some form elements don't stretch in the cross direction (some of the failing test cases in LayoutTests/css3/flexbox/flexitem.html cover this case).

[Shezan Baig]

Created attachment 159147 [details]
Patch

There were two ways to fix this:

1) Make RenderBox::computeLogicalWidth and computeLogicalHeight take RenderFlexibleBox parents into consideration (in similar way that it takes the deprecated flexBox into consideration), or

2) Make RenderFlexibleBox always set the override width/height on the child if the child computes its size as a replaced element.

I went with option (2) in order to avoid the dependency from RenderBox to RenderFlexibleBox.  I also extended the test case to include vertically flowing flexboxes.

[Ojan Vafai]

Comment on attachment 159147 [details]
Patch

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

Thanks for the patch! At a high-level this looks good to me. Just needs a bit more work.

shouldComputeSizeAsReplaced is overridden by RenderIframe. Can you add a test to make sure seamless iframes work? Can you also add a test to make sure that stretch will shrink the iframe if it's too large (e.g. make the flexbox's width/height 100px and the iframe should shrink appropriately).

> Source/WebCore/ChangeLog:18
> +        (WebCore::RenderBox::shouldComputeSizeAsReplaced): Make this public so
> +        that it can be used from RenderFlexibleBox.

RenderFlexibleBox inherits from RenderBox, so leaving it protected should be sufficient.

> Source/WebCore/ChangeLog:23
> +        (WebCore::RenderFlexibleBox::applyStretchAlignmentToChild): Set the
> +        override width and height if the child computes its size as a replaced
> +        element.

I like this! As a followup patch, maybe we could cleanup RenderDeprecatedFlexibleBox the same way so that RenderBox doesn't need to know as much about deprecated flexboxes.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1255
>          LayoutUnit childWidth = lineCrossAxisExtent - crossAxisMarginExtentForChild(child);
>          child->setOverrideLogicalContentWidth(std::max(ZERO_LAYOUT_UNIT, childWidth));

While I'm looking at this code, it occurs to me that we only need to layout if the width changes. Mind adding a FIXME while you're here? Also, if we did that, I'm not sure we need the isMultiline check. I think that's just a performance optimization to avoid layouts. Tony, WDYT?

> LayoutTests/css3/flexbox/flexitem.html:19
> +.flexboxVert {

How about s/flexboxVert/column? That's what we do in other tests as we don't want to confuse it with vertical writing-mode.

> LayoutTests/css3/flexbox/flexitem.html:21
> +    height: 600px;

For easily test management, can you make this height 200px? It's easier when you don't have to scroll as much to get to each testcase.

> LayoutTests/css3/flexbox/flexitem.html:25
> +    min-width: 10px;

Doesn't seem like you test this case, so no point in setting the CSS.

> LayoutTests/css3/flexbox/flexitem.html:112
> +    <div data-expected-display="block" style="-webkit-flex: 1 0 auto; height:400px;"></div>

This div doesn't seem to add value as it's just testing the display, which is covered by other tests.

> LayoutTests/css3/flexbox/flexitem.html:113
> +    <iframe data-expected-display="block" data-expected-height="400" style="-webkit-flex: 1 0 auto;" src="data:text/html,<body bgcolor=#fff>iframe</body>"></iframe>

If you leave out the src, I think you can just set a background-color on the iframe element itself and get the same effect.

> LayoutTests/css3/flexbox/flexitem.html:116
> +<div class="flexboxVert">

Can you make the class="flexbox column"? Then you don't need to add the extra checkLayout call and you can remove the redundant CSS styling above.

> LayoutTests/css3/flexbox/flexitem.html:129
> +<div class="flexboxVert">
> +    <iframe data-expected-display="block" data-expected-width="600" style="-webkit-flex: 1 0 auto;" src="data:text/html,<body bgcolor=#fff>iframe</body>"></iframe>
> +</div>

Can you add data-expected-width to the above case? Then this would be redundant and you could delete it. Also, that way, we'd make sure stretch works on the other replaced element types as well.

[Shezan Baig]

Comment on attachment 159147 [details]
Patch

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

Thanks for the review -- will upload a new patch shortly.  Just waiting for Tony's comments w.r.t. the isMultiline change.

>> Source/WebCore/ChangeLog:18
>> +        that it can be used from RenderFlexibleBox.
> 
> RenderFlexibleBox inherits from RenderBox, so leaving it protected should be sufficient.

Actually, it doesn't work unless it is public.  When it is protected, derived classes can only access it through the 'this' pointer.  In our case, we need to access it through the 'child' pointer in applyStretchAlignmentToChild.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1255
>>          child->setOverrideLogicalContentWidth(std::max(ZERO_LAYOUT_UNIT, childWidth));
> 
> While I'm looking at this code, it occurs to me that we only need to layout if the width changes. Mind adding a FIXME while you're here? Also, if we did that, I'm not sure we need the isMultiline check. I think that's just a performance optimization to avoid layouts. Tony, WDYT?

Good point, I'll add the FIXME.  I'll wait for Tony's comment to decide whether to remove the isMultiline check, or should that be in a separate bug/patch?

>> LayoutTests/css3/flexbox/flexitem.html:19
>> +.flexboxVert {
> 
> How about s/flexboxVert/column? That's what we do in other tests as we don't want to confuse it with vertical writing-mode.

Will do.

>> LayoutTests/css3/flexbox/flexitem.html:21
>> +    height: 600px;
> 
> For easily test management, can you make this height 200px? It's easier when you don't have to scroll as much to get to each testcase.

Will do.

>> LayoutTests/css3/flexbox/flexitem.html:25
>> +    min-width: 10px;
> 
> Doesn't seem like you test this case, so no point in setting the CSS.

Will do.

>> LayoutTests/css3/flexbox/flexitem.html:112
>> +    <div data-expected-display="block" style="-webkit-flex: 1 0 auto; height:400px;"></div>
> 
> This div doesn't seem to add value as it's just testing the display, which is covered by other tests.

The purpose of the div was to make the height of the flexbox 400px (it needs to be more than the intrinsic height of the iframe for this test to be meaningful).  But maybe I can just set "height:400px" on th flexbox and get rid of the inner div.

>> LayoutTests/css3/flexbox/flexitem.html:113
>> +    <iframe data-expected-display="block" data-expected-height="400" style="-webkit-flex: 1 0 auto;" src="data:text/html,<body bgcolor=#fff>iframe</body>"></iframe>
> 
> If you leave out the src, I think you can just set a background-color on the iframe element itself and get the same effect.

Will do.  Should I also do this to the other iframe which already exists in this test?

>> LayoutTests/css3/flexbox/flexitem.html:116
>> +<div class="flexboxVert">
> 
> Can you make the class="flexbox column"? Then you don't need to add the extra checkLayout call and you can remove the redundant CSS styling above.

Will do.

>> LayoutTests/css3/flexbox/flexitem.html:129
>> +</div>
> 
> Can you add data-expected-width to the above case? Then this would be redundant and you could delete it. Also, that way, we'd make sure stretch works on the other replaced element types as well.

Nice one -- will do.

[Shezan Baig]

(In reply to comment #10)
> (From update of attachment 159147 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159147&action=review
> 
> Thanks for the patch! At a high-level this looks good to me. Just needs a bit more work.
> 
> shouldComputeSizeAsReplaced is overridden by RenderIframe. Can you add a test to make sure seamless iframes work? Can you also add a test to make sure that stretch will shrink the iframe if it's too large (e.g. make the flexbox's width/height 100px and the iframe should shrink appropriately).
>


Are seamless iframes enabled everywhere?  iirc, it is only enabled in chromium.  Would adding a test for it cause problems for other ports?

[Ojan Vafai]

Comment on attachment 159147 [details]
Patch

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

>>> Source/WebCore/ChangeLog:18
>>> +        that it can be used from RenderFlexibleBox.
>> 
>> RenderFlexibleBox inherits from RenderBox, so leaving it protected should be sufficient.
> 
> Actually, it doesn't work unless it is public.  When it is protected, derived classes can only access it through the 'this' pointer.  In our case, we need to access it through the 'child' pointer in applyStretchAlignmentToChild.

Ah, duh. Good point.

>>> LayoutTests/css3/flexbox/flexitem.html:112
>>> +    <div data-expected-display="block" style="-webkit-flex: 1 0 auto; height:400px;"></div>
>> 
>> This div doesn't seem to add value as it's just testing the display, which is covered by other tests.
> 
> The purpose of the div was to make the height of the flexbox 400px (it needs to be more than the intrinsic height of the iframe for this test to be meaningful).  But maybe I can just set "height:400px" on th flexbox and get rid of the inner div.

Yeah, I think that'd be better. It'd be more clear what's going on.

>>> LayoutTests/css3/flexbox/flexitem.html:113
>>> +    <iframe data-expected-display="block" data-expected-height="400" style="-webkit-flex: 1 0 auto;" src="data:text/html,<body bgcolor=#fff>iframe</body>"></iframe>
>> 
>> If you leave out the src, I think you can just set a background-color on the iframe element itself and get the same effect.
> 
> Will do.  Should I also do this to the other iframe which already exists in this test?

Sure

[Ojan Vafai]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 159147 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=159147&action=review
> > 
> > Thanks for the patch! At a high-level this looks good to me. Just needs a bit more work.
> > 
> > shouldComputeSizeAsReplaced is overridden by RenderIframe. Can you add a test to make sure seamless iframes work? Can you also add a test to make sure that stretch will shrink the iframe if it's too large (e.g. make the flexbox's width/height 100px and the iframe should shrink appropriately).
> >
> 
> 
> Are seamless iframes enabled everywhere?  iirc, it is only enabled in chromium.  Would adding a test for it cause problems for other ports?

If we need to we can skip the test for any ports that disable it. At first glance, it doesn't seem disabled. Or, at least, the existing seamless tests are not skipped for other ports (e.g. I checked platform/mac/Skipped and platform/mac/TestExpectations).

> >> Source/WebCore/rendering/RenderFlexibleBox.cpp:1255
> >>          child->setOverrideLogicalContentWidth(std::max(ZERO_LAYOUT_UNIT, childWidth));
> > 
> > While I'm looking at this code, it occurs to me that we only need to layout if the width changes. Mind adding a FIXME while you're here? Also, if we did that, I'm not sure we need the isMultiline check. I think that's just a performance optimization to avoid layouts. Tony, WDYT?
> 
> Good point, I'll add the FIXME.  I'll wait for Tony's comment to decide whether to remove the isMultiline check, or should that be in a separate bug/patch?

Just add a FIXME for both right now. I wouldn't want to remove the isMultiline check without first only laying out if the width changes.

[Shezan Baig]

(In reply to comment #13)
> >>> LayoutTests/css3/flexbox/flexitem.html:113
> >>> +    <iframe data-expected-display="block" data-expected-height="400" style="-webkit-flex: 1 0 auto;" src="data:text/html,<body bgcolor=#fff>iframe</body>"></iframe>
> >> 
> >> If you leave out the src, I think you can just set a background-color on the iframe element itself and get the same effect.
> > 
> > Will do.  Should I also do this to the other iframe which already exists in this test?
> 
> Sure


Just realized that when I do this, and run the test in a browser, we no longer see the text "iframe" in the space allocated for the iframe.  I feel that keeping this body text adds some clarity to the test when viewed in the browser.  WDYT?

[Shezan Baig]

Created attachment 159443 [details]
Patch

New patch.  The updated test case has identified new bugs, namely that buttons, selects and textareas don't stretch in the cross direction.  This can be addressed in future patches.

[Ojan Vafai]

Comment on attachment 159443 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1240
> +            child->setOverrideLogicalContentHeight(stretchedLogicalHeight - child->borderAndPaddingLogicalHeight());

As I look more closely, I think this will do the wrong thing in some cases. At the least, when the item has a fixed logical height (e.g. height:100px...it shouldn't stretch) or a min/max logical height (it should respect min/max constraints).

Sorry I didn't catch this earlier.

I think the right solution might be to move the following code out of RenderBox::computeLogicalHeight into a helper function that RenderFlexibleBox can call:
            heightResult = computeLogicalHeightUsing(MainOrPreferredSize, styleToUse->logicalHeight());
            if (heightResult == -1)
                heightResult = logicalHeight();
            LayoutUnit minH = computeLogicalHeightUsing(MinSize, styleToUse->logicalMinHeight()); // Leave as -1 if unset.
            LayoutUnit maxH = styleToUse->logicalMaxHeight().isUndefined() ? heightResult : computeLogicalHeightUsing(MaxSize, styleToUse->logicalMaxHeight());
            if (maxH == -1)
                maxH = heightResult;
            heightResult = min(maxH, heightResult);
            heightResult = max(minH, heightResult);

The helper function (computeLogicalHeightCheckingMinMax?) would take the "logicalHeight()" as an argument and return the heightResult. Then here in RenderFlexibleBox, we'd call:
LayoutUnit logicalHeightAfter = computeLogicalHeightCheckingMinMax(stretchedLogicalHeight);

Then we don't need the computeLogicalHeight call below and we can do the same thing for replace and non-replaced items.

As a followup, we could also remove the essentially duplicate code in RenderFlexibleBox::computeAvailableFreeSpace by having it call computeLogicalHeightCheckingMinMax in the else clause and then adjusting appropriately for box-sizing.

Tony, WDYT of this solution?

[Shezan Baig]

(In reply to comment #17)
> I think the right solution might be to move the following code out of RenderBox::computeLogicalHeight into a helper function that RenderFlexibleBox can call:
>             heightResult = computeLogicalHeightUsing(MainOrPreferredSize, styleToUse->logicalHeight());
>             if (heightResult == -1)
>                 heightResult = logicalHeight();
>             LayoutUnit minH = computeLogicalHeightUsing(MinSize, styleToUse->logicalMinHeight()); // Leave as -1 if unset.
>             LayoutUnit maxH = styleToUse->logicalMaxHeight().isUndefined() ? heightResult : computeLogicalHeightUsing(MaxSize, styleToUse->logicalMaxHeight());
>             if (maxH == -1)
>                 maxH = heightResult;
>             heightResult = min(maxH, heightResult);
>             heightResult = max(minH, heightResult);
> 


Thanks, I'll experiment with this.  I'm assuming for column flexboxes, we'll also need something similar for logical width? i.e. move this part from computeLogicalWidthInRegion into a helper function:

        // Calculate LogicalWidth
        setLogicalWidth(computeLogicalWidthInRegionUsing(LogicalWidth, containerWidthInInlineDirection, cb, region, offsetFromLogicalTopOfFirstPage));

        // Calculate MaxLogicalWidth
        if (!styleToUse->logicalMaxWidth().isUndefined()) {
            LayoutUnit maxLogicalWidth = computeLogicalWidthInRegionUsing(MaxLogicalWidth, containerWidthInInlineDirection, cb, region, offsetFromLogicalTopOfFirstPage);
            if (logicalWidth() > maxLogicalWidth)
                setLogicalWidth(maxLogicalWidth);
        }

        // Calculate MinLogicalWidth
        LayoutUnit minLogicalWidth = computeLogicalWidthInRegionUsing(MinLogicalWidth, containerWidthInInlineDirection, cb, region, offsetFromLogicalTopOfFirstPage);
        if (logicalWidth() < minLogicalWidth)
            setLogicalWidth(minLogicalWidth);


and call that helper function for the column flow case?


> The helper function (computeLogicalHeightCheckingMinMax?) would take the "logicalHeight()" as an argument and return the heightResult. Then here in RenderFlexibleBox, we'd call:


How about naming it "constrainLogicalHeightByMinMax"?  I think this will be a better name because "computeLogicalHeightCheckingMinMax" sounds like we are going to take everything to do with logical height into account (e.g. intrinsic height, override height etc) and checking min/max in addition to all that; but "constrainLogicalHeightByMinMax" makes it clear that we are just limiting the logical height by the min/max constraints.



> LayoutUnit logicalHeightAfter = computeLogicalHeightCheckingMinMax(stretchedLogicalHeight);
> 
> Then we don't need the computeLogicalHeight call below and we can do the same thing for replace and non-replaced items.
> 
> As a followup, we could also remove the essentially duplicate code in RenderFlexibleBox::computeAvailableFreeSpace by having it call computeLogicalHeightCheckingMinMax in the else clause and then adjusting appropriately for box-sizing.
> 
> Tony, WDYT of this solution?

[Tony Chang]

Comment on attachment 159443 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1240
>> +            child->setOverrideLogicalContentHeight(stretchedLogicalHeight - child->borderAndPaddingLogicalHeight());
> 
> As I look more closely, I think this will do the wrong thing in some cases. At the least, when the item has a fixed logical height (e.g. height:100px...it shouldn't stretch) or a min/max logical height (it should respect min/max constraints).
> 
> Sorry I didn't catch this earlier.
> 
> I think the right solution might be to move the following code out of RenderBox::computeLogicalHeight into a helper function that RenderFlexibleBox can call:
>             heightResult = computeLogicalHeightUsing(MainOrPreferredSize, styleToUse->logicalHeight());
>             if (heightResult == -1)
>                 heightResult = logicalHeight();
>             LayoutUnit minH = computeLogicalHeightUsing(MinSize, styleToUse->logicalMinHeight()); // Leave as -1 if unset.
>             LayoutUnit maxH = styleToUse->logicalMaxHeight().isUndefined() ? heightResult : computeLogicalHeightUsing(MaxSize, styleToUse->logicalMaxHeight());
>             if (maxH == -1)
>                 maxH = heightResult;
>             heightResult = min(maxH, heightResult);
>             heightResult = max(minH, heightResult);
> 
> The helper function (computeLogicalHeightCheckingMinMax?) would take the "logicalHeight()" as an argument and return the heightResult. Then here in RenderFlexibleBox, we'd call:
> LayoutUnit logicalHeightAfter = computeLogicalHeightCheckingMinMax(stretchedLogicalHeight);
> 
> Then we don't need the computeLogicalHeight call below and we can do the same thing for replace and non-replaced items.
> 
> As a followup, we could also remove the essentially duplicate code in RenderFlexibleBox::computeAvailableFreeSpace by having it call computeLogicalHeightCheckingMinMax in the else clause and then adjusting appropriately for box-sizing.
> 
> Tony, WDYT of this solution?

Ojan's suggestion sounds reasonable.

Yes, you need to keep the isMultiline() check for now (it's a performance optimization), but we can probably remove it if we check to see if the width has changed.

[Ojan Vafai]

> Thanks, I'll experiment with this.  I'm assuming for column flexboxes, we'll also need something similar for logical width? i.e. move this part from computeLogicalWidthInRegion into a helper function:
...
> and call that helper function for the column flow case?

Oh, interesting, we likely have a bug here where we don't respect min/max logical width when stretching with column flow. Mind adding tests for that as well? If that's true, then, yes, I think it makes sense to do something similar for logical width and the code in the column clause of applyStretchAlignmentToChild will need to be more like the row flow case (i.e. check that the width isn't changing and only setOverride* if it is changing).

Sorry this is ballooning into a larger patch.

> > The helper function (computeLogicalHeightCheckingMinMax?) would take the "logicalHeight()" as an argument and return the heightResult. Then here in RenderFlexibleBox, we'd call:
> 
> 
> How about naming it "constrainLogicalHeightByMinMax"?  I think this will be a better name because "computeLogicalHeightCheckingMinMax" sounds like we are going to take everything to do with logical height into account (e.g. intrinsic height, override height etc) and checking min/max in addition to all that; but "constrainLogicalHeightByMinMax" makes it clear that we are just limiting the logical height by the min/max constraints.

Yeah, the only thing confusing about that is it's constraining by min, max and actual. The passed in value is only used if the actual logical height is not definite (e.g. it's auto). I can't think of a better name though. How about...logicalHeightConstrainedByMinMax?

None of these names are great, but I can't think of anything better.

[Shezan Baig]

(In reply to comment #20)
> Sorry this is ballooning into a larger patch.


I'm lovin' it :)  This is all giving me a better idea of how the layout works and all the different components :)


> How about...logicalHeightConstrainedByMinMax?

That sounds good -- I'll go with that for now.  Thanks!

[Tony Chang]

(In reply to comment #20)
> > Thanks, I'll experiment with this.  I'm assuming for column flexboxes, we'll also need something similar for logical width? i.e. move this part from computeLogicalWidthInRegion into a helper function:
> ...
> > and call that helper function for the column flow case?
> 
> Oh, interesting, we likely have a bug here where we don't respect min/max logical width when stretching with column flow. Mind adding tests for that as well? If that's true, then, yes, I think it makes sense to do something similar for logical width and the code in the column clause of applyStretchAlignmentToChild will need to be more like the row flow case (i.e. check that the width isn't changing and only setOverride* if it is changing).

There's already a FIXME for handling min-width/max-width when stretching columns.

It would probably be easiest to fix rows and columns as 2 separate patches since the code is different in both cases.

[Tony Chang]

(But if you want to fix as a single patch, that's fine too.)

[Ojan Vafai]

(In reply to comment #22)
> It would probably be easiest to fix rows and columns as 2 separate patches since the code is different in both cases.

As Tony said, up to you, but smaller patches are usually better if you're willing. It's easier to verify that the tests cover all the cases, etc.

[Shezan Baig]

Agreed that the code is very different and should be done in two patches.  Will fix stretching of replaced elements in row flexboxes under this bug.  I have entered bug 94604 to handle the same issue in column flexboxes.

[Shezan Baig]

Created attachment 159707 [details]
Patch

New patch with new logicalHeightConstrainedByMinMax() helper method. I would have liked to make this a "const" method, but it is using some methods that are currently non-const, and when trying to convert those, I ended up going down a wormhole of const-correcting.  Perhaps this can be addressed in a future patch.

[Ojan Vafai]

Comment on attachment 159707 [details]
Patch

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

Looks great! Please address my last couple comments then feel free to commit.

> Source/WebCore/rendering/RenderBox.cpp:2018
> +            heightResult = logicalHeightConstrainedByMinMax(logicalHeight());
>          } else {

Nit: Single line if's shouldn't have curly braces, even if there is an else clause.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1241
> +        if (logicalHeightAfter != logicalHeightBefore) {

Nit: lets get rid of logicalHeightBefore and just call child->logicalHeight() here. Before this patch, we needed to save it out first because computeLogicalHeight actually modifies it.

> LayoutTests/css3/flexbox/flexitem.html:111
> +<div class="flexbox column" style="height:210px">

Can we add this last test-case for the row flows as well? Just to make sure all these cases work (and if not, having test coverage showing which cases don't).

[WebKit Review Bot]

Comment on attachment 159707 [details]
Patch

Attachment 159707 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13551404

New failing tests:
css3/flexbox/writing-modes.html
css3/flexbox/flex-item-min-size.html

[WebKit Review Bot]

Created attachment 159742 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Shezan Baig]

Sorry the landing patch is taking some time.  I am investigating these test failures, not quite sure yet why this change would cause these failures.

[Shezan Baig]

Created attachment 159771 [details]
Patch

I think this patch requires another review.  I had to disable stretching when the child and the flexbox had orthogonal flows, in order to make the tests above pass.  The issue here is that the child already gets overrideHeight set during the flex layout, so we can't use overrideHeight for stretching.  I'm not sure what to do here (do we use overrideWidth instead in this case?).  I've added a FIXME for this, so hopefully it can be addressed in a future patch.

[Shezan Baig]

(In reply to comment #27)
> (From update of attachment 159707 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159707&action=review
> 
> Looks great! Please address my last couple comments then feel free to commit.
> 
> > Source/WebCore/rendering/RenderBox.cpp:2018
> > +            heightResult = logicalHeightConstrainedByMinMax(logicalHeight());
> >          } else {
> 
> Nit: Single line if's shouldn't have curly braces, even if there is an else clause.
> 


Done.


> > Source/WebCore/rendering/RenderFlexibleBox.cpp:1241
> > +        if (logicalHeightAfter != logicalHeightBefore) {
> 
> Nit: lets get rid of logicalHeightBefore and just call child->logicalHeight() here. Before this patch, we needed to save it out first because computeLogicalHeight actually modifies it.
> 


Done.  I also renamed logicalHeightAfter to desiredLogicalHeight, since there's no "before", it seemed to me like there shouldn't be an "after" :)


> > LayoutTests/css3/flexbox/flexitem.html:111
> > +<div class="flexbox column" style="height:210px">
> 
> Can we add this last test-case for the row flows as well? Just to make sure all these cases work (and if not, having test coverage showing which cases don't).


Done.  I added this to the first test-case (from which the last test-case was originally duped from).

[Ojan Vafai]

Comment on attachment 159771 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1237
> +        if (!hasOrthogonalFlow(child)) {

Good catch! I think we just need to use the overrideHeight if it is set and use logicalHeight otherwise.

Similarly, when we make the column flow case first check if the width is changing, if hasOrthogonalFlow(child) is false, we'll need to grab the override width if it exists.

In either case, both of these can be followup patches.

[WebKit Review Bot]

Comment on attachment 159771 [details]
Patch

Clearing flags on attachment: 159771

Committed r126257: <http://trac.webkit.org/changeset/126257>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

This change caused a regression of css3/flexbox/flexitem.html on mac.  See:
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r126299%20(2115)/css3/flexbox/flexitem-pretty-diff.html.

Please fix.  Thanks.

[Ojan Vafai]

FYI, it's possible this patch caused a performance regression: http://code.google.com/p/chromium/issues/detail?id=144251. Not clear yet it's this one for sure.