[webkit-reviews] review denied: [Bug 65477] Nested fixed position element not staying with parent : [Attachment 119763] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 65477: Nested fixed position element not staying with parent
https://bugs.webkit.org/show_bug.cgi?id=65477

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119763&action=review


> Source/WebCore/rendering/RenderBlock.cpp:2215
> +    } else if (isRenderView() || hasTransform()) {
> +	   // If an absolute position element inside a relative positioned
container moves, and the absolute element has a fixed position
> +	   // child, neither the renderview nor the fixed element learn of the
movement since posChildNeedsLayout() is only marked as far as the 
> +	   // relative positioned container. So check if any fixed position
elements in the renderview's need to move with their absolute ancestors.
> +	   layoutFixedPositionObjects();
> +    }

Why do we need a separate layout function? (which means another positioned
object traversal). Isn't it possible to actually not layout some positioned
child because of your logic?

> Source/WebCore/rendering/RenderBlock.cpp:2305
> +    bool positionedAncestorForcesLayout = false;

The name is misleading. It's not the ancestor that you are checking but the
siblings.

> Source/WebCore/rendering/RenderBlock.cpp:2319
> +	   if (r->style()->position() == AbsolutePosition && r->needsLayout())
> +	       positionedAncestorForcesLayout = true;
> +	   else if (r->style()->position() == FixedPosition &&
positionedAncestorForcesLayout && r->parent() != this)
> +	       r->setChildNeedsLayout(true, false);

I don't think I understand this logic. If you have seen an absolute position
sibling then it impacts this sibling |r|. It doesn't sound good to me.

> LayoutTests/fast/inline/bug65477-fixed-position-movement.html:55
> +  <div id='wrap'>

The test should include the bug title and number. Also it would be easier to
see what's wrong if you actually said what you expect and why. (I am guessing
we would put the children under the wrong parent but that's a guess).