Bug 83981 - [Qt][WK2] Fixed elements position is wrong after zooming.
Summary: [Qt][WK2] Fixed elements position is wrong after zooming.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-14 10:22 PDT by Yael
Modified: 2012-06-13 10:36 PDT (History)
9 users (show)

See Also:


Attachments
Patch. (9.02 KB, patch)
2012-04-15 18:32 PDT, Yael
kenneth: review+
Details | Formatted Diff | Diff
Patch for landing. (9.17 KB, patch)
2012-04-16 06:23 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2012-04-14 10:22:06 PDT
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.
Comment 1 Yael 2012-04-15 18:32:07 PDT
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.
Comment 2 Kenneth Rohde Christiansen 2012-04-16 04:00:16 PDT
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?
Comment 3 Yael 2012-04-16 06:17:33 PDT
(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 :)
Comment 4 Yael 2012-04-16 06:23:06 PDT
Created attachment 137332 [details]
Patch for landing.
Comment 5 WebKit Review Bot 2012-04-16 07:05:58 PDT
Comment on attachment 137332 [details]
Patch for landing.

Clearing flags on attachment: 137332

Committed r114249: <http://trac.webkit.org/changeset/114249>
Comment 6 WebKit Review Bot 2012-04-16 07:06:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Abhishek Arya 2012-06-11 00:03:45 PDT
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.
Comment 8 Yael 2012-06-11 05:45:08 PDT
(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 :)
Comment 9 Yael 2012-06-12 18:15:45 PDT
(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).
Comment 10 Abhishek Arya 2012-06-12 19:19:51 PDT
(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()
Comment 11 Abhishek Arya 2012-06-12 19:24:05 PDT
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
Comment 12 Yael 2012-06-13 10:36:47 PDT
Created https://bugs.webkit.org/show_bug.cgi?id=89019 to remove the redundant code.