[CSS Shapes][CSS Regions] Shape-inside on a parent element does not affect region content

[Bear Travis]

Created attachment 191342 [details]
Testcase

If the regions' parent element has a shape inside, it does not affect the flow content inside the regions.

[Zoltan Horvath]

Created attachment 192033 [details]
proposed patch

I turned the attached test case to different bugs: bug #111604 and bug #111607.

This change fixes the bug. The patch file probably doesn't contain the previous test moving to the new directory, but I can add it when committing.

[WebKit Review Bot]

Attachment 192033 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/regions/shape-inside-on-regions-expected.html', u'LayoutTests/fast/regions/shape-inside-on-regions.html', u'LayoutTests/fast/regions/shape-inside/shape-inside-on-parent-element-expected.html', u'LayoutTests/fast/regions/shape-inside/shape-inside-on-parent-element.html', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp']" exit_code: 1
LayoutTests/platform/chromium/TestExpectations:4397:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/qt/TestExpectations:2280:  Path does not exist.  [test/expectations] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Comment on attachment 192033 [details]
proposed patch

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

r-

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:86
> +        if (region) {

if (!region)
    return 0;

// Rest of code...

would read better. That way it's not so indented.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:95
> +            RenderBlock* parent = toRenderBlock(region->parent());
> +            while (parent) {
> +                if (parent->exclusionShapeInsideInfo())
> +                    return parent->exclusionShapeInsideInfo();
> +                parent = toRenderBlock(parent->parent());
> +            }

This code is broken. You can have non-RenderBlock parents in the render tree. I'm not sure exactly what you're trying to do here, so I'm not sure how to suggest you patch it. Can exclusions intrude into anything, even out-of-flow content? If so, then I guess you wanted containingBlock() instead of parent(). If not, then maybe you just need to go out until you establish a block formatting context? I would think objects that avoid floats would avoid exclusions also, so it seems like there needs to be some kind of stopping point here.

[Zoltan Horvath]

I modified the parent() to containingBlock():

Actually, I wanted to use 

while (containingBlock && containingBlock->flowThreadContainingBlock())

to run the loop only for the flow, but flowThreadContainingBlock() always returns 0, because their flowThreadState() is NotInsideFlowThread, though they are in a flow thread.

Index: Source/WebCore/rendering/RenderBlockLineLayout.cpp
===================================================================
--- Source/WebCore/rendering/RenderBlockLineLayout.cpp	(revision 145230)
+++ Source/WebCore/rendering/RenderBlockLineLayout.cpp	(working copy)
@@ -83,7 +83,17 @@
     if (!shapeInsideInfo && flowThreadContainingBlock()) {
         LayoutUnit offset = logicalHeight() + logicalHeightForLine(this, false);
         RenderRegion* region = regionAtBlockOffset(offset);
-        return region ? region->exclusionShapeInsideInfo() : 0;
+        if (!region)
+            return 0;
+        if (region->exclusionShapeInsideInfo())
+            return region->exclusionShapeInsideInfo();
+
+        RenderBlock* containingBlock = region->containingBlock();
+        while (containingBlock && containingBlock->flowThreadContainingBlock()) {
+            if (containingBlock->exclusionShapeInsideInfo())
+                return containingBlock->exclusionShapeInsideInfo();
+            containingBlock = containingBlock->containingBlock();
+        }
     }
     return shapeInsideInfo;
 }

[Dave Hyatt]

(In reply to comment #4)

> +        RenderBlock* containingBlock = region->containingBlock();
> +        while (containingBlock && containingBlock->flowThreadContainingBlock()) {
> +            if (containingBlock->exclusionShapeInsideInfo())
> +                return containingBlock->exclusionShapeInsideInfo();
> +            containingBlock = containingBlock->containingBlock();
> +        }

You're going up the region's containing block chain. The region isn't in a flow thread. It renders a portion of the flow thread. I guess I'm just confused why you have to walk up a containing block chain at all. Aren't you propagating exclusion info down to the region? Doesn't the region just know about it?

Also, please limit this code to isOutOfFlowRenderFlowThreads (you can call this on the flowThreadContainingBlock), since it's not going to work for in-flow RenderFlowThreads.

[Zoltan Horvath]

Comment on attachment 192033 [details]
proposed patch

The spec will be going with a more conservative behavior. => This bug is invalid.

[Alan Stearns]

Surprise! The spec changed to make this issue valid again.