Bug 113932 – [CoordinatedGraphics] CSS fixed elements are shaking while scrolling

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[Mikhail Pozdnyakov]

SSIA. It's a regression actually of r147163 when Scrolling Coordinator interface was modified.

[Mikhail Pozdnyakov]

Created attachment 196472 [details]
patch

[Noam Rosenthal]

Comment on attachment 196472 [details]
patch

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

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:52
> +    do {
> +        if (layer->renderer()->style()->position() == FixedPosition)
> +            return true;
> +
> +        layer = layer->parent();
> +    } while (layer && !layer->isComposited()); // Look between a composited layer and its nearest composited ancestor

did you mean RenderLayer::enclosingCompositingLayer()?

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:62
> +    ASSERT(layer->isComposited());

If a layer is no longer fixed wouldn't it become un-composited?

[Mikhail Pozdnyakov]

Comment on attachment 196472 [details]
patch

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

>> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:52
>> +    } while (layer && !layer->isComposited()); // Look between a composited layer and its nearest composited ancestor
> 
> did you mean RenderLayer::enclosingCompositingLayer()?

I wouldn't say so, traversal algorithm is similar but in this case we need also check whether layer position is fixed.

>> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:62
>> +    ASSERT(layer->isComposited());
> 
> If a layer is no longer fixed wouldn't it become un-composited?

This method can only be invoked for composited layers (those having backing).
I could substitute it with ASSERT(layer->backing()), would you like it?

[Mikhail Pozdnyakov]

Created attachment 196631 [details]
patch

There is no need actually to traverse uncomposited parent layers for being fixed, as we have acceleratedCompositingForFixedPositionEnabled set providing constrained layers to be composited.

[Mikhail Pozdnyakov]

(In reply to comment #4)
> Created an attachment (id=196631) [details]
> patch
> 
> There is no need actually to traverse uncomposited parent layers for being fixed, as we have acceleratedCompositingForFixedPositionEnabled set providing constrained layers to be composited.

I mean traverse and check for being fixed.

[Noam Rosenthal]

Comment on attachment 196631 [details]
patch

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

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:50
> +    // This method can only be invoked for composited layers (those having backing).

(those having backing) is redundant :)

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:56
> +        toCoordinatedGraphicsLayer(backing->childForSuperlayers())->setFixedToViewport(true);

why childForSuperlayers instead of graphicsLayer() ?

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:59
> +        if (backing->hasAncestorClippingLayer())
> +            toCoordinatedGraphicsLayer(backing->ancestorClippingLayer())->setFixedToViewport(false);

What does this do?

[Mikhail Pozdnyakov]

(In reply to comment #6)
> (From update of attachment 196631 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196631&action=review
> 
> > Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:50
> > +    // This method can only be invoked for composited layers (those having backing).
> 
> (those having backing) is redundant :)
> 
> > Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:56
> > +        toCoordinatedGraphicsLayer(backing->childForSuperlayers())->setFixedToViewport(true);
> 
> why childForSuperlayers instead of graphicsLayer() ?
FOA, this is actually what had been already in RenderLayerBacking::registerScrollingLayers() previously (before r147163)
and now the similar functionality is in coordinator for chromium.

As far as I understand childForSuperlayers would return a layer to apply
any transform and which should actually be set as fixed on coord graphics side. It can be either m_ancestorClippingLayer (we are clipped by an ancestor which is not a stacking context) or m_contentsContainmentLayer (we need a compositing layer to render its background into a separate GraphicsLayer (e.g. for background-attachment: fixed in some scenarios) ) or m_graphicsLayer
itself otherwise.

> 
> > Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:59
> > +        if (backing->hasAncestorClippingLayer())
> > +            toCoordinatedGraphicsLayer(backing->ancestorClippingLayer())->setFixedToViewport(false);
> 
> What does this do?

here we actually have to reset all the layers that childForSuperlayers could  return. (And apparently I forgot to reset contentsContainmentLayer)

[Mikhail Pozdnyakov]

*** This bug has been marked as a duplicate of bug 114353 ***