Changeset 256513 in webkit

Timestamp:

Feb 13, 2020 9:20:42 AM (4 years ago)

Author:

Simon Fraser

Message:

REGRESSION (r255037): Zooming in and out on Quip in macOS Safari can cause the content to be offset to the side
​https://bugs.webkit.org/show_bug.cgi?id=207674
rdar://problem/59404866

Reviewed by Antti Koivisto.

Source/WebCore:

Remove the early return in ScrollingStateScrollingNode::setRequestedScrollData(); comparing
with the last m_requestedScrollData is wrong, because requested scroll positions are not "state"
in the scrolling tree, they are requests to scroll. Ideally, they would be represented in some
different way in the scrolling tree.

Test: fast/scrolling/programmatic-scroll-to-zero-zero.html

• page/scrolling/ScrollingStateScrollingNode.cpp:

(WebCore::ScrollingStateScrollingNode::setRequestedScrollData):

LayoutTests:

Test that does a programmatic scroll to 0,0, does a user scroll, then a second programmatic scroll to 0,0,
which is expected to work.

• fast/scrolling/programmatic-scroll-to-zero-zero-expected.html: Added.
• fast/scrolling/programmatic-scroll-to-zero-zero.html: Added.
• platform/ios/TestExpectations: Skip the new test on iOS (it relies on eventSender) and sort the grouping.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/scrolling/programmatic-scroll-to-zero-zero-expected.html (added)
• LayoutTests/fast/scrolling/programmatic-scroll-to-zero-zero.html (added)
• LayoutTests/platform/ios/TestExpectations (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-02-13  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r255037): Zooming in and out on Quip in macOS Safari can cause the content to be offset to the side
+        https://bugs.webkit.org/show_bug.cgi?id=207674
+        rdar://problem/59404866
+
+        Reviewed by Antti Koivisto.
+        
+        Test that does a programmatic scroll to 0,0, does a user scroll, then a second programmatic scroll to 0,0,
+        which is expected to work.
+
+        * fast/scrolling/programmatic-scroll-to-zero-zero-expected.html: Added.
+        * fast/scrolling/programmatic-scroll-to-zero-zero.html: Added.
+        * platform/ios/TestExpectations: Skip the new test on iOS (it relies on eventSender) and sort the grouping.
+
 2020-02-13  Said Abou-Hallawa  <said@apple.com>
 

trunk/LayoutTests/platform/ios/TestExpectations

@@ -70 +70 @@
 
 # No wheel events on iOS
-fast/scrolling/iframe-scrollable-after-back.html [ Skip ]
-fast/scrolling/overflow-scrollable-after-back.html [ Skip ]
-fast/events/wheel-event-destroys-frame.html [ Skip ]
-fast/events/wheel-event-destroys-overflow.html [ Skip ]
-fast/events/wheel-event-outside-body.html [ Skip ]
-fast/scrolling/scroll-container-horizontally.html [ Failure ]
-fast/events/wheelevent-basic.html [ Skip ]
-fast/events/wheelevent-direction-inverted-from-device.html [ Skip ]
-fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html [ Skip ]
-fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html [ Skip ]
-fast/events/wheelevent-mousewheel-interaction.html [ Skip ]
+fast/events/continuous-platform-wheelevent-in-scrolling-div.html [ Skip ]
 fast/events/platform-wheelevent-in-scrolling-div.html [ Skip ]
 fast/events/platform-wheelevent-paging-x-in-non-scrolling-div.html [ Skip ]
@@ -92 +82 @@
 fast/events/platform-wheelevent-paging-y-in-scrolling-div.html [ Skip ]
 fast/events/platform-wheelevent-paging-y-in-scrolling-page.html [ Skip ]
+fast/events/wheel-event-destroys-frame.html [ Skip ]
+fast/events/wheel-event-destroys-overflow.html [ Skip ]
+fast/events/wheel-event-outside-body.html [ Skip ]
+fast/events/wheelevent-basic.html [ Skip ]
+fast/events/wheelevent-direction-inverted-from-device.html [ Skip ]
+fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html [ Skip ]
 fast/events/wheelevent-in-text-node.html [ Skip ]
-fast/events/continuous-platform-wheelevent-in-scrolling-div.html [ Skip ]
+fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html [ Skip ]
+fast/events/wheelevent-mousewheel-interaction.html [ Skip ]
+fast/scrolling/iframe-scrollable-after-back.html [ Skip ]
+fast/scrolling/overflow-scrollable-after-back.html [ Skip ]
+fast/scrolling/programmatic-scroll-to-zero-zero.html [ Skip ]
+fast/scrolling/scroll-container-horizontally.html [ Failure ]
 
 # This test requires alpha-channel video support.

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-02-13  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r255037): Zooming in and out on Quip in macOS Safari can cause the content to be offset to the side
+        https://bugs.webkit.org/show_bug.cgi?id=207674
+        rdar://problem/59404866
+
+        Reviewed by Antti Koivisto.
+        
+        Remove the early return in ScrollingStateScrollingNode::setRequestedScrollData(); comparing
+        with the last m_requestedScrollData is wrong, because requested scroll positions are not "state"
+        in the scrolling tree, they are requests to scroll. Ideally, they would be represented in some
+        different way in the scrolling tree.
+
+        Test: fast/scrolling/programmatic-scroll-to-zero-zero.html
+
+        * page/scrolling/ScrollingStateScrollingNode.cpp:
+        (WebCore::ScrollingStateScrollingNode::setRequestedScrollData):
+
 2020-02-13  Said Abou-Hallawa  <said@apple.com>
 

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

@@ -221 +221 @@
 void ScrollingStateScrollingNode::setRequestedScrollData(const RequestedScrollData& scrollData)
 {
-    if (scrollData == m_requestedScrollData)
-        return;
-
+    // Scroll position requests are imperative, not stateful, so we can't early return here.
     m_requestedScrollData = scrollData;
     setPropertyChanged(RequestedScrollPosition);