Bug 107426 – [CoordinatedGraphics] CSS sticky elements are blinking while scrolling

• Component: WebKit2
• Status: ASSIGNED

[Mikhail Pozdnyakov]

CSS sticky elements are blinking while scrolling

[Mikhail Pozdnyakov]

Created attachment 183988 [details]
patch

[Noam Rosenthal]

Comment on attachment 183988 [details]
patch

The current fixed-position code in CoordinatedLayerTreeHost is hacky and unmaintainable. I would rather we fix it before we add more stuff on top.

[Noam Rosenthal]

To be more constructive :)
We need to switch to using scrolling coordinator, and not have our own home-brewed scrolling coordinator hidden inside CoordinatedLayerTreeHost.
That code was there prior to ScrollingCoordinator, which made it passable at the time, but I don't want to continue down that route.

[Mikhail Pozdnyakov]

(In reply to comment #3)
> To be more constructive :)
> We need to switch to using scrolling coordinator, and not have our own home-brewed scrolling coordinator hidden inside CoordinatedLayerTreeHost.
> That code was there prior to ScrollingCoordinator, which made it passable at the time, but I don't want to continue down that route.

I completely agree that the current approach looks hackish and we must switch to scrolling coordinator. I would enjoy taking part in this switch or start it (if the work has not been started yet). 

However, the given patch is tiny (4 new lines) and actually does not bring much on top of the existing code but it could significantly improve the sticky elements behaviour in the interim. What do you think?

[Kenneth Rohde Christiansen]

Given the size of the change, I think it is fine for now, but we do seriously need to these root issues at some point soon.

Could we change your mind Noam?

[Noam Rosenthal]

Comment on attachment 183988 [details]
patch

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

OK, but I have some questions.
How does this work while you scroll? Does the position change?

> Source/WebKit2/ChangeLog:9
> +        Set scroll position delta for sticky elements if the they are
> +        located out of their in-flow layout position (when they have actually stuck). 

Please add

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:478
> +    if (renderer->isInFlowPositioned() && position == StickyPosition) { // Sticky element

The comment is unnecessary.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:479
> +        // Set FixedToViewport 'true' if sticky element is out of its in-flow position; set 'false' otherwise.

Comment doesn't add any information :)

[Noam Rosenthal]

> Please add
Ignore this, I was going to put a comment here but changed my mind :)

[Mikhail Pozdnyakov]

(In reply to comment #6)
> (From update of attachment 183988 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183988&action=review
> 
> OK, but I have some questions.
> How does this work while you scroll? Does the position change?

I have tested scrolling with many web sites having sticky elements and added new manual test also. Scrolling looks quite OK.

The position changes if offsetForInFlowPosition is '0' (meaning sticky elements are scrolling as well as the other contents of the page) and becomes fixed otherwise.

[Mikhail Pozdnyakov]

Created attachment 184434 [details]
patch v2

Removed unneeded comments.

[Kenneth Rohde Christiansen]

Should still be rubberstamped by a WebKit2 owner

[Benjamin Poulain]

Comment on attachment 184434 [details]
patch v2

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

I suggest asking Simon for a review.

> Source/WebKit2/ChangeLog:14
> +        Set scroll position delta for sticky elements if the they are
> +        located out of their in-flow layout position (when they have actually stuck). 
> +
> +        * UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:
> +        (WebKit::LayerTreeRenderer::setLayerState):
> +        * WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:
> +        (WebKit::updateOffsetFromViewportForSelf):

This does not explain the patch in the slightest.

You should give some context about why the bug (CSS sticky elements are blinking while scrolling) was happening. This helps the reader evaluate the fix.

[Mikhail Pozdnyakov]

Created attachment 184498 [details]
patch v3

Improved change log description.

[Simon Fraser (smfr)]

Comment on attachment 184498 [details]
patch v3

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:479
> +    if (renderer->isInFlowPositioned() && position == StickyPosition) {
> +        CoordinatedGraphicsLayer* graphicsLayer = toCoordinatedGraphicsLayer(backing->graphicsLayer());
> +        graphicsLayer->setFixedToViewport(!renderLayer->offsetForInFlowPosition().isZero());
>          return;

You need to keep track of much more information to do correct sticky repositioning on scrolling (see the ViewportConstraints classes). Sticky elements are not just fixed to the viewport; they are constrained by their container too.

[Mikhail Pozdnyakov]

(In reply to comment #13)
> (From update of attachment 184498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184498&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:479
> > +    if (renderer->isInFlowPositioned() && position == StickyPosition) {
> > +        CoordinatedGraphicsLayer* graphicsLayer = toCoordinatedGraphicsLayer(backing->graphicsLayer());
> > +        graphicsLayer->setFixedToViewport(!renderLayer->offsetForInFlowPosition().isZero());
> >          return;
> 
> You need to keep track of much more information to do correct sticky repositioning on scrolling (see the ViewportConstraints classes). Sticky elements are not just fixed to the viewport; they are constrained by their container too.

Agree, and I guess same applies to fixed elements. I think switch to using scrolling coordinator will help to do the things right (comment #4).

[Noam Rosenthal]

Do we really need to do this "temporary change that is going to help only for particular cases and only until we change the architecture to use scrolling coordinator" in trunk, or can you guys do it at your product branch if it's urgent?

[Noam Rosenthal]

Comment on attachment 184498 [details]
patch v3

This is a hack that belongs in product branch. I don't see the point in doing this in trunk.

[Kvet]

I faced the same bug. Will it be fixed?