|
Description
Tony Chang
2012-03-21 11:35:19 PDT
Hi Tony, can you please elaborate?. Are you speaking about RenderFlexibleBox? (In reply to comment #1) > Hi Tony, can you please elaborate?. > Are you speaking about RenderFlexibleBox? Yes, this is in RenderFlexibleBox.cpp. Demo in the URL. This may actually just work (the above test case with min-height seems to). We should add tests for this. Added a few more cases to test: http://plexode.com/u/#R!*!style%3DAzframe2q%3Eb_%3C%2FdivqfZ0pxHSK-8blueOrYXR!columnTK-8grerXP)NHVL-directionrWNTmargin-bottomRrVSdisplayRLbox%3B6T%3B!ES!!(%272R%3AQPJow93)!!)bO9)G8red92)G*bN%0A(%27*50pxfL!-webkit-flexrKIlid!orangeHGminrJ0pxTmax-8yellrIborderR!3px!soQH%27q%0AMG!!SU9%27qtestr84round-colorR!r4*100%25TbackgU*heightR!n)_%0An(%3Cdiv!*2!%20~http://plexode.com/eval3/#ht=%3Ciz!*%22heightR350px%22!src%3D%22dataRtext%2Fhtml%2CW!11Z10YenO10P)%22q%3C%2Fizq%0AW!6Z6YenO6P_&jt=zs%5B0%5D.document.compatMode I agree that this is working correctly, but looking at the code I don't understand why. :( Makes me a little worried that there may be cases where it doesn't work. Ugh. All of these are invalid due to renaming flexbox to flex. Uploading a valid test case. Still, we seem to get the height right. Created attachment 156758 [details]
testcase
(In reply to comment #5) > Ugh. All of these are invalid due to renaming flexbox to flex. Uploading a valid test case. Still, we seem to get the height right. Yeah, I think we just need to add some layout tests. (In reply to comment #7) > (In reply to comment #5) > > Ugh. All of these are invalid due to renaming flexbox to flex. Uploading a valid test case. Still, we seem to get the height right. > > Yeah, I think we just need to add some layout tests. I've found at least one bug as I've been trying to understand this. I'll post an update once I feel that I've found all the relevant bugs. I've found a few bugs and will post a patch soon. Having some weird issues with my fix. :( Created attachment 157004 [details]
Patch
Comment on attachment 157004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157004&action=review > Source/WebCore/rendering/RenderBox.cpp:2064 > +LayoutUnit RenderBox::computeLogicalClientHeight(SizeType heightType, const Length& height) Tony originally said this didn't make sense here because it's not useful for things outside new Flexbox. What's different now? > Source/WebCore/rendering/RenderFlexibleBox.cpp:817 > + return minSize; The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug? Comment on attachment 157004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157004&action=review >> Source/WebCore/rendering/RenderBox.cpp:2064 >> +LayoutUnit RenderBox::computeLogicalClientHeight(SizeType heightType, const Length& height) > > Tony originally said this didn't make sense here because it's not useful for things outside new Flexbox. What's different now? I'm thinking ahead to Grid, which will need this same logic for restricting to min/max sizes. The alternative for this patch would have been to pass in the RenderBox to use, which is fine (we'd pass in "this" everwhere except in the three cases where we'd pass in the flex item). But given the Grid need, I went with the simpler patch. I'm a little wary of all these methods with very similar sounding names, but I couldn't think of a great solution. >> Source/WebCore/rendering/RenderFlexibleBox.cpp:817 >> + return minSize; > > The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug? computePercentageLogicalHeight can return -1 in some cases depending on the state of the containingBlock (e.g. in standards mode if the containing block is height:auto). Comment on attachment 157004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157004&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:407 > + if (isColumnFlow()) > + return child->computeLogicalClientHeight(sizeType, size); > + return child->computeContentBoxLogicalWidth(valueForLength(size, availableSize, view())); This looks wrong for orthogonal flows, but I guess it's what we had before. >>> Source/WebCore/rendering/RenderFlexibleBox.cpp:817 >>> + return minSize; >> >> The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug? > > computePercentageLogicalHeight can return -1 in some cases depending on the state of the containingBlock (e.g. in standards mode if the containing block is height:auto). I wonder if we can remove the isSpecified() check. I don't have a strong preference one way or another. > Source/WebCore/rendering/RenderFlexibleBox.h:88 > + LayoutUnit computeMainAxisSizeForChild(RenderBox* child, SizeType, const Length& size, LayoutUnit availableSize); Nit: I think we normally use Extent instead of Size for lengths. I don't feel strongly about this. > LayoutTests/css3/flexbox/percentage-sizes-quirks.html:37 > +<script> > +if (window.testRunner) > + testRunner.dumpAsText(); > +</script> Nit: I would probably print document.compatMode somewhere just to be sure. Comment on attachment 157004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157004&action=review >> Source/WebCore/rendering/RenderFlexibleBox.cpp:407 >> + return child->computeContentBoxLogicalWidth(valueForLength(size, availableSize, view())); > > This looks wrong for orthogonal flows, but I guess it's what we had before. Good catch. Adding a FIXME for now. >>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:817 >>>> + return minSize; >>> >>> The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug? >> >> computePercentageLogicalHeight can return -1 in some cases depending on the state of the containingBlock (e.g. in standards mode if the containing block is height:auto). > > I wonder if we can remove the isSpecified() check. I don't have a strong preference one way or another. Seems fine to me. I suppose the minSize != -1 is redundant with the childSize<minSize check since childSize is at least 0 (but the -1 check is definitely needed for maxSize). This just means we'll need to check of the auto case first since we want that to go down a special code path for flexitems. >> Source/WebCore/rendering/RenderFlexibleBox.h:88 >> + LayoutUnit computeMainAxisSizeForChild(RenderBox* child, SizeType, const Length& size, LayoutUnit availableSize); > > Nit: I think we normally use Extent instead of Size for lengths. I don't feel strongly about this. Yeah, I thought about it after the fact and was too lazy to change it. :) But you're right, I should be consistent. >> LayoutTests/css3/flexbox/percentage-sizes-quirks.html:37 >> +</script> > > Nit: I would probably print document.compatMode somewhere just to be sure. makes sense Comment on attachment 157004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157004&action=review >>>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:817 >>>>> + return minSize; >>>> >>>> The -1 check seems bad. Negative min/max height are illegal, so it shouldn't even be isSpecified() if it's negative. Is that a bug? >>> >>> computePercentageLogicalHeight can return -1 in some cases depending on the state of the containingBlock (e.g. in standards mode if the containing block is height:auto). >> >> I wonder if we can remove the isSpecified() check. I don't have a strong preference one way or another. > > Seems fine to me. I suppose the minSize != -1 is redundant with the childSize<minSize check since childSize is at least 0 (but the -1 check is definitely needed for maxSize). > > This just means we'll need to check of the auto case first since we want that to go down a special code path for flexitems. Actually, on closer look, the valueForLength functions all ASSERT_NOT_REACHED for intrinsic values other than FillAvailable and Auto. So, we should probably keep the checks in place. Committed r125055: <http://trac.webkit.org/changeset/125055> |