https://bugs.webkit.org/show_bug.cgi?id=89114
authorbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Jul 2012 18:15:17 +0000 (18:15 +0000)
committerbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Jul 2012 18:15:17 +0000 (18:15 +0000)
REGRESSION (r112919): Setting scrollTop after setting display from none to block
fails
-and corresponding-
<rdar://problem/11656050>

Reviewed by Simon Fraser.

Source/WebCore:

ScrollAnimatorMac::immediateScrollTo() and ScrollAnimatorMac::immediateScrollBy()
both have an optimization in place so that they do not call
notifyPositionChanged() if the new scroll offset matches the ScrollAnimator's
cached m_currentPosX and m_currentPosY. So revision 112919 caused troubled with
this optimization because it allowed RenderLayers to restore a scrollOffset from
the Element if there is one cached there. This caused the RenderLayer to have a
scrollOffset that is improperly out-of-synch with the ScrollAnimator's
currentPosition (which will just be 0,0 since it is being re-created like the
RenderLayer). This fix makes sure they are in synch by calling
setCurrentPosition() on the ScrollAnimator when the cached position is non-zero.
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::RenderLayer):

LayoutTests:

* fast/overflow/setting-scrollTop-after-hide-show-expected.txt: Added.
* fast/overflow/setting-scrollTop-after-hide-show.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@123637 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/overflow/setting-scrollTop-after-hide-show-expected.txt [new file with mode: 0644]
LayoutTests/fast/overflow/setting-scrollTop-after-hide-show.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp

index 17e71a3..ae7a095 100644 (file)
@@ -1,3 +1,16 @@
+2012-07-25  Beth Dakin  <bdakin@apple.com>
+
+        https://bugs.webkit.org/show_bug.cgi?id=89114
+        REGRESSION (r112919): Setting scrollTop after setting display from none to block 
+        fails
+        -and corresponding-
+        <rdar://problem/11656050>
+
+        Reviewed by Simon Fraser.
+
+        * fast/overflow/setting-scrollTop-after-hide-show-expected.txt: Added.
+        * fast/overflow/setting-scrollTop-after-hide-show.html: Added.
+
 2012-07-25  Andreas Kling  <kling@webkit.org>
 
         Make ElementAttributeData a variable-sized object to reduce memory use.
diff --git a/LayoutTests/fast/overflow/setting-scrollTop-after-hide-show-expected.txt b/LayoutTests/fast/overflow/setting-scrollTop-after-hide-show-expected.txt
new file mode 100644 (file)
index 0000000..3a3b6e4
--- /dev/null
@@ -0,0 +1,7 @@
+In this test, we set a new scrollTop for a scrolling div, and then we make the div display:none. The test ensures that when we bring the div back by giving it a display value of block, that we also restore its scroll position. The test also ensures that we are able to set a new scrollTop value of 0 after that.
+
+scrollTop after restoring div: 20
+
+scrollTop after setting scrollTop back to 0: 0
+
+
diff --git a/LayoutTests/fast/overflow/setting-scrollTop-after-hide-show.html b/LayoutTests/fast/overflow/setting-scrollTop-after-hide-show.html
new file mode 100644 (file)
index 0000000..0f8ab23
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<script>
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+    
+    function log(message)
+     {
+         document.getElementById("console").appendChild(document.createTextNode(message + "\n"));
+     }
+</script>
+<body>
+    <p>In this test, we set a new scrollTop for a scrolling div, and then we make the div display:none. The test ensures that when we bring the div back by giving it a display value of block, that we also restore its scroll position. The test also ensures that we are able to set a new scrollTop value of 0 after that.</p>
+    <div id="scroller" style="height: 20px; overflow: scroll;">
+        <div style="height: 60px;"></div>
+    </div>
+    <pre id="console"></pre>
+    <script>
+    a = document.getElementById('scroller');
+    a.scrollTop = 20;
+    a.style.display = 'none';
+    a.scrollTop = 20;
+    a.style.display = 'block';
+    log('scrollTop after restoring div: ' + a.scrollTop + '\n');
+    a.scrollTop = 0;
+    log('scrollTop after setting scrollTop back to 0: ' + a.scrollTop + '\n');
+    </script>
+</body>
+</html>
index b50f34f..d8ee053 100644 (file)
@@ -1,3 +1,26 @@
+2012-07-25  Beth Dakin  <bdakin@apple.com>
+
+        https://bugs.webkit.org/show_bug.cgi?id=89114
+        REGRESSION (r112919): Setting scrollTop after setting display from none to block 
+        fails
+        -and corresponding-
+        <rdar://problem/11656050>
+
+        Reviewed by Simon Fraser.
+
+        ScrollAnimatorMac::immediateScrollTo() and ScrollAnimatorMac::immediateScrollBy() 
+        both have an optimization in place so that they do not call 
+        notifyPositionChanged() if the new scroll offset matches the ScrollAnimator's 
+        cached m_currentPosX and m_currentPosY. So revision 112919 caused troubled with 
+        this optimization because it allowed RenderLayers to restore a scrollOffset from 
+        the Element if there is one cached there. This caused the RenderLayer to have a 
+        scrollOffset that is improperly out-of-synch with the ScrollAnimator's 
+        currentPosition (which will just be 0,0 since it is being re-created like the 
+        RenderLayer). This fix makes sure they are in synch by calling 
+        setCurrentPosition() on the ScrollAnimator when the cached position is non-zero.
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::RenderLayer):
+
 2012-07-25  Andreas Kling  <kling@webkit.org>
 
         Make ElementAttributeData a variable-sized object to reduce memory use.
index 27425f7..29f7b29 100644 (file)
@@ -86,6 +86,7 @@
 #include "RenderTreeAsText.h"
 #include "RenderView.h"
 #include "ScaleTransformOperation.h"
+#include "ScrollAnimator.h"
 #include "Scrollbar.h"
 #include "ScrollbarTheme.h"
 #include "Settings.h"
@@ -195,6 +196,8 @@ RenderLayer::RenderLayer(RenderBoxModelObject* renderer)
         // We save and restore only the scrollOffset as the other scroll values are recalculated.
         Element* element = toElement(node);
         m_scrollOffset = element->savedLayerScrollOffset();
+        if (!m_scrollOffset.isZero())
+            scrollAnimator()->setCurrentPosition(FloatPoint(m_scrollOffset.width(), m_scrollOffset.height()));
         element->setSavedLayerScrollOffset(IntSize());
     }
 }