Created attachment 160020 [details] Patch

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.

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!

(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).

Created attachment 160237 [details] Patch (with changes from comment 2)

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.

Created attachment 160253 [details] Patch (with changes from comment 6)

Comment on attachment 160253 [details] Patch (with changes from comment 6) Thanks!

Thank *you*! :)

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 on attachment 160253 [details] Patch (with changes from comment 6) Trying again to see if applying the ChangeLog failing was just a fluke.

Comment on attachment 160253 [details] Patch (with changes from comment 6) Clearing flags on attachment: 160253 Committed r126503: <http://trac.webkit.org/changeset/126503>

All reviewed patches have been landed. Closing bug.