RESOLVED FIXED 102846
[CSS Shapes] Use the float height to determine position in shape-inside 
https://bugs.webkit.org/show_bug.cgi?id=102846
Summary [CSS Shapes] Use the float height to determine position in shape-inside
Bem Jones-Bey
Reported 2012-11-20 15:14:58 PST
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.
Attachments
proposed patch (5.91 KB, patch)
2013-07-30 14:03 PDT, Zoltan Horvath 		no flags
DetailsFormatted DiffDiff
proposed patch (37.81 KB, patch)
2013-09-17 09:45 PDT, Zoltan Horvath 		darin: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
incorporating comments (37.98 KB, patch)
2013-09-17 13:18 PDT, Zoltan Horvath 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Zoltan Horvath
Comment 1 2013-07-30 14:03:23 PDT
Created attachment 207764 [details] proposed patch
Zoltan Horvath
Comment 2 2013-09-17 09:45:49 PDT
Created attachment 211916 [details] proposed patch
Darin Adler
Comment 3 2013-09-17 10:44:18 PDT
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.
Bem Jones-Bey
Comment 4 2013-09-17 10:58:35 PDT
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?
Build Bot
Comment 5 2013-09-17 12:14:11 PDT
Comment on attachment 211916 [details] proposed patch Attachment 211916 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1877799
Zoltan Horvath
Comment 6 2013-09-17 13:18:02 PDT
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.
WebKit Commit Bot
Comment 7 2013-09-17 20:26:29 PDT
Comment on attachment 211932 [details] incorporating comments Clearing flags on attachment: 211932 Committed r156022: <http://trac.webkit.org/changeset/156022>
WebKit Commit Bot
Comment 8 2013-09-17 20:26:32 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.