Bug 109033 – [Qt][EFL] Coordinated Graphics: scroll animation makes fixed element shake.

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Dongseong Hwang]

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'.

[Mikhail Pozdnyakov]

might be relevant to https://bugs.webkit.org/show_bug.cgi?id=106656

[Dongseong Hwang]

(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.

[Allan Sandfeld Jensen]

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.

[Dongseong Hwang]

I think Bug 110080 is also related.
I suspect there is a bug in PageViewportController related to pending and commit.

[Dongseong Hwang]

Created attachment 190252 [details]
WIP: need more tests

[Kenneth Rohde Christiansen]

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?

[Dongseong Hwang]

(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.

[Jocelyn Turcotte]

(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.

[Dongseong Hwang]

(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 :)

[Dongseong Hwang]

(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.

[Dongseong Hwang]

Created attachment 191158 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 191158 [details]
Patch

Patch doesn't apply in UIProcess/efl/WebView.cpp.

[Dongseong Hwang]

(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. :)

[Jocelyn Turcotte]

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.

[Dongseong Hwang]

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.

[Dongseong Hwang]

Created attachment 191424 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Dongseong Hwang]

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?

[Jocelyn Turcotte]

(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.

[Dongseong Hwang]

Created attachment 191671 [details]
Patch

[Dongseong Hwang]

(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. :-)

[Dongseong Hwang]

Created attachment 192165 [details]
Patch

[Dongseong Hwang]

now ews can run this patch. could you review? :)

[Dongseong Hwang]

Recently, regression related to scroll occurs. I filed Bug 111829.
Bug 111829 should be fixed before this bug.

[Dongseong Hwang]

Comment on attachment 192165 [details]
Patch

After landing 111829, this bug will be valid again.

[Gyuyoung Kim]

(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 ?

[Anders Carlsson]

Comment on attachment 192165 [details]
Patch

Marking this patch r- for now since it touches Qt code and Qt has been removed.

[Michael Catanzaro]

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.