Summary: | [Qt][WK2] Nested fixed elements scroll too fast | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | noam, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Yael
2012-04-11 14:44:39 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.
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 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? (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 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. Created attachment 136809 [details] Patch. Cleanup the initial patch as suggested in comment #3 and #5. 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? Created attachment 136881 [details]
Patch.
Split out the manual test from the existing test. And added missing spaces.
Thanks for the review :) Comment on attachment 136881 [details] Patch. Clearing flags on attachment: 136881 Committed r113983: <http://trac.webkit.org/changeset/113983> All reviewed patches have been landed. Closing bug. |