Bug 36783 – Update of fixed elements is not made correctly when the page has been scrolled

• Status: CLOSED
• Resolution: FIXED
• Component: Layout and Rendering

[Benjamin Poulain]

Created attachment 51966 [details]
Test case

When moving a fixed element, the area at the previous position is not updated correctly when the page has been scrolled.

It looks like the update rect of the previous position is computed without taking into account the scrolling.

Test case attached, it can be reproduced with Safari 4.0.4.

[Benjamin Poulain]

Created attachment 52031 [details]
Patch and pixel test

A patch with a new test case with descriptive text.

The problem comes from the repaint rect of RenderLayer which is still in the coordinates of whatever position the viewport was for the last layout.

[Simon Fraser (smfr)]

Comment on attachment 52031 [details]
Patch and pixel test

I think the actual bug here is that scrolling doesn't invalidate the RenderLayer's cached repaint rect.

[Simon Fraser (smfr)]

I'd like to talk to Mitz when he gets back. We could either have FrameView::scrollPositionChanged() do an updateWidgetPositions() pass, or do something like this patch.

[Benjamin Poulain]

Simon, any news for this?

I don't mind implementing the update in FrameView::scrollPositionChanged(), if you prefer this approach.

[Simon Fraser (smfr)]

I talked to Dan; we prefer the scrollPositionChanged() approach.

[Benjamin Poulain]

(In reply to comment #5)
> I talked to Dan; we prefer the scrollPositionChanged() approach.

Good for me, I will do that tomorrow morning.

[Benjamin Poulain]

Created attachment 52963 [details]
Patch and pixel test

Here is the alternative patch.

[Simon Fraser (smfr)]

Comment on attachment 52963 [details]
Patch and pixel test


> +void RenderLayer::updateRepaintRectPosition()
> +{
> +    RenderBox* box = renderBox();
> +    FloatPoint absPos = box->localToAbsolute();
> +    absPos.move(box->borderLeft(), box->borderTop());
> +    m_repaintRect.setLocation(IntPoint(absPos.x(), absPos.y()));
> +}

This isn't correct for fixed inside of transformed elements, and some other configurations.
It should just be removed in favor of computeRepaintRects().

> +void RenderView::updateFixedElementPositions()
> +{
> +    ListHashSet<RenderBox*>::const_iterator end = positionedObjects()->end();
> +    for (ListHashSet<RenderBox*>::const_iterator it = positionedObjects()->begin(); it != end; ++it) {
> +        RenderBox* renderBox = *it;
> +        if (renderBox->style()->position() != FixedPosition)
> +            continue;
> +        renderBox->layer()->updateRepaintRectPosition();
> +    }
> +}

You also need to hit RenderLayers with a fixed ancestor. It may be better to do something similar to what updateLayerPositions() does, or add a new flag to updateLayerPositions() and call that.

[Benjamin Poulain]

(In reply to comment #8)
> (From update of attachment 52963 [details])
> 
> > +void RenderLayer::updateRepaintRectPosition()
> > +{
> > +    RenderBox* box = renderBox();
> > +    FloatPoint absPos = box->localToAbsolute();
> > +    absPos.move(box->borderLeft(), box->borderTop());
> > +    m_repaintRect.setLocation(IntPoint(absPos.x(), absPos.y()));
> > +}
> 
> This isn't correct for fixed inside of transformed elements, and some other
> configurations.
> It should just be removed in favor of computeRepaintRects().

I thought the fixed element inside a transformed layer would not be in the list returned by positionedObjects() on RenderView.
My understanding was that they are in the positionedObjects() list of the parent layer (the one with the transformation)?


> You also need to hit RenderLayers with a fixed ancestor.

Right, of course, I need to update the child layers.

[Benjamin Poulain]

Created attachment 52970 [details]
Patch with updatePositions()

> You also need to hit RenderLayers with a fixed ancestor. It may be better to do
> something similar to what updateLayerPositions() does, or add a new flag to
> updateLayerPositions() and call that.

updateLayerPositions() should do the job. I thought it would be a bit overkill but lets wait until it is a problem before trying to optimize :)

[Eric Seidel (no email)]

Comment on attachment 52970 [details]
Patch with updatePositions()

This patch was attached with the wrong mime type and missing the "patch" flag.  You should consider using one of our patch attaching tools like "webkit-patch upload" for uploading your patches, as they take care of all these details for you.

See "WebKitTools/Scripts/webkit-patch help" for more information.

[Simon Fraser (smfr)]

Comment on attachment 52970 [details]
Patch with updatePositions()

I'm not seeing the RenderLayer.cpp changes in this patch.

[Benjamin Poulain]

Comment on attachment 52970 [details]
Patch with updatePositions()

(In reply to comment #11)
> (From update of attachment 52970 [details])
> This patch was attached with the wrong mime type and missing the "patch" flag. 
> You should consider using one of our patch attaching tools like "webkit-patch
> upload" for uploading your patches, as they take care of all these details for
> you.
> 
> See "WebKitTools/Scripts/webkit-patch help" for more information.

Sorry, I was in a hurry. I will have a look at webkit-patch, thanks for the info.


> (From update of attachment 52970 [details])
> I'm not seeing the RenderLayer.cpp changes in this patch.

Damn, I forgot to add the change to the commit. I will try to update it this weekend.

[Benjamin Poulain]

Created attachment 53217 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 53217 [details]
patch

> diff --git a/WebCore/page/FrameView.cpp b/WebCore/page/FrameView.cpp
> index 4b6cc36..badca3b 100644
> --- a/WebCore/page/FrameView.cpp
> +++ b/WebCore/page/FrameView.cpp
> @@ -1011,6 +1011,7 @@ void FrameView::scrollPositionChanged()
>      if (!m_nestedLayoutCount && hasFixedObjects()) {
>          if (RenderView* root = m_frame->contentRenderer()) {
>              root->updateWidgetPositions();
> +            root->updateFixedElementPositions();
>  #if USE(ACCELERATED_COMPOSITING)
>              if (root->usesCompositing())
>                  root->compositor()->updateCompositingLayers(CompositingUpdateOnScroll);

Calling updateFixedElementPositions() here may in fact obsolete the need to call updateCompositingLayers().

> diff --git a/WebCore/rendering/RenderView.cpp b/WebCore/rendering/RenderView.cpp

> +void RenderView::updateFixedElementPositions()

The method name doesn't accurately reflect what the method does. It's fixing up cached repaint rects, not updating element positions.

Maybe updateLayersAfterScroll().

> +{
> +    ListHashSet<RenderBox*>::const_iterator end = positionedObjects()->end();
> +    for (ListHashSet<RenderBox*>::const_iterator it = positionedObjects()->begin(); it != end; ++it) {
> +        RenderBox* renderBox = *it;
> +        if (renderBox->style()->position() != FixedPosition)
> +            continue;
> +        renderBox->layer()->updateLayerPositions();

You should look at the UpdateLayerPositionsFlags argument to updateLayerPositions(), and use the appropriate flags. For example, I don't think you need the 'full repaint' flag.

You may also do redundant work here if there are nested fixed position elements.

I'd like to see a final version of this patch.

[Benjamin Poulain]

Sorry for the delay, a ash cloud is preventing real work from happening :)

> >      if (!m_nestedLayoutCount && hasFixedObjects()) {
> >          if (RenderView* root = m_frame->contentRenderer()) {
> >              root->updateWidgetPositions();
> > +            root->updateFixedElementPositions();
> >  #if USE(ACCELERATED_COMPOSITING)
> >              if (root->usesCompositing())
> >                  root->compositor()->updateCompositingLayers(CompositingUpdateOnScroll);
> 
> Calling updateFixedElementPositions() here may in fact obsolete the need to
> call updateCompositingLayers().

I think you are right. I am not familiar with GraphicsLayer but I don't see any reason why updatePositions would not work.

> 
> > diff --git a/WebCore/rendering/RenderView.cpp b/WebCore/rendering/RenderView.cpp
> 
> > +void RenderView::updateFixedElementPositions()
> 
> The method name doesn't accurately reflect what the method does. It's fixing up
> cached repaint rects, not updating element positions.
> 
> Maybe updateLayersAfterScroll().
> 
> > +{
> > +    ListHashSet<RenderBox*>::const_iterator end = positionedObjects()->end();
> > +    for (ListHashSet<RenderBox*>::const_iterator it = positionedObjects()->begin(); it != end; ++it) {
> > +        RenderBox* renderBox = *it;
> > +        if (renderBox->style()->position() != FixedPosition)
> > +            continue;
> > +        renderBox->layer()->updateLayerPositions();
> 
> You should look at the UpdateLayerPositionsFlags argument to
> updateLayerPositions(), and use the appropriate flags. For example, I don't
> think you need the 'full repaint' flag.
> 
> You may also do redundant work here if there are nested fixed position
> elements.

You are right, a fixed child of a fixed element would be processed twice when in the same containingBlock. I will look for a way to process the layer tree only once.

[Benjamin Poulain]

Created attachment 53700 [details]
Patch

To avoid the duplicated work on update, I added the flag "UpdateFixedSubtreeOnScroll" to updatePositions(). When this flag is set, updatePositions only work for fixed subtree.

Don't be afraid by the size of the patch, the code is very small. I have made new tests to improve the coverage of the different path.

My main concern is about GraphicsLayer.

[Benjamin Poulain]

Created attachment 53701 [details]
The patch with the images

[Simon Fraser (smfr)]

Comment on attachment 53701 [details]
The patch with the images

> +      <p style="position: absolute; top: 210px">You should see a yellow rect on the left, and a red one on the right. No green pixels.</p>

You should change the colors so that red pixels indicate failure. Green is good, red is bad.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> [phone]  Benjamin Poulain  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Update of fixed elements is not made correctly when the page has been scrolled
> +        https://bugs.webkit.org/show_bug.cgi?id=36783

This changelog entry needs to describe what the problem was, and how these changes tbat you are making fix the bug.

> +        * page/FrameView.cpp:
> +        (WebCore::FrameView::scrollPositionChanged):
> +        * rendering/RenderLayer.cpp:
> +        (WebCore::RenderLayer::updateLayerPositions):
> +        * rendering/RenderLayer.h:
> +        (WebCore::RenderLayer::):
> +        * rendering/RenderLayerCompositor.cpp:
> +        (WebCore::RenderLayerCompositor::updateCompositingLayers):
> +        * rendering/RenderLayerCompositor.h:
> +        (WebCore::):

A good changelog entry will also annotate each of the file changes.

> diff --git a/WebCore/page/FrameView.cpp b/WebCore/page/FrameView.cpp
> index 4b6cc36..60d0ce9 100644
> --- a/WebCore/page/FrameView.cpp
> +++ b/WebCore/page/FrameView.cpp
> @@ -1011,10 +1011,7 @@ void FrameView::scrollPositionChanged()
>      if (!m_nestedLayoutCount && hasFixedObjects()) {
>          if (RenderView* root = m_frame->contentRenderer()) {
>              root->updateWidgetPositions();
> -#if USE(ACCELERATED_COMPOSITING)
> -            if (root->usesCompositing())
> -                root->compositor()->updateCompositingLayers(CompositingUpdateOnScroll);
> -#endif
> +            root->layer()->updateLayerPositions(RenderLayer::UpdateCompositingLayers | RenderLayer::UpdateFixedSubtreeOnScroll);
>          }

The updateCompositingLayers() is still required.We may need to re-analyze the compositing hierarchy on scrolling, and updateLayerPositions() doesn't do this.

> diff --git a/WebCore/rendering/RenderLayer.cpp b/WebCore/rendering/RenderLayer.cpp
> index 2d868bb..aff19df 100644
> --- a/WebCore/rendering/RenderLayer.cpp
> +++ b/WebCore/rendering/RenderLayer.cpp
> @@ -247,6 +247,20 @@ bool RenderLayer::hasAcceleratedCompositing() const
>  
>  void RenderLayer::updateLayerPositions(UpdateLayerPositionsFlags flags, IntPoint* cachedOffset)
>  {
> +    if (flags & UpdateFixedSubtreeOnScroll) {
> +        if (renderer()->style()->position() == FixedPosition)
> +            flags &= ~UpdateFixedSubtreeOnScroll;
> +        else if (hasTransform()) {
> +            // transformation is a referencial for fixed child layers
> +            // no need to update them
> +            return;
> +        } else {
> +            for (RenderLayer* child = firstChild(); child; child = child->nextSibling())
> +                child->updateLayerPositions(flags, cachedOffset);
> +            return;
> +        }
> +    }

I don't really like how UpdateFixedSubtreeOnScroll causes early returns here. It should work for this flag to be used in combination with the other flags, but this code doesn't work that way.

I'm leaning towards having a new method just to update fixed layers. Sorry to not be more specific earlier.


> diff --git a/WebCore/rendering/RenderLayerCompositor.cpp b/WebCore/rendering/RenderLayerCompositor.cpp
> index 9430613..a8cf221 100644
> --- a/WebCore/rendering/RenderLayerCompositor.cpp
> +++ b/WebCore/rendering/RenderLayerCompositor.cpp
> @@ -165,12 +165,6 @@ void RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
>      case CompositingUpdateOnPaitingOrHitTest:
>          checkForHierarchyUpdate = true;
>          break;
> -    case CompositingUpdateOnScroll:
> -        if (m_compositingConsultsOverlap)
> -            checkForHierarchyUpdate = true; // Overlap can change with scrolling, so need to check for hierarchy updates.
> -
> -        needGeometryUpdate = true;
> -        break;
>      }
>  
>      if (!checkForHierarchyUpdate && !needGeometryUpdate)
> diff --git a/WebCore/rendering/RenderLayerCompositor.h b/WebCore/rendering/RenderLayerCompositor.h
> index 4dd8712..173062c 100644
> --- a/WebCore/rendering/RenderLayerCompositor.h
> +++ b/WebCore/rendering/RenderLayerCompositor.h
> @@ -41,7 +41,6 @@ class RenderVideo;
>  enum CompositingUpdateType {
>      CompositingUpdateAfterLayoutOrStyleChange,
>      CompositingUpdateOnPaitingOrHitTest,
> -    CompositingUpdateOnScroll
>  };
>  
>  // RenderLayerCompositor manages the hierarchy of

You should revert these changes.

[Benjamin Poulain]

(In reply to comment #19)
> (From update of attachment 53701 [details])
> > +      <p style="position: absolute; top: 210px">You should see a yellow rect on the left, and a red one on the right. No green pixels.</p>
> 
> You should change the colors so that red pixels indicate failure. Green is
> good, red is bad.

Good point, I will change that.

> The updateCompositingLayers() is still required.We may need to re-analyze the
> compositing hierarchy on scrolling, and updateLayerPositions() doesn't do this.

I trust you on that one. I am not familiar with that code.


> I don't really like how UpdateFixedSubtreeOnScroll causes early returns here.
> It should work for this flag to be used in combination with the other flags,
> but this code doesn't work that way.
> 
> I'm leaning towards having a new method just to update fixed layers. Sorry to
> not be more specific earlier.

No problem, back to the first patch then :)
I also like better to have a separate method.

[Benjamin Poulain]

Created attachment 53739 [details]
Patch

[Benjamin Poulain]

Comment on attachment 53739 [details]
Patch

I forgot to update the image of the last test.

[Benjamin Poulain]

Created attachment 53741 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 53741 [details]
patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 4e75c14..79a3bd6 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,24 @@
> [phone]  Benjamin Poulain  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Update of fixed elements is not made correctly when the page has been scrolled
> +        https://bugs.webkit.org/show_bug.cgi?id=36783
> +
> +        Add the method RenderLayer::updateRepaintRectsAfterScroll() to update
> +        the repaint rect of all fixed tree after scroll.

This doesn't clearly state what the problem was (stale cached on RenderLayers), and what
you did to fix it.

> diff --git a/WebCore/rendering/RenderLayer.cpp b/WebCore/rendering/RenderLayer.cpp
> index f6c3d80..c8a398e 100644
> --- a/WebCore/rendering/RenderLayer.cpp
> +++ b/WebCore/rendering/RenderLayer.cpp
> @@ -373,6 +373,18 @@ void RenderLayer::computeRepaintRects()
>      m_outlineBox = renderer()->outlineBoundsForRepaint(repaintContainer);
>  }
>  
> +void RenderLayer::updateRepaintRectsAfterScroll(bool fixed)
> +{
> +    if (fixed || renderer()->style()->position() == FixedPosition) {
> +        computeRepaintRects();
> +        fixed = true;
> +    } else if (renderer()->hasTransform())
> +        return;

I would like to see a comment here. Something like
 // Transforms act as fixed position containers, so nothing inside a transformed element can be fixed relative to the viewport.

But now I think about this, what happens with:

<div style="position: fixed>
  <div style="-webkit-transform: translate(10px, 10px)">
    <div style="position: fixed>
    </div>
  </div>
</div>
?
r=me if that last testcase works.

[Benjamin Poulain]

Created attachment 53755 [details]
Patch

Patch update with the comments of Simon.

> But now I think about this, what happens with:
> 
> <div style="position: fixed>
>   <div style="-webkit-transform: translate(10px, 10px)">
>     <div style="position: fixed>
>     </div>
>   </div>
> </div>
> ?

This case works since the whole subtree of a fixed element is updated.


However, I am surprised by the result. For me, a transformed child of a fixed element is not special, it should be fixed in the viewport. Currently, it is not, it act as if it has absolute positioning, bypassing the parent's fixed position.
I think the behavior is not correct, I will open a other bug for that case if you agree this is not correct.

[Benjamin Poulain]

Created attachment 53756 [details]
Test case for the strange positioning

This illustrate the problem I mention in my last comment. This is a layout behavior, it is not related to this change.

If that behavior is correct, I can update the patch to skip all branches that include a transformation.

[Simon Fraser (smfr)]

Created attachment 53759 [details]
Fixed inside transformed inside fixed

Here's another illustration of the latter problem. Please file a new bug on this.

[WebKit Commit Bot]

Comment on attachment 53755 [details]
Patch

Clearing flags on attachment: 53755

Committed r57971: <http://trac.webkit.org/changeset/57971>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r57971 cherry-picked into qtwebkit-2.0 with commit 0079f8cce5502c9806d2c171afefa3517c7323ec