Bug 89993

Created attachment 161344 [details] Child blocks respect parent's shape-inside

Created attachment 162105 [details] LayoutState Approach New code approach adding WrapShapeInfo to LayoutState, rather than always using the parent chain to look up WrapShapeInfo.

Comment on attachment 162105 [details] LayoutState Approach Attachment 162105 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13734951

Created attachment 162130 [details] Fixing windows build error Surrounding a exclusions-specific test with compile flags

Created attachment 162279 [details] Removing duplicate changelog entry

Comment on attachment 162279 [details] Removing duplicate changelog entry View in context: https://bugs.webkit.org/attachment.cgi?id=162279&action=review AFAICT it's OK to hop onto LayoutState in this case (exclusions behaves like multi-column from that perspective). Dave or Simon may have more perspective about that though. Note that it's not strictly required but it avoids having to walk up the tree to find your enclosing wrapShapeInfo. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1301 > + logicalTopOffset = logicalTop() + view()->layoutState()->layoutOffset().height(); If layout state is disabled, I don't think this is correct. We may be fine with that for now, but it's definitely worth a FIXME. > Source/WebCore/rendering/RenderView.h:229 > + || (renderer->isRenderBlock() && toRenderBlock(renderer)->wrapShapeInfo()) We may want to add a RenderObject::hasWrapShapeInfo()?

Created attachment 162833 [details] Incorporating feedback Incorporating Julien's feedback. Mentioning multi-column layout in changelog Adding FIXME to the layout offset Not yet adding hasWrapShape to RenderObject, but if we have more similar tests, I will.

Comment on attachment 162833 [details] Incorporating feedback View in context: https://bugs.webkit.org/attachment.cgi?id=162833&action=review In general I have concerns here about propagating everything through LayoutState. I'm not sure of this approach. It could be I have a misunderstanding of WrapShapeInfo though. Is this struct used both for being inside and outside, i.e., is it used both to fit lines inside a shape as well as to help lines avoid intruding into a shape from the outside? If it's the latter, I am concerned about mindlessly propagating every last bit of information from ancestors down to children. It seems like you'd run into performance problems if you haven't limited the scope somehow. Mostly though I'm concerned about writing modes, since connecting to LayoutState is going to get you into trouble. Another question I have is it looks like an inner WrapShapeInfo will override an outer WrapShapeInfo? Does only one truly apply? Can multiple ones not apply? Do you not have to go up the LayoutState stack to examine all of them? > Source/WebCore/rendering/LayoutState.cpp:46 > + , m_wrapShapeInfo(0) I'd still like to see a rename at some point from wrapShape -> exclusionShape. Especially once the ifdefs eventually fall away, people aren't going to link "wrap' with the exclusions feature. > Source/WebCore/rendering/LayoutState.h:98 > + WrapShapeInfo* wrapShapeInfo() { return m_wrapShapeInfo; } This method should be const. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:83 > + WrapShapeInfo* wrapShapeInfo = m_block->view()->layoutState()->wrapShapeInfo(); Rather than removing the RenderBlock method, I think you'd be better off having it still exist and return view()->layoutState()->wrapShapeInfo(). > Source/WebCore/rendering/RenderBlockLineLayout.cpp:807 > + WrapShapeInfo* wrapShapeInfo = view()->layoutState()->wrapShapeInfo(); Ditto here. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1298 > + LayoutUnit logicalTopOffset; This is not a good name, since it sounds like you're in local coordinates. With the layout offset applied, though, this is really absolute coordinates. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1302 > + logicalTopOffset = logicalTop() + view()->layoutState()->layoutOffset().height(); You've got a big writing modes problem here. We don't use LayoutState for flipped blocks writing modes (the LayoutState gets disabled), since right now m_layoutOffset isn't accounting for flipping. Connecting propagation to m_layoutOffset creates a big problem, since LayoutState basically doesn't work with flipped blocks writing modes. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1305 > + if (wrapShapeInfo && logicalHeight() + logicalTopOffset < wrapShapeInfo->shapeTop()) > + setLogicalHeight(wrapShapeInfo->shapeTop() - logicalTopOffset); Everything seems completely muddled here with writing modes. You can't just add the layout offset's height to a logical top. The layout offset is physical and logicalTop is not. I still don't know what shapeTop means. Is it logical? Is it physical? I don't know. > Source/WebCore/rendering/RenderView.h:227 > + || (renderer->isRenderBlock() && toRenderBlock(renderer)->wrapShapeInfo()) It's a little weird to me that once a parent has wrap shape info, that you then will push that along down to every descendant. It seems like your filtering needs to be smarter than that. Aren't you going to want to limit this scope?

Created attachment 162919 [details] Incorporating feedback Fixing up patch WrapShapeInfo only deals with shape-inside, which is why one RenderBlock's shape-inside affects all descendants, and a new shape-inside overrides the previous one. I have added bug 96160 to rename WrapShapeInfo to something more useful, ExclusionShapeInfo or ShapeInsideInfo. I have also logged bug 96161 to refactor WrapShapeInfo to be more straightforward when dealing with logical vs physical coordinates. WrapShapes should be sized physically (based on the width/height of their block), but passed logical coordinates when computing line segments. Other fixes: LayoutState::wrapShapeInfo was made const Factored out block->view()->layoutState()->wrapShapeInfo() to a layoutWrapShapeInfo() function. RenderBlock::wrapShapeInfo() is still necessary to tell whether or not a given block has a shape-inside from CSS Renamed logicalTopOffset to absoluteLogicalTop absoluteLogicalTop will now use layoutOffset width or height depending on writing mode shapeTop will be renamed to logicalTop as part of bug 96161

Created attachment 163241 [details] Updating patch Merging with recent changes.

Comment on attachment 163241 [details] Updating patch View in context: https://bugs.webkit.org/attachment.cgi?id=163241&action=review Just some minor suggestions at this point. Make sure to follow up on the rename of wrapShapeInfo, if it really is only about "inside". > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1313 > + absoluteLogicalTop = logicalTop(); > + // FIXME: If layout state is disabled, the offset will be incorrect > + if (isHorizontalWritingMode()) > + absoluteLogicalTop += view()->layoutState()->layoutOffset().height(); > + else > + absoluteLogicalTop += view()->layoutState()->layoutOffset().width(); Minor nitpick, but I think this would read better if you cache in a local. Consider changing this block of code to: LayoutSize layoutOffset = view()->layoutState()->layoutOffset(); absoluteLogicalTop = logicalTop() + (isHorizontalWritingMode() ? layoutOffset.height() : layoutOffset.width()); > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1317 > + if (wrapShapeInfo && logicalHeight() + absoluteLogicalTop < wrapShapeInfo->shapeLogicalTop()) > + setLogicalHeight(wrapShapeInfo->shapeLogicalTop() - absoluteLogicalTop); Can't this go inside the previous if statement that assigned to wrapShapeInfo? Then it would be inside that if and just turn into: if (logicalHeight() + absoluteLogicalTop < wrapShapeInfo->shapeLogicalTop()) since you already know wrapShapeInfo isn't null.

Created attachment 163984 [details] Incorporating feedback Fixing up based on Dave Hyatt's comments.

Comment on attachment 163984 [details] Incorporating feedback View in context: https://bugs.webkit.org/attachment.cgi?id=163984&action=review Could you add some testing with vertical writing modes? The previous patch was wrong because it confused physical coordinates and logical ones which would have been caught by such a test. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1307 > + WrapShapeInfo* wrapShapeInfo = layoutWrapShapeInfo(this); > + if (wrapShapeInfo) { Nit: maybe it would be better just: if (WrapShapeInfo* wrapShapeInfo = layoutWrapShapeInfo(this)) { > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1313 > + // Move to the top of the shape inside to begin layout This comment is not super clear IMO. How about: // Layout start at the logical top of our shape inside. (feel free to adapt the prose)

Created attachment 165463 [details] Incorporating feedback Addressing Julien's feedback. Leaving wrapShapeInfo in place, as it is subsequently used in the while loop. Modifying comment in RenderBlockLineLayout to be more clear. Adding a vertical writing mode test.

Comment on attachment 165463 [details] Incorporating feedback View in context: https://bugs.webkit.org/attachment.cgi?id=165463&action=review > Source/WebCore/rendering/LayoutState.cpp:116 > + RenderBlock* renderBlock = toRenderBlock(renderer); > + m_wrapShapeInfo = renderBlock->wrapShapeInfo(); Unless you are sure you need |renderBlock| in a follow-up patch, I would write it on one line (as it is not used outside this scope): m_wrapShapeInfo = toRenderBlock(renderer)->wrapShapeInfo(); > LayoutTests/fast/exclusions/shape-inside/shape-inside-multiple-blocks.html:12 > + -webkit-shape-inside: rectangle(10px, 10px, 180px, 280px); It's usually better to select different values for each direction (as you did for the vertical writing mode test). That makes it easier to spot the wrong size being used.

Created attachment 165507 [details] Updated patch Incorporating Julien's feedback. Removed the RenderBlock variable. Changed the shape-inside to be asymmetrical.

Comment on attachment 165507 [details] Updated patch Clearing flags on attachment: 165507 Committed r129530: <http://trac.webkit.org/changeset/129530>

All reviewed patches have been landed. Closing bug.