Bug 127588

Summary: [CSS Shapes] Image valued shape can fail
Product: WebKit Reporter: Hans Muller <giles_joplin>
Component: CSSAssignee: Hans Muller <giles_joplin>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, dino, esprehn+autocc, glenn, hyatt, kling, kondapallykalyan, krit, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test case.
none
Patch
hyatt: review-
Patch none

Description Hans Muller 2014-01-24 14:41:32 PST
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.
Comment 1 Hans Muller 2014-01-28 16:40:46 PST
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().
Comment 2 Dave Hyatt 2014-01-29 13:40:07 PST
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.
Comment 3 Hans Muller 2014-01-29 15:24:46 PST
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.
Comment 4 Dean Jackson 2014-01-31 10:48:29 PST
Comment on attachment 222598 [details]
Patch

It would be nice to have a vertical writing mode test too.
Comment 5 Radar WebKit Bug Importer 2014-01-31 10:49:39 PST
<rdar://problem/15958369>
Comment 6 Hans Muller 2014-01-31 11:25:57 PST
(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.
Comment 7 WebKit Commit Bot 2014-01-31 11:57:13 PST
Comment on attachment 222598 [details]
Patch

Clearing flags on attachment: 222598

Committed r163186: <http://trac.webkit.org/changeset/163186>
Comment 8 WebKit Commit Bot 2014-01-31 11:57:16 PST
All reviewed patches have been landed.  Closing bug.