[CSS Shapes] Image valued shape can fail

[Hans Muller]

Created attachment 222157 [details]
Test case.

If an image shape's implicit scale,translation causes the origin of the image to be offset more than shape-margin in the negative Y direction, the RasterShapeIntervals() class will fail in an ASSERT:

ASSERTION FAILED: static_cast<int>(y + m_shapeMargin) >= 0 && y + m_shapeMargin < m_intervalLists.size()
/Users/hansmuller/webkit/Source/WebCore/rendering/shapes/RasterShape.h(65) : IntShapeIntervals &WebCore::RasterShapeIntervals::intervalsAt(int)

The attached test case demonstrates the problem.

[Hans Muller]

Created attachment 222531 [details]
Patch

Correct the handling of image valued shapes that extend into or beyond the margin box. This can happen when object-fit causes one dimension of the shape to be greater than the corresponding margin box dimension.

Made some simplifications in RasterShapeIntervals::computeShapeMarginIntervals() by making the shapeMargin parameter an int, removing some unnecessary variables.

Revised the RasterShapeIntervals class. It's now a just a list of size() interval-lists, with valid indices: -offset() <= y < size() - offset(), or minY() to maxY(). If margin-top and shape-margin are specified, then offset() is the larger of shape-margin and margin-top. Similarly size() is the vertical size of the margin-box or the content-box expanded by shape-margin, whichever is larger. See computeShapeMarginIntervals().

[Dave Hyatt]

Comment on attachment 222531 [details]
Patch

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

r-

Seems muddled with regards to vertical writing.

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:75
> +static LayoutRect getShapeImageMarginRect(const RenderBox& renderBox, const LayoutSize& shapeSize)
> +{
> +    LayoutPoint marginBoxOrigin(-renderBox.marginLogicalLeft() - renderBox.borderAndPaddingLogicalLeft(), -renderBox.marginTop() - renderBox.borderTop() - renderBox.paddingTop());
> +    LayoutSize marginBoxSizeDelta(renderBox.marginLogicalWidth() + renderBox.borderAndPaddingLogicalWidth(), renderBox.marginLogicalHeight() + renderBox.borderAndPaddingLogicalHeight());
> +    return LayoutRect(marginBoxOrigin, shapeSize + marginBoxSizeDelta);
> +}

This looks wrong with vertical writing. You're mixing physical and logical margins.

[Hans Muller]

Created attachment 222598 [details]
Patch

Yes, what I'd written was inconsistent.  Sorry about that. 

I'm now using the {padding,border,margin}Before() methods to compute the vertical coordinate of the margin rect.  Hopefully I've got that right.

[Dean Jackson]

Comment on attachment 222598 [details]
Patch

It would be nice to have a vertical writing mode test too.

[Radar WebKit Bug Importer]

<rdar://problem/15958369>

[Hans Muller]

(In reply to comment #4)
> (From update of attachment 222598 [details])
> It would be nice to have a vertical writing mode test too.

Good point.  I will add some writing mode depend tests in a separate patch.

[WebKit Commit Bot]

Comment on attachment 222598 [details]
Patch

Clearing flags on attachment: 222598

Committed r163186: <http://trac.webkit.org/changeset/163186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.