Programmatic scrolling and content changes are not always synchronized
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 6 Dec 2014 01:25:21 +0000 (01:25 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 6 Dec 2014 01:25:21 +0000 (01:25 +0000)
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: https://svn.webkit.org/repository/webkit/trunk@176899 268f45cc-cd09-0410-ab3c-d52691b4dbfc

ChangeLog
ManualTests/programmatic-scroll-flicker.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp

index 7d785f0..e5b5ca9 100644 (file)
--- 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 (file)
index 0000000..f680f73
--- /dev/null
@@ -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>
+
index c1c4b04..5e40cf5 100644 (file)
@@ -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.
index 1064014..e021501 100644 (file)
@@ -289,7 +289,7 @@ void FrameView::reset()
     m_visuallyNonEmptyPixelCount = 0;
     m_isVisuallyNonEmpty = false;
     m_firstVisuallyNonEmptyLayoutCallbackPending = true;
-    m_maintainScrollPositionAnchor = 0;
+    m_maintainScrollPositionAnchor = nullptr;
     m_throttledTimers.clear();
 }
 
@@ -2023,7 +2023,7 @@ void FrameView::scrollElementToRect(Element* element, const IntRect& rect)
 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 @@ void FrameView::setWasScrolledByUser(bool wasScrolledByUser)
 {
     if (m_inProgrammaticScroll)
         return;
-    m_maintainScrollPositionAnchor = 0;
+    m_maintainScrollPositionAnchor = nullptr;
     if (m_wasScrolledByUser == wasScrolledByUser)
         return;
     m_wasScrolledByUser = wasScrolledByUser;
index 1f82303..e4cb06f 100644 (file)
@@ -176,8 +176,9 @@ bool AsyncScrollingCoordinator::requestScrollPositionUpdate(FrameView* frameView
     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 @@ bool AsyncScrollingCoordinator::requestScrollPositionUpdate(FrameView* frameView
     if (!stateNode)
         return false;
 
-    stateNode->setRequestedScrollPosition(scrollPosition, frameView->inProgrammaticScroll());
+    stateNode->setRequestedScrollPosition(scrollPosition, isProgrammaticScroll);
     return true;
 }
 
@@ -196,6 +197,10 @@ void AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll(Scr
 {
     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;