Bug 149060 – [ThreadedCompositor] Scrolling artifacts on accelerated subframes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Emanuele Aina]

Created attachment 261000 [details]
Accelerated subframe testcase

When scrolling an accelerated subframe (eg. an <iframe> containing a transformed element, see the attached scroll.html file) the main frame is scrolled temporarily producing ugly glitches.

This seems to due to excessive scroll notifications where the lower layers notify the upper layers again.

I guess an automated testcase would be appropriate, but I don't know where to start, any suggestion would be appropriate.

[Emanuele Aina]

Created attachment 261001 [details]
Patch

[Michael Catanzaro]

Yoon, what do you think?

[Zan Dobersek]

Comment on attachment 261001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261001&action=review

This needs some clarification. Yoon should chip in as well.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:126
>  void ThreadedCoordinatedLayerTreeHost::scrollNonCompositedContents(const WebCore::IntRect& rect)

The parameter is now unused.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:-128
> -    m_compositor->scrollTo(rect.location());

After removing this, ThreadedCompositor::scrollTo() is not called anymore. Do we still need it?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:-242
> -    if (m_lastScrollPosition != roundedIntPoint(rect.location())) {
> -        m_lastScrollPosition = roundedIntPoint(rect.location());
> -
> -        if (!m_webPage->corePage()->mainFrame().view()->useFixedLayout())
> -            m_webPage->corePage()->mainFrame().view()->notifyScrollPositionChanged(m_lastScrollPosition);
> -    }
> -
> -    if (m_lastScaleFactor != scale) {
> -        m_lastScaleFactor = scale;
> -        didScaleFactorChanged(m_lastScaleFactor, m_lastScrollPosition);
> -    }

Same for m_lastScrollPosition and m_lastScaleFactor -- these are unused afterwards.

[Carlos Garcia Campos]

I don't see any differences with the attached test case between threaded compositor enabled and disabled.

[Michael Catanzaro]

Emanuele, any update on this?

[Emanuele Aina]

I still see the page jumping badly when trying to scroll down https://people.collabora.com/~em/wk/tests/scroll.html

My steps (under X11):

* ./MiniBrowser https://people.collabora.com/~em/wk/tests/scroll.html
* move mouse pointer over the top iframe with the 300x300 image placeholder
* scroll down with a double-finger drag on my touchpad
* iframe scrolls correctly
* when the iframe has been scrolled completely, the scroll events propagate to the main frame, scrolling the page correctly
* if I keep scrolling down at some point the page goes back to its zero scrolling position

[Emanuele Aina]

Tested on r205858/git 804075239217.

[Emanuele Aina]

I haven't re-tested the patch.

[Emanuele Aina]

It seems that the jump happens as soon as the mouse pointer enters the second iframe during the scroll gesture.

[Emanuele Aina]

Oh, right, that's what I meant in the original report when I was talking about iframes, the scrollable placeholder on top isn't an iframe... :/

In fact, touch-scrolling in the nested iframe produces the flashes due to the page being temporarily scrolled to 0,0 and then back to the right offset.

[Emanuele Aina]

Created attachment 288948 [details]
Patch

[Emanuele Aina]

The updated patch seems to fix the issue on my testcase. I'm not sure if it breaks anything else though. :)

[Emanuele Aina]

For what is worth, the one I removed was the only use of SimpleViewportController::didScroll(), so I tried to drop it as well and things still seem to work.

I now need to check where SimpleViewportController::visibleContentsRect() is called, as it uses the value updated by didScroll().

[Carlos Garcia Campos]

Comment on attachment 288948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288948&action=review

> w/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:82
> -    m_viewportController.didScroll(rect.location());
>      didChangeViewport();

This is not correct, it breaks fixed layout scrolling. Also, if we don't call didScroll, we shouldn't need to call didChangeViewport() either.

[Carlos Garcia Campos]

Created attachment 289055 [details]
Patch

This should fix the issue

[Carlos Garcia Campos]

Committed r206066: <http://trac.webkit.org/changeset/206066>