Bug 83720 – [Qt][WK2] Nested fixed elements scroll too fast

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yael]

After http://trac.webkit.org/changeset/113791, the position of fixed elements is adjusted during scrolling to compensate for the difference between the scroll offset in the web process and the ui process.
Nested fixed elements are adjusted, but so are their ancestors, resulting in the delta being applied twice.

A patch is coming soon.

[Yael]

Created attachment 136766 [details]
Patch.

Before setting the scrollPositionDelta to a fixed layer, check if it has an ancestor which also has fixed position. If it does, do not set scrollPositionDelta.
Added a flag to TextureMapperLayer indicating if it is a fixed position layer. The flag is set in WebLayerTreeRenderer::flushLayerChanges to avoid adding it also to GraphicsLayerTextureMapper.
If reviewers think it is better to add it to GraphicsLayerTextureMapper, I can do that.

[Yael]

I know some people do not like to add fixed position flags to GraphicsLayer sub classes. Please note that the Blackberry port does that too :)

[Noam Rosenthal]

Comment on attachment 136766 [details]
Patch.

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:525
> +    TextureMapperLayer* parent = m_parent;
> +    while (parent && !parent->m_fixedToViewport)
> +        parent = parent->m_parent;
> +
> +    if (parent)
> +        return;

Strange traversal...
How about a function isAncestorFixedToViewport() that returns true/false, and this function calling it?

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:364
> +    if (m_fixedLayers.isEmpty())
> +        return;
> +
> +    LayerMap::iterator end = m_fixedLayers.end();
> +    for (LayerMap::iterator it = m_fixedLayers.begin(); it != end; ++it)
> +        toTextureMapperLayer(it->second)->setFixedToViewport(true);

This seems like a different bug/fix?

[Yael]

(In reply to comment #3)
Thanks for reviewing :)
> (From update of attachment 136766 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136766&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:525
> > +    TextureMapperLayer* parent = m_parent;
> > +    while (parent && !parent->m_fixedToViewport)
> > +        parent = parent->m_parent;
> > +
> > +    if (parent)
> > +        return;
> 
> Strange traversal...
> How about a function isAncestorFixedToViewport() that returns true/false, and this function calling it?
> 
ok

> > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:364
> > +    if (m_fixedLayers.isEmpty())
> > +        return;
> > +
> > +    LayerMap::iterator end = m_fixedLayers.end();
> > +    for (LayerMap::iterator it = m_fixedLayers.begin(); it != end; ++it)
> > +        toTextureMapperLayer(it->second)->setFixedToViewport(true);
> 
> This seems like a different bug/fix?
No, it is for this bug :) This is for setting the flag on TextureMapperLayer. Please see comment #1. If you prefer that I set it via GraphicsLayerTextureMapper, please let me know.

[Noam Rosenthal]

Comment on attachment 136766 [details]
Patch.

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

>>> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:364
>>> +        toTextureMapperLayer(it->second)->setFixedToViewport(true);
>> 
>> This seems like a different bug/fix?
> 
> No, it is for this bug :) This is for setting the flag on TextureMapperLayer. Please see comment #1. If you prefer that I set it via GraphicsLayerTextureMapper, please let me know.

I don't mind adding it in GraphicsLayerTextureMapper, since it would make the sync code easier to read.

[Yael]

Created attachment 136809 [details]
Patch.

Cleanup the initial patch as suggested in comment #3 and #5.

[Noam Rosenthal]

Comment on attachment 136809 [details]
Patch.

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

> ChangeLog:10
> +        * ManualTests/fixed-position.html: Added a test for nested fixed position elements
> +        and a way to add/remove the fixed parent of a fixed position element.
> +

I'd rather we add this as an additional manual test.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:517
> +    for (TextureMapperLayer* parent = m_parent;parent;parent = parent->m_parent) {

Spacing?

[Yael]

Created attachment 136881 [details]
Patch.

Split out the manual test from the existing test. And added missing spaces.

[Yael]

Thanks for the review :)

[WebKit Review Bot]

Comment on attachment 136881 [details]
Patch.

Clearing flags on attachment: 136881

Committed r113983: <http://trac.webkit.org/changeset/113983>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.