Bug 102846

Summary:

[CSS Shapes] Use the float height to determine position in shape-inside

Product:

WebKit

Reporter:

Bem Jones-Bey <bjonesbe>

Component:

CSS

Assignee:

Zoltan Horvath <zoltan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bjonesbe, commit-queue, donggwan.kim, esprehn+autocc, glenn, kondapallykalyan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

http://dev.w3.org/csswg/css3-exclusions/

Bug Depends on:

Bug Blocks:

89256, 119274, 121615, 121616

Attachments:

Description	Flags
proposed patch	none
proposed patch	darin: review+, buildbot: commit-queue-
incorporating comments	none

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	Details Formatted Diff Diff
proposed patch (37.81 KB, patch) 2013-09-17 09:45 PDT, Zoltan Horvath	darin: review+ buildbot: commit-queue-	Details Formatted Diff Diff
incorporating comments (37.98 KB, patch) 2013-09-17 13:18 PDT, Zoltan Horvath	no flags	Details Formatted Diff Diff
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.