Programmatic scrolling and content changes are not always synchronized
https://bugs.webkit.org/show_bug.cgi?id=139245
rdar://problem/18833612
Reviewed by Anders Carlsson.
Manual test that tries to sync layout with programmatic scrolling.
* ManualTests/programmatic-scroll-flicker.html: Added.
Source/WebCore:
For programmatic scrolls, AsyncScrollingCoordinator::requestScrollPositionUpdate()
calls updateScrollPositionAfterAsyncScroll(), then dispatches the requested
scroll position to the scrolling thread.
Once the scrolling thread commits, it calls back to the main thread via
scheduleUpdateScrollPositionAfterAsyncScroll(), which schedules a second
call to updateScrollPositionAfterAsyncScroll() on a timer. That's a problem,
because some other scroll may have happened in the meantime; when the timer
fires, it can sometimes restore a stale scroll position.
Fix by bailing early from scheduleUpdateScrollPositionAfterAsyncScroll()
for programmatic scrolls, since we know that requestScrollPositionUpdate()
already did the updateScrollPositionAfterAsyncScroll().
Test:
ManualTests/programmatic-scroll-flicker.html
* page/FrameView.cpp:
(WebCore::FrameView::reset): nullptr.
(WebCore::FrameView::setScrollPosition): Ditto.
(WebCore::FrameView::setWasScrolledByUser): Ditto.
* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate): Use a local variable for
isProgrammaticScroll just to make sure we use the same value for the duration of this function.
(WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll): Do nothing
if this is a programmatic scroll.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@176899 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/ChangeLog b/ChangeLog
index 7d785f0..e5b5ca9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2014-12-05 Simon Fraser <simon.fraser@apple.com>
+
+ Programmatic scrolling and content changes are not always synchronized
+ https://bugs.webkit.org/show_bug.cgi?id=139245
+ rdar://problem/18833612
+
+ Reviewed by Anders Carlsson.
+
+ Manual test that tries to sync layout with programmatic scrolling.
+
+ * ManualTests/programmatic-scroll-flicker.html: Added.
+
2014-12-04 Alberto Garcia <berto@igalia.com>
can not find cairo-gl.h when build webkit with gtk on ubuntu 14.04
diff --git a/ManualTests/programmatic-scroll-flicker.html b/ManualTests/programmatic-scroll-flicker.html
new file mode 100644
index 0000000..f680f73
--- /dev/null
+++ b/ManualTests/programmatic-scroll-flicker.html
@@ -0,0 +1,120 @@
+<!DOCTYPE html>
+<head>
+<style>
+ #testInner {
+ position: absolute;
+ width: 600px;
+ top: 400px;
+ left: 200px;;
+ padding: 1em;
+ background: yellow;
+ box-shadow: 0 0 0.5em gray;
+ }
+
+ .marker {
+ position: fixed;
+ background: black;
+ color: white;
+ top: 200px;
+ padding: 1em;
+ }
+
+ .spacer {
+ height: 200px;
+ }
+
+ button {
+ position: fixed;
+ top: 100px;
+ left: 200px;
+ padding: 2em;
+ font-size: 16px;
+ font-weight: bold;
+ }
+
+ .trigger #testInner {
+ -webkit-transform: translateY(-200px);
+ }
+</style>
+</head>
+<body>
+<button onclick="toggleRunning()">
+ CLICK ME to start / stop test
+</button>
+<div class="marker">Correct top position</div>
+<div id="parent" class="trigger">
+ <div id="testInner">test element</div>
+</div>
+<p class="spacer">.</p>
+<p class="spacer">.</p>
+<p class="spacer">.</p>
+<p class="spacer">.</p>
+<p class="spacer">.</p>
+<p class="spacer">.</p>
+<p class="spacer" id="last">.</p>
+
+<script>
+var INTERVAL_MS = 17;
+var testElement = document.getElementById('testInner');
+var testContainerElement = document.getElementById('parent');
+
+var state = {};
+state.triggerTranslationOnOrOff = false;
+state.running = false;
+state.intervalId = 0;
+
+function updateState()
+{
+ var translated = testContainerElement.classList.contains('trigger');
+ state.scrolled = !translated;
+}
+
+function toggleRunning()
+{
+ state.running = !state.running;
+ if (state.running) {
+ updateState();
+ state.intervalId = setInterval(runSequence, INTERVAL_MS);
+ } else {
+ clearInterval(state.intervalId);
+ }
+}
+
+function doExpensiveContentUpdate()
+{
+ var content = 'I should be stable at the correct position and not flicker above/below';
+ if (state.scrolled)
+ content += '.';
+
+ testElement.innerHTML = content;
+
+ var lastElement = document.getElementById('last');
+ var startTime = Date.now();
+ for (var i = 0; i < 1000; i++) {
+ lastElement.innerHTML = (lastElement.innerHTML + '.');
+ }
+ var domWriteTimeMs = Date.now() - startTime;
+ if (domWriteTimeMs > 2 * INTERVAL_MS)
+ lastElement.innerHTML = '';
+}
+
+function runSequence()
+{
+ doExpensiveContentUpdate();
+
+ var newScrollTop;
+ if (state.scrolled) {
+ testContainerElement.classList.add('trigger');
+ newScrollTop = 0;
+ } else {
+ testContainerElement.classList.remove('trigger');
+ newScrollTop = 200;
+ }
+
+ document.body.scrollTop = newScrollTop;
+ state.scrolled = !state.scrolled;
+}
+
+</script>
+</body>
+
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index c1c4b04..5e40cf52 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,38 @@
+2014-12-05 Simon Fraser <simon.fraser@apple.com>
+
+ Programmatic scrolling and content changes are not always synchronized
+ https://bugs.webkit.org/show_bug.cgi?id=139245
+ rdar://problem/18833612
+
+ Reviewed by Anders Carlsson.
+
+ For programmatic scrolls, AsyncScrollingCoordinator::requestScrollPositionUpdate()
+ calls updateScrollPositionAfterAsyncScroll(), then dispatches the requested
+ scroll position to the scrolling thread.
+
+ Once the scrolling thread commits, it calls back to the main thread via
+ scheduleUpdateScrollPositionAfterAsyncScroll(), which schedules a second
+ call to updateScrollPositionAfterAsyncScroll() on a timer. That's a problem,
+ because some other scroll may have happened in the meantime; when the timer
+ fires, it can sometimes restore a stale scroll position.
+
+ Fix by bailing early from scheduleUpdateScrollPositionAfterAsyncScroll()
+ for programmatic scrolls, since we know that requestScrollPositionUpdate()
+ already did the updateScrollPositionAfterAsyncScroll().
+
+ Test:
+ ManualTests/programmatic-scroll-flicker.html
+
+ * page/FrameView.cpp:
+ (WebCore::FrameView::reset): nullptr.
+ (WebCore::FrameView::setScrollPosition): Ditto.
+ (WebCore::FrameView::setWasScrolledByUser): Ditto.
+ * page/scrolling/AsyncScrollingCoordinator.cpp:
+ (WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate): Use a local variable for
+ isProgrammaticScroll just to make sure we use the same value for the duration of this function.
+ (WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll): Do nothing
+ if this is a programmatic scroll.
+
2014-12-05 Timothy Horton <timothy_horton@apple.com>
Build fix.
diff --git a/Source/WebCore/page/FrameView.cpp b/Source/WebCore/page/FrameView.cpp
index 1064014..e021501 100644
--- a/Source/WebCore/page/FrameView.cpp
+++ b/Source/WebCore/page/FrameView.cpp
@@ -289,7 +289,7 @@
m_visuallyNonEmptyPixelCount = 0;
m_isVisuallyNonEmpty = false;
m_firstVisuallyNonEmptyLayoutCallbackPending = true;
- m_maintainScrollPositionAnchor = 0;
+ m_maintainScrollPositionAnchor = nullptr;
m_throttledTimers.clear();
}
@@ -2023,7 +2023,7 @@
void FrameView::setScrollPosition(const IntPoint& scrollPoint)
{
TemporaryChange<bool> changeInProgrammaticScroll(m_inProgrammaticScroll, true);
- m_maintainScrollPositionAnchor = 0;
+ m_maintainScrollPositionAnchor = nullptr;
ScrollView::setScrollPosition(scrollPoint);
}
@@ -3713,7 +3713,7 @@
{
if (m_inProgrammaticScroll)
return;
- m_maintainScrollPositionAnchor = 0;
+ m_maintainScrollPositionAnchor = nullptr;
if (m_wasScrolledByUser == wasScrolledByUser)
return;
m_wasScrolledByUser = wasScrolledByUser;
diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
index 1f82303..e4cb06f 100644
--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
@@ -176,8 +176,9 @@
if (!coordinatesScrollingForFrameView(frameView))
return false;
- if (frameView->inProgrammaticScroll() || frameView->frame().document()->inPageCache())
- updateScrollPositionAfterAsyncScroll(frameView->scrollLayerID(), scrollPosition, frameView->inProgrammaticScroll(), SetScrollingLayerPosition);
+ bool isProgrammaticScroll = frameView->inProgrammaticScroll();
+ if (isProgrammaticScroll || frameView->frame().document()->inPageCache())
+ updateScrollPositionAfterAsyncScroll(frameView->scrollLayerID(), scrollPosition, isProgrammaticScroll, SetScrollingLayerPosition);
// If this frame view's document is being put into the page cache, we don't want to update our
// main frame scroll position. Just let the FrameView think that we did.
@@ -188,7 +189,7 @@
if (!stateNode)
return false;
- stateNode->setRequestedScrollPosition(scrollPosition, frameView->inProgrammaticScroll());
+ stateNode->setRequestedScrollPosition(scrollPosition, isProgrammaticScroll);
return true;
}
@@ -196,6 +197,10 @@
{
ScheduledScrollUpdate scrollUpdate(nodeID, scrollPosition, programmaticScroll, scrollingLayerPositionAction);
+ // For programmatic scrolls, requestScrollPositionUpdate() has already called updateScrollPositionAfterAsyncScroll().
+ if (programmaticScroll)
+ return;
+
if (m_updateNodeScrollPositionTimer.isActive()) {
if (m_scheduledScrollUpdate.matchesUpdateType(scrollUpdate)) {
m_scheduledScrollUpdate.scrollPosition = scrollPosition;
