The height of the float should be used to determine the longest segment offset over it's height, and that should be used to position it. Otherwise, in the case of a shape that gets narrower as it goes down, the float will be positioned so that it's edge is outside of the shape.
Created attachment 207764 [details] proposed patch
Created attachment 211916 [details] proposed patch
Comment on attachment 211916 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review > Source/WebCore/rendering/LineWidth.cpp:210 > +void LineWidth::updateCurrentLineSegment() > +{ > + if (ShapeInsideInfo* shapeInsideInfo = m_block.layoutShapeInsideInfo()) > + m_segment = shapeInsideInfo->currentSegment(); > +} Does this need to go inside #if ENABLE(CSS_SHAPES)? > Source/WebCore/rendering/RenderBlock.cpp:3387 > + // FIXME Bug 102949: Add support for shapes with multipe segments. multiple rather than multipe > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2580 > + bool lineOverlapsWithFloat = lastFloatFromPreviousLine->y() < (block->logicalHeight() + lineLogicalHeight) && (block->logicalHeight() < lastFloatFromPreviousLine->maxY()); Parentheses here make this expression confusing. I, at least, would be able to read it bette without those parens. If you really do want parens, I would suggest one more set around the expression before the &&. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2587 > + if ((block->width() - lastFloatFromPreviousLine->maxX()) < minWidth) Again, I find the parentheses make it harder to read this expression.
Comment on attachment 211916 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review Here's the feedback we talked about in person. > LayoutTests/fast/shapes/shape-inside/shape-inside-left-float-in-lower-left-triangle-inline-content-expected.html:1 > +<html> You should put <!DOCTYPE html> at the top of all of your test files, unless you have a good reason not to. (Same feedback on all of the test files) > Source/WebCore/rendering/LineWidth.cpp:206 > +void LineWidth::updateCurrentLineSegment() I think that updateCurrentShapeSegment would be a clearer name. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2574 > +static void adjustLineTopAndBlockLogicalHeightForShapeInsideWithFloats(RenderBlock* block, const FloatingObject* lastFloatFromPreviousLine, const WordMeasurements& wordMeasurements, LineWidth& width, bool isFirstLine) This name makes me think that this method is trying to do too many things. Maybe there's a way to break it up into multiple methiods?
Comment on attachment 211916 [details] proposed patch Attachment 211916 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1877799
Created attachment 211932 [details] incorporating comments (In reply to comment #3) > (From update of attachment 211916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review > Does this need to go inside #if ENABLE(CSS_SHAPES)? You're right! I added the missing guard to the patch. > > Source/WebCore/rendering/RenderBlock.cpp:3387 > > + // FIXME Bug 102949: Add support for shapes with multipe segments. > > multiple rather than multipe Fixed. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2580 > > + bool lineOverlapsWithFloat = lastFloatFromPreviousLine->y() < (block->logicalHeight() + lineLogicalHeight) && (block->logicalHeight() < lastFloatFromPreviousLine->maxY()); > > Parentheses here make this expression confusing. I, at least, would be able to read it bette without those parens. If you really do want parens, I would suggest one more set around the expression before the &&. Fixed. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2587 > > + if ((block->width() - lastFloatFromPreviousLine->maxX()) < minWidth) > > Again, I find the parentheses make it harder to read this expression. Fixed too. (In reply to comment #4) > (From update of attachment 211916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211916&action=review > > You should put <!DOCTYPE html> at the top of all of your test files, unless you have a good reason not to. (Same feedback on all of the test files) Done. > > Source/WebCore/rendering/LineWidth.cpp:206 > > +void LineWidth::updateCurrentLineSegment() > > I think that updateCurrentShapeSegment would be a clearer name. I updated the function name. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2574 > > +static void adjustLineTopAndBlockLogicalHeightForShapeInsideWithFloats(RenderBlock* block, const FloatingObject* lastFloatFromPreviousLine, const WordMeasurements& wordMeasurements, LineWidth& width, bool isFirstLine) > > This name makes me think that this method is trying to do too many things. Maybe there's a way to break it up into multiple methiods? I renamed the function. I'll probably split the method up in one of the following floats-shape-inside patches. Thank you guys to look at the patch. I'm giving it to the EWS now, then land.
Comment on attachment 211932 [details] incorporating comments Clearing flags on attachment: 211932 Committed r156022: <http://trac.webkit.org/changeset/156022>
All reviewed patches have been landed. Closing bug.