[CSS Shapes] Shape's content gets extra left offset when left-border is positive on the content box

[Zoltan Horvath]

I will attach a test file later.

[Bear Travis]

I think this may be related to bug 110475

[Bear Travis]

Created attachment 206482 [details]
Initial patch

[Dave Hyatt]

Comment on attachment 206482 [details]
Initial patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206482&action=review

r-

> Source/WebCore/rendering/RenderBlock.cpp:1439
> +LayoutSize RenderBlock::offsetFromShapeAncestorContainer(const RenderBlock* container) const

Please rename this to logicalOffsetFromShapeAncestorContainer to reflect that you're putting the y into the width() variable in vertical writing modes.

> Source/WebCore/rendering/RenderBlock.cpp:1442
> +    LayoutRect blockRect(0, 0, currentBlock->width(), currentBlock->height());

This is borderBoxRect(), so you can initialize to that, e.g., LayoutRect blockRect(currentBlock->borderBoxRect());

> Source/WebCore/rendering/RenderBlock.cpp:1450
> +        LayoutPoint currentBlockLocation = currentBlock->location();
> +
> +        blockRect.moveBy(currentBlockLocation);

This doesn't work when you cross writing mode boundaries. Remember that block coordinates can be flipped. When you cross into writing modes with different flips, you have to adjust accordingly. You'll want to test with mixed vertical-rl and vertical-lr and/or horizontal-tb/horizontal-bt to see what I mean.

> Source/WebCore/rendering/RenderBlock.cpp:2741
> +    LayoutSize childOffset = child->location() - oldRect.location();
> +    if (childOffset.width() && childRenderBlock && layoutShapeInsideInfo() && !childRenderBlock->shapeInsideInfo()) {
> +        childRenderBlock->setNormalChildNeedsLayout(true);
> +        childRenderBlock->markShapeInsideDescendantsForLayout();
> +        childRenderBlock->layoutIfNeeded();
> +    }

Put this in a helper function. Also, why are you only checking width? Is the intent to look for a change in logical left position? If so, you need to check height() if vertical.

> Source/WebCore/rendering/RenderBlock.h:591
> +    LayoutSize offsetFromShapeAncestorContainer(const RenderBlock* container) const;

logicalOffset...

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1688
> +    shapeInsideInfo->computeSegmentsForLine(lineLeft, lineTop, lineHeight);

Use a LayoutSize rather than two arguments here.

> Source/WebCore/rendering/shapes/ShapeInsideInfo.h:71
> +    bool computeSegmentsForLine(LayoutUnit lineLeft, LayoutUnit lineTop, LayoutUnit lineHeight)

Why not combine lineLeft/lineTop into a LayoutSize offset?

[Bear Travis]

Created attachment 207341 [details]
Updated patch

[Bear Travis]

Created attachment 207344 [details]
Updating changelogs

[Bear Travis]

Created attachment 208828 [details]
Updated patch

[Build Bot]

Comment on attachment 208828 [details]
Updated patch

Attachment 208828 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1544282

[Bear Travis]

Created attachment 210139 [details]
Updated patch

[Dave Hyatt]

Comment on attachment 210139 [details]
Updated patch

r=me

[WebKit Commit Bot]

Comment on attachment 210139 [details]
Updated patch

Clearing flags on attachment: 210139

Committed r155002: <http://trac.webkit.org/changeset/155002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.