Created attachment 172882 [details] proposed patch

Comment on attachment 172882 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=172882&action=review The patch only tests a shape inside larger than the content area on the right/bottom sides. It also needs to deal with the upper/left sides. Check out the beginning of layoutRunsAndFloatsInRange and the setLogicalHeight call for how the top is set now. Also, the simple-rectangle.js script is designed to cover these basic test cases. I know that some of the changes here may break the simple float test case, but you might remove that test case to be added at a later date, since making floats work with shape-inside will require significant work. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:165 > m_left = max<float>(m_segment->logicalLeft, m_left); > - m_right = min<float>(m_segment->logicalRight, m_right); > + > + if (!(m_block->m_floatingObjects && m_block->m_floatingObjects->hasRightObjects()) && m_segment->logicalRight > m_right) > + m_right = m_segment->logicalRight; > + else > + m_right = min<float>(m_segment->logicalRight, m_right); Do you know that if there are right floating objects, they necessarily affect this line? I think that the floating objects set contains all of this block's floats, so they may not. I think these lines should just be: m_left = m_segment->logicalLeft; m_right = m_segment->logicalRight; This leaves the issue of floats, but perhaps the better thing would be to pull out the float test and implement that in a subsequent patch. > LayoutTests/fast/exclusions/shape-inside/shape-inside-with-larger-content-area.html:16 > + -webkit-shape-inside: rectangle(0, 0, 200px, 200px); You need to test the rectangle going beyond the bounds of each of the boxes (content, padding, border, margin). I think to start, just going outside the margin box would be sufficient. eg, #shape-inside { position: absolute; left: 10px; top: 10px; width: 180px; height: 180px; -webkit-shape-inside: rectangle(-10px, -10px, 200px, 200px); } Although really, this should be wrapped into using resources/simple-rectangle.js, in which case you would include the custom styling: #shape-inside { position: absolute; left: 10px; top: 10px; } and the script: createRectangleTest('shape-inside', 'stylesheet', { width: 200, height: 200 }, { x: -10, y: -10, width: 200, height: 200 }, 'px', null); // use null for the last argument if you want to leave shape-inside's content in place You can also see shape-inside-subsequent-blocks, and shape-inside-multiple-blocks-dynamic. The scripts obviate the need for the 'helper' element.

Comment on attachment 172882 [details] proposed patch I removed the patch from the review queue since it needs some improvement. Thanks for the comments, things are one step clearer know.

The spec has been changed. This bug is not valid anymore.