Bug 127588 - [CSS Shapes] Image valued shape can fail
Summary: [CSS Shapes] Image valued shape can fail
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Muller
Keywords: InRadar
Depends on:
Reported: 2014-01-24 14:41 PST by Hans Muller
Modified: 2014-01-31 11:57 PST (History)
12 users (show)

See Also:

Test case. (918 bytes, text/html)
2014-01-24 14:41 PST, Hans Muller
no flags Details
Patch (21.09 KB, patch)
2014-01-28 16:40 PST, Hans Muller
hyatt: review-
Details | Formatted Diff | Diff
Patch (21.10 KB, patch)
2014-01-29 15:24 PST, Hans Muller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]

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]

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


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]

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]

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
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]

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.