Summary: | [CSS Shapes] Image valued shape can fail | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hans Muller <giles_joplin> | ||||||||
Component: | CSS | Assignee: | 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: |
|
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 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. 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 on attachment 222598 [details]
Patch
It would be nice to have a vertical writing mode test too.
(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 on attachment 222598 [details] Patch Clearing flags on attachment: 222598 Committed r163186: <http://trac.webkit.org/changeset/163186> All reviewed patches have been landed. Closing bug. |
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.