Bug 94604

Summary: Fix cross-direction stretch for replaced elements in column flexbox
Product: WebKit Reporter: Shezan Baig <shezbaig.wk>
Component: Layout and RenderingAssignee: Shezan Baig <shezbaig.wk>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch (with changes from comment 2)
none
Patch (with changes from comment 6) none

Description Shezan Baig 2012-08-21 08:12:11 PDT
Cross direction stretch should ignore intrinsic width of replaced elements, but take into account fixed size, min-size and max-size.  See bug 94237 for more discussion about this.
Comment 1 Shezan Baig 2012-08-22 15:26:02 PDT
Created attachment 160020 [details]
Patch
Comment 2 Ojan Vafai 2012-08-22 19:16:39 PDT
Comment on attachment 160020 [details]
Patch

Can you add test cases that set min-width/max-width to a pixel value, a percent value, fit-content, min-content, max-content and fill-available? See http://dev.w3.org/csswg/css3-sizing/ if you're unfamilar with the last four of those (or just see the test-cases we have for those).

Also, sigh...as I look at this more closely, I think maybe our last patch was not the best. Instead of logicalHeightConstrainedByMinMax, we should have added constrainLogicalHeightByMinMax, which just takes a height value and constrains it:
    LayoutUnit minH = computeLogicalHeightUsing(MinSize, styleToUse->logicalMinHeight()); // Leave as -1 if unset.
    LayoutUnit maxH = styleToUse->logicalMaxHeight().isUndefined() ? result : computeLogicalHeightUsing(MaxSize, styleToUse->logicalMaxHeight());
    if (maxH == -1)
        maxH = result;
    result = min(maxH, result);
    result = max(minH, result);
    return result;

Since, in this case, we know the height is auto, we don't need to do the first few lines that are currently at the beginning of logicalHeightConstrainedByMinMax. Also, we'll need this same stretching logic for RenderGrid only if the height/width is auto. So, I'm thinking the above change makes sense, WDYT?

Then we could get the logicalWidth case to do something similar so we can share more code with RenderBox:
1. Add a constrainLogicalWidthByMinMax(availableWidth) that does the equivalent of constrainLogicalHeightByMinMax.
2. Restructure RenderBox::computeLogicalWidthInRegion to use constrainLogicalHeightByMinMax.
3. Use constrainLogicalHeightByMinMax here in RenderFlexibleBox.

Then you don't need computeStretchedLogicalWidthUsing and you'll handle the above test-cases I suggested correctly.
Comment 3 Shezan Baig 2012-08-23 07:23:26 PDT
Thanks for the review.

(In reply to comment #2)
> (From update of attachment 160020 [details])
> Can you add test cases that set min-width/max-width to a pixel value, a percent value, fit-content, min-content, max-content and fill-available? See http://dev.w3.org/csswg/css3-sizing/ if you're unfamilar with the last four of those (or just see the test-cases we have for those).
> 


Will do.



> Also, sigh...as I look at this more closely, I think maybe our last patch was not the best. Instead of logicalHeightConstrainedByMinMax, we should have added constrainLogicalHeightByMinMax, which just takes a height value and constrains it:
>     LayoutUnit minH = computeLogicalHeightUsing(MinSize, styleToUse->logicalMinHeight()); // Leave as -1 if unset.
>     LayoutUnit maxH = styleToUse->logicalMaxHeight().isUndefined() ? result : computeLogicalHeightUsing(MaxSize, styleToUse->logicalMaxHeight());
>     if (maxH == -1)
>         maxH = result;
>     result = min(maxH, result);
>     result = max(minH, result);
>     return result;
> 
> Since, in this case, we know the height is auto, we don't need to do the first few lines that are currently at the beginning of logicalHeightConstrainedByMinMax. Also, we'll need this same stretching logic for RenderGrid only if the height/width is auto. So, I'm thinking the above change makes sense, WDYT?
> 


Agreed, I've entered bug 94807 to do this as a separate patch since this will be a relatively simple change.


> Then we could get the logicalWidth case to do something similar so we can share more code with RenderBox:
> 1. Add a constrainLogicalWidthByMinMax(availableWidth) that does the equivalent of constrainLogicalHeightByMinMax.
> 2. Restructure RenderBox::computeLogicalWidthInRegion to use constrainLogicalHeightByMinMax.
> 3. Use constrainLogicalHeightByMinMax here in RenderFlexibleBox.
> 
> Then you don't need computeStretchedLogicalWidthUsing and you'll handle the above test-cases I suggested correctly.


Will do.  It might be a little more complicated than that though, because computeLogicalWidthInRegion call computeLogicalWidthInRegionUsing when constraining the min/max width.  These functions take "region" and "offsetFromLogicalTopOfFirstPage" arguments, which I'm assuming we would need to compute in RenderFlexibleBox?  I'll experiment with this today.

Thanks!
Comment 4 Ojan Vafai 2012-08-23 11:15:52 PDT
(In reply to comment #3)
> Will do.  It might be a little more complicated than that though, because computeLogicalWidthInRegion call computeLogicalWidthInRegionUsing when constraining the min/max width.  These functions take "region" and "offsetFromLogicalTopOfFirstPage" arguments, which I'm assuming we would need to compute in RenderFlexibleBox?  I'll experiment with this today.

Oh right. I knew I was forgetting something. Using computeLogicalWidthInRegionUsing is correct I think. You can just pass 0 for both of those arguments for now. It's not clear how flexbox and regions are supposed to interact exactly anyways. That's what RenderBox::computeLogicalWidth does. It calls computeLogicalWidthInRegion without any arguments (see the default arguments for computeLogicalWidthInRegion in RenderBox.h).
Comment 5 Shezan Baig 2012-08-23 14:05:44 PDT
Created attachment 160237 [details]
Patch (with changes from comment 2)
Comment 6 Ojan Vafai 2012-08-23 14:36:19 PDT
Comment on attachment 160237 [details]
Patch (with changes from comment 2)

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

Looks great. Just a few nits.

> Source/WebCore/rendering/RenderBox.cpp:440
> +        // Constrain by MaxSize.

I know the old code had these comments, but they're just stating what the code obviously does. Mind deleting these while you're in here?

> Source/WebCore/rendering/RenderBox.cpp:443
> +    // Constrain by MinSize.

ditto

> Source/WebCore/rendering/RenderBox.cpp:1702
> +        // Calculate LogicalWidth constrained by MaxLogicalWidth and MinLogicalWidth

ditto

> LayoutTests/css3/flexbox/flexitem.html:130
> +    <img data-expected-display="block" data-expected-height="60" style="min-height:60px" src="../images/resources/blue-100.png">

I think we have enough coverage of the expected-display. Mind remove this from these new tests? It adds clutter that makes it harder to see what the test actually cares about.

> LayoutTests/platform/chromium/TestExpectations:-3513
> -BUGWK94675 : css3/flexbox/flexitem.html = TEXT

TL;DR: I think we still need this line for now.

I think there's still the issue with the image cases that are flakily giving the wrong results, so I think we need to leave this in until we figure out what's going wrong there. I was hoping that once we got this checked in it would filter out the cases that were due to the column-flow issue so we can see where the flaky image failures happen across platforms and try to reproduce it locally.
Comment 7 Shezan Baig 2012-08-23 14:46:24 PDT
Created attachment 160253 [details]
Patch (with changes from comment 6)
Comment 8 Ojan Vafai 2012-08-23 14:47:05 PDT
Comment on attachment 160253 [details]
Patch (with changes from comment 6)

Thanks!
Comment 9 Shezan Baig 2012-08-23 14:49:04 PDT
Thank *you*! :)
Comment 10 WebKit Review Bot 2012-08-23 15:50:51 PDT
Comment on attachment 160253 [details]
Patch (with changes from comment 6)

Rejecting attachment 160253 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
content): Merge conflict in LayoutTests/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Trailing spaces in CSP source lists should not generate console warnings.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13564862
Comment 11 Ojan Vafai 2012-08-23 16:15:42 PDT
Comment on attachment 160253 [details]
Patch (with changes from comment 6)

Trying again to see if applying the ChangeLog failing was just a fluke.
Comment 12 WebKit Review Bot 2012-08-23 16:40:47 PDT
Comment on attachment 160253 [details]
Patch (with changes from comment 6)

Clearing flags on attachment: 160253

Committed r126503: <http://trac.webkit.org/changeset/126503>
Comment 13 WebKit Review Bot 2012-08-23 16:40:51 PDT
All reviewed patches have been landed.  Closing bug.