[webkit-reviews] review denied: [Bug 111176] [CSS Regions] position: fixed is computed relative to the first region, not the viewport : [Attachment 206646] Patch 3

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Mihnea Ovidenie <mihnea at adobe.com>'s
request for review:
Bug 111176: [CSS Regions] position: fixed is computed relative to the first
region, not the viewport
https://bugs.webkit.org/show_bug.cgi?id=111176

Attachment 206646: Patch 3
https://bugs.webkit.org/attachment.cgi?id=206646&action=review

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


r-

In general, I don't like what's happening here, but it's a spec disagreement. I
would rather fixed positioned objects just not be placed inside the named flow
thread at all. I don't see any value whatsoever in allowing them to be inside
the flow thread. If instead they were just outside the named flow thread
completely, then all of this code would get way simpler. Is there any
compelling use case for fixed positioned objects being inside the flow threads?


Anyway, the rest of the comments are assuming the spec stays the way it is, so
here goes:

Don't you need to patch RenderLayer::accumulateOffsetTowardsAncestor also?

> Source/WebCore/rendering/FlowThreadController.cpp:105
>	   flowRenderer->layoutIfNeeded();
> +	  
flowRenderer->view()->layoutNamedFlowFixedPositionedObjects(flowRenderer);

I would make a member function on RenderNamedFlowThread that handles doing both
of these operations.

layoutNormalFlowAndFixedPositionedObjects.

> Source/WebCore/rendering/RenderBlock.cpp:2878
> +	   if (namedFlow) {
> +	       ASSERT(this->isRenderView());
> +	       if (namedFlow !=
r->flowThreadAncestorForFixedPositionedObjects())
> +	       continue;
> +	   }

I don't like this code in RenderBlock when it's view-specific. Also, you put it
in front of comment that describes code that comes after the code you put in,
so you just disconnected the comment from some code it applied to. 

I would suggest doing something differently here. I think you should refactor
layoutPositionedObjects so that the work it does for each object inside the
loop is in a helper function, e.g., layoutPositionedObject(). Then you could
just have a complete implementation of laying out positioned objects over in
layoutNamedFlowPositionedObjects that has your named flow filtering check and
that then just calls layoutPositionedObject() to do the rest of the work. That
keeps you from having to touch this RenderBlock code.

> Source/WebCore/rendering/RenderBlock.h:511
> -    void layoutPositionedObjects(bool relayoutChildren, bool
fixedPositionObjectsOnly = false);
> +    void layoutPositionedObjects(bool relayoutChildren, bool
fixedPositionObjectsOnly = false, const RenderNamedFlowThread* = 0);

Shouldn't have to add this, as per my comment above.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1787
> +	   && !layer->isOutOfFlowRenderFlowThread();

These aren't equivalent. You just changed behavior for in-flow
RenderFlowThreads.

> Source/WebCore/rendering/RenderLayerModelObject.cpp:136
> +		   if (flowThreadState() ==
RenderObject::InsideOutOfFlowThread) {
> +		       bool wasFixedPositionedInFlowThread =
oldStyle->position() == FixedPosition;
> +		       bool isFixedPositionedInFlowThread =
newStyle->position() == FixedPosition;
> +		       if (wasFixedPositionedInFlowThread !=
isFixedPositionedInFlowThread)
> +			   layer()->dirtyStackingContainerZOrderLists();

Helper function please.

> Source/WebCore/rendering/RenderLayerModelObject.cpp:182
> +	   if (flowThreadState() == RenderObject::InsideOutOfFlowThread) {
> +	       bool wasFixedPositionedInFlowThread = oldStyle &&
oldStyle->position() == FixedPosition;
> +	       bool isFixedPositionedInFlowThread = style()->position() ==
FixedPosition;
> +	       if (wasFixedPositionedInFlowThread !=
isFixedPositionedInFlowThread)
> +		   layer()->dirtyStackingContainerZOrderLists();

Helper function please, since this is identical to the code above.

> Source/WebCore/rendering/RenderObject.cpp:631
> +bool RenderObject::fixedPositionInNamedFlowWithViewContainingBlock() const
> +{
> +    return (flowThreadState() == RenderObject::InsideOutOfFlowThread)
> +	   && (style()->position() == FixedPosition)
> +	   && (containingBlock() == view());
> +}

This is only used by RenderLayer. I'd just put it over there rather than adding
new code to RenderObject.

> Source/WebCore/rendering/RenderObject.cpp:665
> +RenderFlowThread*
RenderObject::flowThreadAncestorForFixedPositionedObjects() const
> +{
> +    if ((flowThreadState() != InsideOutOfFlowThread) || (style()->position()
!= FixedPosition))
> +	   return 0;
> +
> +    RenderObject* curr = const_cast<RenderObject*>(this);
> +    while (curr) {
> +	   if (curr->isRenderFlowThread())
> +	       return toRenderFlowThread(curr);
> +	   curr = curr->parent();
> +    }
> +
> +    return 0;
> +}

I would factor this a little differently. I would have a RenderObject function
that does only the while loop. Make flowThreadContainingBlock() also use it for
the slow case. Then RenderNamedFlowThread could do the flowThreadState() and
fixedPosition condition checks in its own loop, and then all it has to do is
call the crawl function if those pass.

> Source/WebCore/rendering/RenderObject.cpp:820
> +    || (isOutOfFlowRenderFlowThread() &&
!toRenderFlowThread(this)->hasRegions());

This check confuses me. I don't quite understand it. Why is hasRegions()
supposed to be false here?

> Source/WebCore/rendering/RenderView.cpp:141
> +void RenderView::layoutNamedFlowFixedPositionedObjects(const
RenderNamedFlowThread* namedFlow)
> +{
> +    layoutPositionedObjects(false, true, namedFlow);
> +}

I would implement the loop yourself and do the filtering yourself.