[webkit-reviews] review denied: [Bug 118665] [CSS Regions] Implement visual overflow for first & last regions : [Attachment 207087] Patch integrating feedback

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Radu Stavila <stavila at adobe.com>'s
request for review:
Bug 118665: [CSS Regions] Implement visual overflow for first & last regions
https://bugs.webkit.org/show_bug.cgi?id=118665

Attachment 207087: Patch integrating feedback
https://bugs.webkit.org/attachment.cgi?id=207087&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=207087&action=review


r- for a number of writing-mode issues and the weird parent walk that could
just use containing blocks.

I think you might find that you could use a simpler approach here though.
Instead of trying to write your own propagation code, I think instead you
should just mark RenderRegions for a simplified layout pass if you detect that
the RenderFlow has overflow that needs to propagate. If you do that, then the
layout code can just handle it. You can have RenderRegion when it computes its
overflow ask the flow thread if it's in a good state to hand the overflow back
and if so, get it from the flow thread.

I think this will be much less code, and you won't have to add all this code to
try to propagate outside of layout etc. It also won't work anyway, since the
layer code has cached some overflow stuff that you're not fixing up.

> Source/WebCore/rendering/RenderLayer.cpp:4434
> +    LayoutRect hitTestArea;
> +    if (isOutOfFlowRenderFlowThread()) {
> +	   RenderFlowThread* renderFlowThread = toRenderFlowThread(renderer());

> +	   hitTestArea = renderFlowThread->hasVisualOverflow() ?
renderFlowThread->visualOverflowRect() : renderFlowThread->borderBoxRect();
> +    } else
> +	   hitTestArea = renderer()->view()->documentRect();
> +    

This is wrong. Overflow is not physical. It is in the coordinate space of the
block, so you can't use visualOverflowRect without flipping it for writing
mode.

> Source/WebCore/rendering/RenderRegion.cpp:103
> +void RenderRegion::addVisualOverflowFromFlowThreadOverflow()
> +{
> +    LayoutRect flowThreadRect = flowThreadPortionOverflowRect();
> +    flowThreadRect.expand(borderLeft(), borderTop());
> +    flowThreadRect.expand(paddingLeft(), paddingTop());
> +    addVisualOverflow(flowThreadRect);
> +}

This is not correct either. In the case of flipped blocks, the border and
padding can be on the bottom and right.

Also, if we're going to support the writing mode of the region differing from
the writing mode of the flow thread (do we support that now?), then this code
isn't going to be correct either. I don't think you necessarily have to get
that right, but you should at least switch to using the correct border sides
for flipped block writing modes.

> Source/WebCore/rendering/RenderRegion.cpp:120
> +	   RenderObject* objParent = renderChild->parent();
> +	   ASSERT(objParent);
> +	   
> +	   // Skip non-box parents (for instance a region sitting inside a
span).
> +	   while (!objParent->isBox())
> +	       objParent = objParent->parent();

Just use containingBlock(). Overflow follows the containing block hierarchy.
This will simplify all of this code.