Bug 36783

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 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.

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.

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

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

(In reply to comment #5) > I talked to Dan; we prefer the scrollPositionChanged() approach. Good for me, I will do that tomorrow morning.

Created attachment 52963 [details] Patch and pixel test Here is the alternative patch.

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.

(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.

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 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 on attachment 52970 [details] Patch with updatePositions() I'm not seeing the RenderLayer.cpp changes in this patch.

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.

Created attachment 53217 [details] patch

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.

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.

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.

Created attachment 53701 [details] The patch with the images

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.

(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.

Created attachment 53739 [details] Patch

Comment on attachment 53739 [details] Patch I forgot to update the image of the last test.

Created attachment 53741 [details] patch

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.

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.

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.

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 on attachment 53755 [details] Patch Clearing flags on attachment: 53755 Committed r57971: <http://trac.webkit.org/changeset/57971>

All reviewed patches have been landed. Closing bug.

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