Bug 36783

Summary: Update of fixed elements is not made correctly when the page has been scrolled
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: Layout and RenderingAssignee: Benjamin Poulain <benjamin>
Status: CLOSED FIXED    
Severity: Major CC: benjamin, commit-queue, hausmann, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
Test case
none
Patch and pixel test
simon.fraser: review-, simon.fraser: commit-queue-
Patch and pixel test
simon.fraser: review-, simon.fraser: commit-queue-
Patch with updatePositions()
benjamin: review-, benjamin: commit-queue-
patch
simon.fraser: review-, simon.fraser: commit-queue-
Patch
none
The patch with the images
simon.fraser: review-
Patch
none
patch
simon.fraser: review+, simon.fraser: commit-queue-
Patch
none
Test case for the strange positioning
none
Fixed inside transformed inside fixed none

Description Benjamin Poulain 2010-03-29 14:37:05 PDT
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.
Comment 1 Benjamin Poulain 2010-03-30 06:55:12 PDT
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.
Comment 2 Simon Fraser (smfr) 2010-04-01 16:59:46 PDT
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.
Comment 3 Simon Fraser (smfr) 2010-04-01 17:16:28 PDT
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.
Comment 4 Benjamin Poulain 2010-04-08 01:35:39 PDT
Simon, any news for this?

I don't mind implementing the update in FrameView::scrollPositionChanged(), if you prefer this approach.
Comment 5 Simon Fraser (smfr) 2010-04-08 07:59:43 PDT
I talked to Dan; we prefer the scrollPositionChanged() approach.
Comment 6 Benjamin Poulain 2010-04-08 08:20:30 PDT
(In reply to comment #5)
> I talked to Dan; we prefer the scrollPositionChanged() approach.

Good for me, I will do that tomorrow morning.
Comment 7 Benjamin Poulain 2010-04-09 10:16:19 PDT
Created attachment 52963 [details]
Patch and pixel test

Here is the alternative patch.
Comment 8 Simon Fraser (smfr) 2010-04-09 10:59:00 PDT
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.
Comment 9 Benjamin Poulain 2010-04-09 11:05:20 PDT
(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.
Comment 10 Benjamin Poulain 2010-04-09 11:31:30 PDT
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 :)
Comment 11 Eric Seidel (no email) 2010-04-09 13:45:08 PDT
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.
Comment 12 Simon Fraser (smfr) 2010-04-09 13:47:26 PDT
Comment on attachment 52970 [details]
Patch with updatePositions()

I'm not seeing the RenderLayer.cpp changes in this patch.
Comment 13 Benjamin Poulain 2010-04-09 15:14:59 PDT
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.
Comment 14 Benjamin Poulain 2010-04-12 20:41:57 PDT
Created attachment 53217 [details]
patch
Comment 15 Simon Fraser (smfr) 2010-04-13 09:49:12 PDT
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.
Comment 16 Benjamin Poulain 2010-04-19 11:43:22 PDT
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.
Comment 17 Benjamin Poulain 2010-04-19 12:07:24 PDT
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.
Comment 18 Benjamin Poulain 2010-04-19 12:09:11 PDT
Created attachment 53701 [details]
The patch with the images
Comment 19 Simon Fraser (smfr) 2010-04-19 13:23:40 PDT
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

> +2010-04-19  Benjamin Poulain  <ikipou@gmail.com>
> +
> +        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.
Comment 20 Benjamin Poulain 2010-04-19 13:31:19 PDT
(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.
Comment 21 Benjamin Poulain 2010-04-19 16:52:31 PDT
Created attachment 53739 [details]
Patch
Comment 22 Benjamin Poulain 2010-04-19 16:53:22 PDT
Comment on attachment 53739 [details]
Patch

I forgot to update the image of the last test.
Comment 23 Benjamin Poulain 2010-04-19 16:59:35 PDT
Created attachment 53741 [details]
patch
Comment 24 Simon Fraser (smfr) 2010-04-19 17:24:55 PDT
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 @@
> +2010-04-19  Benjamin Poulain  <ikipou@gmail.com>
> +
> +        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.
Comment 25 Benjamin Poulain 2010-04-19 18:17:40 PDT
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.
Comment 26 Benjamin Poulain 2010-04-19 18:21:47 PDT
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.
Comment 27 Simon Fraser (smfr) 2010-04-19 18:37:41 PDT
Created attachment 53759 [details]
Fixed inside transformed inside fixed

Here's another illustration of the latter problem. Please file a new bug on this.
Comment 28 WebKit Commit Bot 2010-04-21 05:57:22 PDT
Comment on attachment 53755 [details]
Patch

Clearing flags on attachment: 53755

Committed r57971: <http://trac.webkit.org/changeset/57971>
Comment 29 WebKit Commit Bot 2010-04-21 05:57:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Simon Hausmann 2010-04-26 03:20:37 PDT
Revision r57971 cherry-picked into qtwebkit-2.0 with commit 0079f8cce5502c9806d2c171afefa3517c7323ec