Bug 83981 – [Qt][WK2] Fixed elements position is wrong after zooming.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yael]

After zooming, our visibleContentRect becomes smaller and the position of fixed elements is no longer valid.
We need to re-layout these elements after the size change.

[Yael]

Created attachment 137261 [details]
Patch.

Turn on the flag setFixedElementsLayoutRelativeToFrame, so that fixed elements are calculated based on the visibleWidth and visibleHeight of the view.

When the size changes, mark all fixed elements for layout. RenderView maintains a list of fixed elements, so it is easy to find them.
This was adapted from iOS 5.1 branch at opensource.apple.com.

[Kenneth Rohde Christiansen]

Comment on attachment 137261 [details]
Patch.

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

> ManualTests/remove-add-fixed-position.html:5
> +.d1{position:fixed;top:5;right:5;z-index:2;overflow:hidden;}
> +.o {background:green;height:40px;width:200px;}

Some spacing here would be nice :-) or even some newlines.

> Source/WebCore/ChangeLog:8
> +        When setFixedVisibleContentRect is called, we mark all fixed elements in the frame, for layout.

I don't get the commas here :-)

> Source/WebCore/ChangeLog:11
> +        They are added and removed at the same time that they are added and removed from their parent RenderBlock.
> +        The idea is taken from the iOS5.1 branch, at opensource.apple.com.

So you didn't reuse code? Based on opensource code from the iOS port. ?

> Source/WebCore/page/FrameView.cpp:1704
>  {
> +    if (visibleContentRect.size() != this->fixedVisibleContentRect().size()) {

Maybe this deserves a little comment: 

// When the viewport size changes or the content is scaled, we need to
// reposition the fixed positioned elements.

as it is not so obvious that this is called when we scale the content

> Source/WebCore/rendering/RenderBlock.cpp:3423
> +    if (o->style()->position() == FixedPosition) {
> +        if (view())

why not merge those two if's ? if (view() && o->style()->position() == FixedPosition)

> Source/WebCore/rendering/RenderBlock.cpp:3434
> +    if (view())
> +        view()->removeFixedPositionedObject(o);

So this will fail in most cases right.

> Source/WebCore/rendering/RenderView.cpp:923
> +void RenderView::setFixedPositionedObjectsNeedLayout()

need*S* no?

[Yael]

(In reply to comment #2)
Thanks for the review :)
> (From update of attachment 137261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137261&action=review
> 
> > ManualTests/remove-add-fixed-position.html:5
> > +.d1{position:fixed;top:5;right:5;z-index:2;overflow:hidden;}
> > +.o {background:green;height:40px;width:200px;}
> 
> Some spacing here would be nice :-) or even some newlines.
> 
ok
> > Source/WebCore/ChangeLog:8
> > +        When setFixedVisibleContentRect is called, we mark all fixed elements in the frame, for layout.
> 
> I don't get the commas here :-)
> 
removed :)
> > Source/WebCore/ChangeLog:11
> > +        They are added and removed at the same time that they are added and removed from their parent RenderBlock.
> > +        The idea is taken from the iOS5.1 branch, at opensource.apple.com.
> 
> So you didn't reuse code? Based on opensource code from the iOS port. ?
> 
Not everything is there.I needed to fill the blanks.
> > Source/WebCore/page/FrameView.cpp:1704
> >  {
> > +    if (visibleContentRect.size() != this->fixedVisibleContentRect().size()) {
> 
> Maybe this deserves a little comment: 
> 
> // When the viewport size changes or the content is scaled, we need to
> // reposition the fixed positioned elements.
> 
> as it is not so obvious that this is called when we scale the content
> 
ok
> > Source/WebCore/rendering/RenderBlock.cpp:3423
> > +    if (o->style()->position() == FixedPosition) {
> > +        if (view())
> 
> why not merge those two if's ? if (view() && o->style()->position() == FixedPosition)
> 
ok
> > Source/WebCore/rendering/RenderBlock.cpp:3434
> > +    if (view())
> > +        view()->removeFixedPositionedObject(o);
> 
> So this will fail in most cases right.
> 
> > Source/WebCore/rendering/RenderView.cpp:923
> > +void RenderView::setFixedPositionedObjectsNeedLayout()
> 
> need*S* no?
objects need, not needs :)

[Yael]

Created attachment 137332 [details]
Patch for landing.

[WebKit Review Bot]

Comment on attachment 137332 [details]
Patch for landing.

Clearing flags on attachment: 137332

Committed r114249: <http://trac.webkit.org/changeset/114249>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Abhishek Arya]

Comment on attachment 137332 [details]
Patch for landing.

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

> Source/WebCore/rendering/RenderBlock.cpp:3422
> +    if (o->style()->position() == FixedPosition && view())

I don't think this is right and you should definitely ask Dave Hyatt for a review of this patch. Two reasons::
1) 99% of the time, fixed positioned objects are always added to their containing view.
    if (child->isPositioned()) {
        child->containingBlock()->insertPositionedObject(child);
and if you see containingBlock()
if (!isText() && m_style->position() == FixedPosition) {
        while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock()))
            o = o->parent();
we would only return our containing view.
2) There are some exceptions for cases like <foreignObject>. http://trac.webkit.org/changeset/119914

This call is just redundant and forces things to be always added to renderview which is incorrect for cases like <foreignObject>, etc.

[Yael]

(In reply to comment #7)
> (From update of attachment 137332 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137332&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:3422
> > +    if (o->style()->position() == FixedPosition && view())
> 
> I don't think this is right and you should definitely ask Dave Hyatt for a review of this patch. Two reasons::
> 1) 99% of the time, fixed positioned objects are always added to their containing view.
>     if (child->isPositioned()) {
>         child->containingBlock()->insertPositionedObject(child);
> and if you see containingBlock()
> if (!isText() && m_style->position() == FixedPosition) {
>         while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock()))
>             o = o->parent();
> we would only return our containing view.
> 2) There are some exceptions for cases like <foreignObject>. http://trac.webkit.org/changeset/119914
> 
> This call is just redundant and forces things to be always added to renderview which is incorrect for cases like <foreignObject>, etc.
thanks for your comment, I'll take a look :)

[Yael]

(In reply to comment #7)
> (From update of attachment 137332 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137332&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:3422
> > +    if (o->style()->position() == FixedPosition && view())
> 
> I don't think this is right and you should definitely ask Dave Hyatt for a review of this patch. Two reasons::
> 1) 99% of the time, fixed positioned objects are always added to their containing view.
>     if (child->isPositioned()) {
>         child->containingBlock()->insertPositionedObject(child);
> and if you see containingBlock()
> if (!isText() && m_style->position() == FixedPosition) {
>         while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock()))
>             o = o->parent();
> we would only return our containing view.
> 2) There are some exceptions for cases like <foreignObject>. http://trac.webkit.org/changeset/119914
> 
> This call is just redundant and forces things to be always added to renderview which is incorrect for cases like <foreignObject>, etc.

This list is used for quickly identifying all the fixed position elements, so that we can mark them for layout, is that an incorrect way for doing that?
BTW, The same idea is used in http://opensource.apple.com/source/WebCore/WebCore-1298.39/rendering/RenderView.cpp (search for RenderView::setCustomFixedPositionedObjectsNeedLayout).

[Abhishek Arya]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 137332 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137332&action=review
> > 
> > > Source/WebCore/rendering/RenderBlock.cpp:3422
> > > +    if (o->style()->position() == FixedPosition && view())
> > 
> > I don't think this is right and you should definitely ask Dave Hyatt for a review of this patch. Two reasons::
> > 1) 99% of the time, fixed positioned objects are always added to their containing view.
> >     if (child->isPositioned()) {
> >         child->containingBlock()->insertPositionedObject(child);
> > and if you see containingBlock()
> > if (!isText() && m_style->position() == FixedPosition) {
> >         while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock()))
> >             o = o->parent();
> > we would only return our containing view.
> > 2) There are some exceptions for cases like <foreignObject>. http://trac.webkit.org/changeset/119914
> > 
> > This call is just redundant and forces things to be always added to renderview which is incorrect for cases like <foreignObject>, etc.
> 
> This list is used for quickly identifying all the fixed position elements, so that we can mark them for layout, is that an incorrect way for doing that?
> BTW, The same idea is used in http://opensource.apple.com/source/WebCore/WebCore-1298.39/rendering/RenderView.cpp (search for RenderView::setCustomFixedPositionedObjectsNeedLayout).

Fixed position objects are already added to their RenderView in most cases. Why did you need to define insertFixedPositionedObject, removeFixedPositionedObject and call them in insertPositionedObject and removePositionedObject ? That part is wrong. you should see that all callers to insertPositionedObject are like child->containingBlock()->insertPositionedObject and read the containingBlock code

if (!isText() && m_style->position() == FixedPosition) {
        while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) {
#if ENABLE(SVG)
            // foreignObject is the containing block for its contents.
            if (o->isSVGForeignObject())
                break;
#endif
            o = o->parent();
        }
    }

What you are doing here is causing redundant calls which will slow down insertPositionedObject and will cause it to be added in RenderView where it was not intended. e.g. o->hasTransform() && o->isRenderBlock() AND o->isSVGForeignObject()

[Abhishek Arya]

What you are saying about setCustomFixedPositionedObjectsNeedLayout is not relevant here. Basically, you can have setFixedPositionedObjectsNeedLayout function and FrameView::setFixedVisibleContentRect changes, BUT don't need to modify RenderBlock::insertPositionedObject and RenderBlock::removePositionedObject at all and hence don't need insertFixedPositionedObject and removeFixedPositionedObject

[Yael]

Created https://bugs.webkit.org/show_bug.cgi?id=89019 to remove the redundant code.