Bug 83720 - [Qt][WK2] Nested fixed elements scroll too fast
Summary: [Qt][WK2] Nested fixed elements scroll too fast
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-11 14:44 PDT by Yael
Modified: 2012-04-12 08:35 PDT (History)
2 users (show)

See Also:


Attachments
Patch. (7.57 KB, patch)
2012-04-11 15:22 PDT, Yael
noam: review-
Details | Formatted Diff | Diff
Patch. (10.30 KB, patch)
2012-04-11 19:22 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (10.44 KB, patch)
2012-04-12 05:18 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2012-04-11 14:44:39 PDT
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.
Comment 1 Yael 2012-04-11 15:22:09 PDT
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.
Comment 2 Yael 2012-04-11 15:35:19 PDT
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 :)
Comment 3 Noam Rosenthal 2012-04-11 15:42:48 PDT
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?
Comment 4 Yael 2012-04-11 15:49:38 PDT
(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.
Comment 5 Noam Rosenthal 2012-04-11 15:59:15 PDT
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.
Comment 6 Yael 2012-04-11 19:22:46 PDT
Created attachment 136809 [details]
Patch.

Cleanup the initial patch as suggested in comment #3 and #5.
Comment 7 Noam Rosenthal 2012-04-11 19:27:02 PDT
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?
Comment 8 Yael 2012-04-12 05:18:02 PDT
Created attachment 136881 [details]
Patch.

Split out the manual test from the existing test. And added missing spaces.
Comment 9 Yael 2012-04-12 08:27:44 PDT
Thanks for the review :)
Comment 10 WebKit Review Bot 2012-04-12 08:35:31 PDT
Comment on attachment 136881 [details]
Patch.

Clearing flags on attachment: 136881

Committed r113983: <http://trac.webkit.org/changeset/113983>
Comment 11 WebKit Review Bot 2012-04-12 08:35:40 PDT
All reviewed patches have been landed.  Closing bug.