Bug 109033 - [Qt][EFL] Coordinated Graphics: scroll animation makes fixed element shake.
Summary: [Qt][EFL] Coordinated Graphics: scroll animation makes fixed element shake.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dongseong Hwang
URL:
Keywords:
Depends on: 110847 111829
Blocks: 103105 110066
  Show dependency treegraph
 
Reported: 2013-02-06 02:47 PST by Dongseong Hwang
Modified: 2022-02-27 23:32 PST (History)
16 users (show)

See Also:


Attachments
WIP: need more tests (13.12 KB, patch)
2013-02-26 03:05 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (13.69 KB, patch)
2013-03-03 18:26 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (13.82 KB, patch)
2013-03-05 00:40 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (12.81 KB, patch)
2013-03-06 00:23 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (12.90 KB, patch)
2013-03-08 00:09 PST, Dongseong Hwang
andersca: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2013-02-06 02:47:27 PST
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'.
Comment 1 Mikhail Pozdnyakov 2013-02-06 02:58:47 PST
might be relevant to https://bugs.webkit.org/show_bug.cgi?id=106656
Comment 2 Dongseong Hwang 2013-02-06 03:07:41 PST
(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.
Comment 3 Allan Sandfeld Jensen 2013-02-14 02:14:26 PST
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.
Comment 4 Dongseong Hwang 2013-02-19 21:54:18 PST
I think Bug 110080 is also related.
I suspect there is a bug in PageViewportController related to pending and commit.
Comment 5 Dongseong Hwang 2013-02-26 03:05:54 PST
Created attachment 190252 [details]
WIP: need more tests
Comment 6 Kenneth Rohde Christiansen 2013-02-26 06:40:01 PST
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?
Comment 7 Dongseong Hwang 2013-02-26 15:04:02 PST
(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.
Comment 8 Jocelyn Turcotte 2013-02-27 05:21:42 PST
(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.
Comment 9 Dongseong Hwang 2013-02-27 23:18:52 PST
(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 :)
Comment 10 Dongseong Hwang 2013-03-03 18:12:06 PST
(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.
Comment 11 Dongseong Hwang 2013-03-03 18:26:12 PST
Created attachment 191158 [details]
Patch
Comment 12 Allan Sandfeld Jensen 2013-03-04 02:46:46 PST
Comment on attachment 191158 [details]
Patch

Patch doesn't apply in UIProcess/efl/WebView.cpp.
Comment 13 Dongseong Hwang 2013-03-04 02:51:38 PST
(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 14 Jocelyn Turcotte 2013-03-04 03:32:07 PST
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 15 Dongseong Hwang 2013-03-04 15:04:12 PST
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.
Comment 16 Dongseong Hwang 2013-03-05 00:40:17 PST
Created attachment 191424 [details]
Patch
Comment 17 Kenneth Rohde Christiansen 2013-03-05 01:04:36 PST
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 18 Dongseong Hwang 2013-03-05 01:44:34 PST
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?
Comment 19 Jocelyn Turcotte 2013-03-05 05:16:15 PST
(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.
Comment 20 Dongseong Hwang 2013-03-06 00:23:08 PST
Created attachment 191671 [details]
Patch
Comment 21 Dongseong Hwang 2013-03-06 00:23:37 PST
(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. :-)
Comment 22 Dongseong Hwang 2013-03-08 00:09:59 PST
Created attachment 192165 [details]
Patch
Comment 23 Dongseong Hwang 2013-03-08 00:11:52 PST
now ews can run this patch. could you review? :)
Comment 24 Dongseong Hwang 2013-03-08 00:35:23 PST
Recently, regression related to scroll occurs. I filed Bug 111829.
Bug 111829 should be fixed before this bug.
Comment 25 Dongseong Hwang 2013-03-08 02:10:09 PST
Comment on attachment 192165 [details]
Patch

After landing 111829, this bug will be valid again.
Comment 26 Gyuyoung Kim 2013-07-16 18:45:18 PDT
(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 27 Anders Carlsson 2013-10-02 21:24:39 PDT
Comment on attachment 192165 [details]
Patch

Marking this patch r- for now since it touches Qt code and Qt has been removed.
Comment 28 Michael Catanzaro 2017-03-11 10:42:31 PST
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.