[Webkit-unassigned] [Bug 64516] [CSSRegions] Collect flowed elements in different render element

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64516


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #101467|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #9 from Dave Hyatt <hyatt at apple.com>  2011-07-20 11:11:32 PST ---
(From update of attachment 101467)
View in context: https://bugs.webkit.org/attachment.cgi?id=101467&action=review

Few more comments.

> Source/WebCore/rendering/RenderFlowThread.cpp:84
> +RenderObject* RenderFlowThread::nextRendererForNode(Node* node) const
> +{
> +    for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
> +        ASSERT(child->node());
> +        unsigned short position = node->compareDocumentPosition(child->node());
> +        if (position & Node::DOCUMENT_POSITION_FOLLOWING)
> +            return child;
> +    }
> +    return 0;
> +}
> +
> +RenderObject* RenderFlowThread::previousRendererForNode(Node* node) const
> +{
> +    for (RenderObject* child = lastChild(); child; child = child->previousSibling()) {
> +        ASSERT(child->node());
> +        unsigned short position = node->compareDocumentPosition(child->node());
> +        if (position & Node::DOCUMENT_POSITION_PRECEDING)
> +            return child;
> +    }
> +    return 0;
> +}

The one concern I can think of with these functions is that the RenderFlowThread may have been forced to create anonymous blocks to wrap content. For example let's say you put both a <div> and a <span> into the same flow thread. When you put the <span> into the thread, an anonymous block will be created to wrap the <span>. People can also write weird code like div { display: table-cell } and throw that into a RenderFlowThread, and a bunch of anonymous table elements would be created to wrap that object as well.

I think you will probably have to maintain an actual list of the nodes themselves in the RenderFlowThread and walk that instead. Something similar to the ListHashSet used for candidate stylesheet nodes that I mentioned in the previous review.

> Source/WebCore/rendering/RenderLayer.cpp:2551
> +#if ENABLE(CSS_REGIONS)
> +    // Do not let the RenderFlowThread to render directly to screen. It will only render
> +    // inside the RenderRegions.
> +    if (renderer()->isRenderFlowThread())
> +        return;
> +#endif

I think a better way to handle this would be to simply avoid collecting the child layers into the lists. That way you only have to patch one place. I think you could just patch updateZOrderLists to skip flow thread layers. That seems a bit simpler to me than patching both painting and hit testing functions.

> Source/WebCore/rendering/RenderLayer.h:-327
> -    bool isStackingContext() const { return !hasAutoZIndex() || renderer()->isRenderView(); }

I don't think you'll have to patch this function if you just hack updateZOrderLists to avoid collecting the layers instead.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.