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.
Created attachment 261001 [details] Patch
Yoon, what do you think?
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.
I don't see any differences with the attached test case between threaded compositor enabled and disabled.
Emanuele, any update on this?
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
Tested on r205858/git 804075239217.
I haven't re-tested the patch.
It seems that the jump happens as soon as the mouse pointer enters the second iframe during the scroll gesture.
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.
Created attachment 288948 [details] Patch
The updated patch seems to fix the issue on my testcase. I'm not sure if it breaks anything else though. :)
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().
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.
Created attachment 289055 [details] Patch This should fix the issue
Committed r206066: <http://trac.webkit.org/changeset/206066>