When panning using touch, we can see shaking fixed element in ManualTests/fixed-position.html Strangely, if you early return in TextureMapperLayer::setScrollPositionDeltaIfNeeded, this shaking disappears. void TextureMapperLayer::setScrollPositionDeltaIfNeeded(const FloatSize& delta) { + return; ... } But when you drag using touch, we can see shaking with 'early return'.
might be relevant to https://bugs.webkit.org/show_bug.cgi?id=106656
(In reply to comment #1) > might be relevant to https://bugs.webkit.org/show_bug.cgi?id=106656 Yes, I think so. I suspect it is the timing issue in PageViewportController.
If I test on real web pages, it doesn't as much shake as just lack behind. When panning down the fixed elements will be slightly to high moving to catch up. The fast the pan, the more the fixed elements will be behind. Similarly if you pan up the fixed elements will be too low on the page. This is most easy to see with fixed elements fixed to the top or bottom of the page.
I think Bug 110080 is also related. I suspect there is a bug in PageViewportController related to pending and commit.
Created attachment 190252 [details] WIP: need more tests
Comment on attachment 190252 [details] WIP: need more tests View in context: https://bugs.webkit.org/attachment.cgi?id=190252&action=review > Source/WebKit2/UIProcess/qt/QtWebPageSGNode.cpp:65 > - coordinatedGraphicsScene()->paintToCurrentGLContext(renderMatrix, inheritedOpacity(), clipRect(), mirrored ? TextureMapper::PaintingMirrored : 0); > + coordinatedGraphicsScene()->paintToCurrentGLContext(renderMatrix, inheritedOpacity(), clipRect(), pageNode()->scrollPosition(), mirrored ? TextureMapper::PaintingMirrored : 0); isnt the scroll pos part of the renderMatrix?
(In reply to comment #6) > (From update of attachment 190252 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190252&action=review > > > Source/WebKit2/UIProcess/qt/QtWebPageSGNode.cpp:65 > > - coordinatedGraphicsScene()->paintToCurrentGLContext(renderMatrix, inheritedOpacity(), clipRect(), mirrored ? TextureMapper::PaintingMirrored : 0); > > + coordinatedGraphicsScene()->paintToCurrentGLContext(renderMatrix, inheritedOpacity(), clipRect(), pageNode()->scrollPosition(), mirrored ? TextureMapper::PaintingMirrored : 0); > > isnt the scroll pos part of the renderMatrix? good question! we make renderMatrix using scrollposition, native window position, page scale and device scale, so it is hard to extract css position from renderMatrix. And I think it is natural that the clients that call paintToCurrentGLContext() prepare css position because they already prepare renderMatrix.
(In reply to comment #7) > good question! we make renderMatrix using scrollposition, native window position, page scale and device scale, so it is hard to extract css position from renderMatrix. And I think it is natural that the clients that call paintToCurrentGLContext() prepare css position because they already prepare renderMatrix. The position is already in the matrix and I think it would be better if you use it instead of duplicating this information in a separate parameter. That's how OpenGL represents a position: with a matrix, because it's unusual in this world to have all your objects on a single plane.
(In reply to comment #8) > The position is already in the matrix and I think it would be better if you use it instead of duplicating this information in a separate parameter. > > That's how OpenGL represents a position: with a matrix, because it's unusual in this world to have all your objects on a single plane. Ok, Jocelyn and kenneth. I'll survey more :)
(In reply to comment #9) > (In reply to comment #8) > > The position is already in the matrix and I think it would be better if you use it instead of duplicating this information in a separate parameter. > > > > That's how OpenGL represents a position: with a matrix, because it's unusual in this world to have all your objects on a single plane. > > Ok, Jocelyn and kenneth. I'll survey more :) I think it is very hard to extract scroll position from the matrix. In Qt case, QSGRenderNode generates the matrix using scroll position, page scale, device scale and node position. Scene can not get node position. In EFL case, WebView generates the matrix using scroll position, page scale, device scale and userViewportTransform. Scene can not get userViewportTransform. I think this nasty situation (Scene should know scroll position) is originated from fixed element positioned in contents coordinates, not viewport coordinates. I think we need to go with this not good code until we change WebCore to assign fixed element's position in viewport coordinates.
Created attachment 191158 [details] Patch
Comment on attachment 191158 [details] Patch Patch doesn't apply in UIProcess/efl/WebView.cpp.
(In reply to comment #12) > (From update of attachment 191158 [details]) > Patch doesn't apply in UIProcess/efl/WebView.cpp. Yes, because this patch depends on Bug 110847. After resolving Bug 110847, I'll re-post. :)
Comment on attachment 191158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191158&action=review I agree that with the current situation, with fixed layers expecting a delta, it won't be pretty solving this only with matrices. > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:208 > void QRawWebViewPrivate::pageDidRequestScroll(const WebCore::IntPoint& pos) > { > m_client->viewRequestedScroll(pos); > - > + m_position = QPointF(pos.x(), pos.y()); I doubt that adding this line fixes everything. From my understanding there is poor support for fixed elements in QRawWebView, if so please just make sure it builds and avoid adding more complexity to it.
Comment on attachment 191158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191158&action=review >> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:208 >> + m_position = QPointF(pos.x(), pos.y()); > > I doubt that adding this line fixes everything. From my understanding there is poor support for fixed elements in QRawWebView, if so please just make sure it builds and avoid adding more complexity to it. Thank you for review! Do you mean that there is another point to change scroll position in QRawWebView? I introduced m_position to trace scroll position to make Scene know that. In my understanding, QRawWebView does not have flickering or pinch event handling like QQuickWebView, so I think QRawWebViewPrivate::pageDidRequestScroll is only point changing scroll position. On the other hands, IMO supporting for fixed element is the role of Coordinated Graphics, not QRawWebView or QQuickWebView. When it comes to fixed element, what is different between QRawWebView and QQuickWebView? What I need from QRawWebView or QQuickWebView is just to query scroll position.
Created attachment 191424 [details] Patch
Comment on attachment 191424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191424&action=review > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:87 > > - adjustPositionForFixedLayers(); > + adjustPositionForFixedLayers(scrollPosition); why not just call setScrollPosition before this? That would be a minimal change to the code
Comment on attachment 191424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191424&action=review >> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:87 >> + adjustPositionForFixedLayers(scrollPosition); > > why not just call setScrollPosition before this? That would be a minimal change to the code That is a way! What we have to do is to decide to remove either one parameter or one member :) do you think that it is better to remain setScrollPosition() and m_scrollPosition?
(In reply to comment #15) > Do you mean that there is another point to change scroll position in QRawWebView? I introduced m_position to trace scroll position to make Scene know that. > In my understanding, QRawWebView does not have flickering or pinch event handling like QQuickWebView, so I think QRawWebViewPrivate::pageDidRequestScroll is only point changing scroll position. QRawWebView is not completely implemented, we'll reuse it or get rid of it soon, so please just make sure that it compiles. In this case please just pass (0,0) for the new parameter in paintToCurrentGLContext.
Created attachment 191671 [details] Patch
(In reply to comment #19) > QRawWebView is not completely implemented, we'll reuse it or get rid of it soon, so please just make sure that it compiles. In this case please just pass (0,0) for the new parameter in paintToCurrentGLContext. aha, I see. :-)
Created attachment 192165 [details] Patch
now ews can run this patch. could you review? :)
Recently, regression related to scroll occurs. I filed Bug 111829. Bug 111829 should be fixed before this bug.
Comment on attachment 192165 [details] Patch After landing 111829, this bug will be valid again.
(In reply to comment #25) > (From update of attachment 192165 [details]) > After landing 111829, this bug will be valid again. Dongseong, Bug 111829 was landed. Any update ?
Comment on attachment 192165 [details] Patch Marking this patch r- for now since it touches Qt code and Qt has been removed.
Closing this bug because the EFL port has been removed from trunk. If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.