Changeset 249581 in webkit

Timestamp:

Sep 6, 2019, 11:05:51 AM (7 years ago)

Author:

Simon Fraser

Message:

REGRESSION (iOS 13): If an overflow:hidden with a non-zero scroll position is toggled to overflow:scroll, some other scroll causes its scroll position to get reset
​https://bugs.webkit.org/show_bug.cgi?id=201528
rdar://problem/55044885

Reviewed by Frédéric Wang.
Source/WebCore:

If, when an overflow scrolling node is created, the scroller has non-zero scroll
position (for example, via toggling to overflow:hidden, setting scrollTop, then toggling
to overflow:scroll), then on the next update its scroll position will reset back to zero.

The bug was that newly created ScrollingTreeScrollingNodes didn't set m_currentScrollPosition
to the scroll position coming from the state node, so a subsequent update could cause
the 0,0 currentScrollPosition to get applied. If we're making a new node, and there's no
requestedScrollPosition, then initialize m_currentScrollPosition.

Test: scrollingcoordinator/ios/scroller-initial-scroll-position.html

• page/scrolling/ScrollingTreeScrollingNode.cpp:

(WebCore::ScrollingTreeScrollingNode::commitStateBeforeChildren):
(WebCore::ScrollingTreeScrollingNode::commitStateAfterChildren):

• page/scrolling/ScrollingTreeScrollingNode.h:

LayoutTests:

• scrollingcoordinator/ios/scroller-initial-scroll-position-expected.html: Added.
• scrollingcoordinator/ios/scroller-initial-scroll-position.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/scrollingcoordinator/ios/scroller-initial-scroll-position-expected.html (added)
• LayoutTests/scrollingcoordinator/ios/scroller-initial-scroll-position.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp (modified) (2 diffs)
• Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ -631 +631 @@
         * platform/mac/TestExpectations:
 
+2019-09-06  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (iOS 13): If an overflow:hidden with a non-zero scroll position is toggled to overflow:scroll, some other scroll causes its scroll position to get reset
+        https://bugs.webkit.org/show_bug.cgi?id=201528
+        rdar://problem/55044885
+
+        Reviewed by Frédéric Wang.
+
+        * scrollingcoordinator/ios/scroller-initial-scroll-position-expected.html: Added.
+        * scrollingcoordinator/ios/scroller-initial-scroll-position.html: Added.
+
 2019-09-04  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ -511 +511 @@
 
         * loader/EmptyFrameLoaderClient.h:
+
+2019-09-06  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (iOS 13): If an overflow:hidden with a non-zero scroll position is toggled to overflow:scroll, some other scroll causes its scroll position to get reset
+        https://bugs.webkit.org/show_bug.cgi?id=201528
+        rdar://problem/55044885
+
+        Reviewed by Frédéric Wang.
+        
+        If, when an overflow scrolling node is created, the scroller has non-zero scroll
+        position (for example, via toggling to overflow:hidden, setting scrollTop, then toggling
+        to overflow:scroll), then on the next update its scroll position will reset back to zero.
+
+        The bug was that newly created ScrollingTreeScrollingNodes didn't set m_currentScrollPosition
+        to the scroll position coming from the state node, so a subsequent update could cause
+        the 0,0 currentScrollPosition to get applied. If we're making a new node, and there's no
+        requestedScrollPosition, then initialize m_currentScrollPosition.
+
+        Test: scrollingcoordinator/ios/scroller-initial-scroll-position.html
+
+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
+        (WebCore::ScrollingTreeScrollingNode::commitStateBeforeChildren):
+        (WebCore::ScrollingTreeScrollingNode::commitStateAfterChildren):
+        * page/scrolling/ScrollingTreeScrollingNode.h:
 
 2019-09-04  Yusuke Suzuki  <ysuzuki@apple.com>

trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp

@@ -63 +63 @@
         m_reachableContentsSize = state.reachableContentsSize();
 
-    if (state.hasChangedProperty(ScrollingStateScrollingNode::ScrollPosition))
+    if (state.hasChangedProperty(ScrollingStateScrollingNode::ScrollPosition)) {
         m_lastCommittedScrollPosition = state.scrollPosition();
+        if (m_isFirstCommit && !state.hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition))
+            m_currentScrollPosition = m_lastCommittedScrollPosition;
+    }
 
     if (state.hasChangedProperty(ScrollingStateScrollingNode::ParentRelativeScrollableRect))
@@ -112 +115 @@
     if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition))
         scrollingTree().scrollingTreeNodeRequestsScroll(scrollingNodeID(), scrollingStateNode.requestedScrollPosition(), scrollingStateNode.requestedScrollPositionRepresentsProgrammaticScroll());
+
+    m_isFirstCommit = false;
 }
 

trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h

@@ -152 +152 @@
     ScrollableAreaParameters m_scrollableAreaParameters;
     bool m_expectsWheelEventTestTrigger { false };
+    bool m_isFirstCommit { true };
 
 #if PLATFORM(COCOA)