Changeset 219320 in webkit

Timestamp:

Jul 10, 2017 8:40:59 PM (7 years ago)

Author:

Simon Fraser

Message:

[WK2 iOS] REGRESSION (r216803) During momentum scroll, getBoundingClientRect returns wrong coordinates (missing images on pinterest, elle.com and many other sites)
​https://bugs.webkit.org/show_bug.cgi?id=174286
rdar://problem/32864180

Reviewed by Dean Jackson.

Source/WebCore:

r216803 made getBoundingClientRects relative to the layout viewport, but when scrolling we
only update that on stable viewport updates (at the end of the scroll). This meant that during
unstable updates, getBoundingClientRects() used a "frozen" viewport origin so things on-screen
would appear to be off-screen, causing sites to fail to dynamically load images etc. when
scrolling.

Fix by pushing an optional "unstable" layout viewport rect onto FrameView, which gets used by
FrameView::documentToClientOffset(). This is cleared when we do a stable update.

This is a short-term solution. Longer term, I would prefer to always call setLayoutViewportOverrideRect(),
but fix the scrolling tree logic to work correctly in this case.

Add a bit more scrolling logging.

Test: fast/visual-viewport/ios/get-bounding-client-rect-unstable.html

• page/FrameView.cpp:

(WebCore::FrameView::setUnstableLayoutViewportRect):
(WebCore::FrameView::documentToClientOffset):

• page/FrameView.h:
• page/scrolling/AsyncScrollingCoordinator.cpp:

(WebCore::AsyncScrollingCoordinator::reconcileScrollingState):

• page/scrolling/ScrollingStateFixedNode.cpp:

(WebCore::ScrollingStateFixedNode::updateConstraints):
(WebCore::ScrollingStateFixedNode::reconcileLayerPositionForViewportRect):

LayoutTests:

• fast/visual-viewport/ios/get-bounding-client-rect-unstable-expected.txt: Added.
• fast/visual-viewport/ios/get-bounding-client-rect-unstable.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/visual-viewport/ios/get-bounding-client-rect-unstable-expected.txt (added)
• LayoutTests/fast/visual-viewport/ios/get-bounding-client-rect-unstable.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (2 diffs)
• Source/WebCore/page/FrameView.h (modified) (2 diffs)
• Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp (modified) (2 diffs)
• Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-07-10  Simon Fraser  <simon.fraser@apple.com>
+
+        [WK2 iOS] REGRESSION (r216803) During momentum scroll, getBoundingClientRect returns wrong coordinates (missing images on pinterest, elle.com and many other sites)
+        https://bugs.webkit.org/show_bug.cgi?id=174286
+        rdar://problem/32864180
+
+        Reviewed by Dean Jackson.
+
+        * fast/visual-viewport/ios/get-bounding-client-rect-unstable-expected.txt: Added.
+        * fast/visual-viewport/ios/get-bounding-client-rect-unstable.html: Added.
+
 2017-07-10  John Wilander  <wilander@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-07-10  Simon Fraser  <simon.fraser@apple.com>
+
+        [WK2 iOS] REGRESSION (r216803) During momentum scroll, getBoundingClientRect returns wrong coordinates (missing images on pinterest, elle.com and many other sites)
+        https://bugs.webkit.org/show_bug.cgi?id=174286
+        rdar://problem/32864180
+
+        Reviewed by Dean Jackson.
+
+        r216803 made getBoundingClientRects relative to the layout viewport, but when scrolling we
+        only update that on stable viewport updates (at the end of the scroll). This meant that during
+        unstable updates, getBoundingClientRects() used a "frozen" viewport origin so things on-screen
+        would appear to be off-screen, causing sites to fail to dynamically load images etc. when
+        scrolling.
+
+        Fix by pushing an optional "unstable" layout viewport rect onto FrameView, which gets used by
+        FrameView::documentToClientOffset(). This is cleared when we do a stable update.
+
+        This is a short-term solution. Longer term, I would prefer to always call setLayoutViewportOverrideRect(),
+        but fix the scrolling tree logic to work correctly in this case.
+
+        Add a bit more scrolling logging.
+
+        Test: fast/visual-viewport/ios/get-bounding-client-rect-unstable.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::setUnstableLayoutViewportRect):
+        (WebCore::FrameView::documentToClientOffset):
+        * page/FrameView.h:
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::reconcileScrollingState):
+        * page/scrolling/ScrollingStateFixedNode.cpp:
+        (WebCore::ScrollingStateFixedNode::updateConstraints):
+        (WebCore::ScrollingStateFixedNode::reconcileLayerPositionForViewportRect):
+
 2017-07-10  John Wilander  <wilander@apple.com>
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -1919 +1919 @@
 }
 
+void FrameView::setUnstableLayoutViewportRect(std::optional<LayoutRect> rect)
+{
+    if (rect == m_unstableLayoutViewportRect)
+        return;
+
+    m_unstableLayoutViewportRect = rect;
+}
+
 LayoutSize FrameView::baseLayoutViewportSize() const
 {
@@ -4906 +4914 @@
 FloatSize FrameView::documentToClientOffset() const
 {
-    FloatSize clientOrigin = frame().settings().visualViewportEnabled() ? -toFloatSize(layoutViewportRect().location()) : -toFloatSize(visibleContentRect().location());
+    FloatSize clientOrigin;
+    
+    if (frame().settings().visualViewportEnabled())
+        clientOrigin = -toFloatSize(m_unstableLayoutViewportRect ? m_unstableLayoutViewportRect.value().location() : layoutViewportRect().location()); 
+    else
+        clientOrigin = -toFloatSize(visibleContentRect().location());
 
     // Layout and visual viewports are affected by page zoom, so we need to factor that out.

trunk/Source/WebCore/page/FrameView.h

@@ -259 +259 @@
     // Used with delegated scrolling (i.e. iOS).
     WEBCORE_EXPORT void setLayoutViewportOverrideRect(std::optional<LayoutRect>);
+    // If set, overrides m_layoutViewportOverrideRect. Only used during unstable scroll updates, so that "client" coordinates are 
+    // computed with an origin that changes along with the scroll position.
+    // FIXME: This is ugly; would be better to be able to make setLayoutViewportOverrideRect() callable during unstable updates. 
+    WEBCORE_EXPORT void setUnstableLayoutViewportRect(std::optional<LayoutRect>);
 
     // These are in document coordinates, unaffected by page scale (but affected by zooming).
@@ -834 +838 @@
     LayoutPoint m_layoutViewportOrigin;
     std::optional<LayoutRect> m_layoutViewportOverrideRect;
+    std::optional<LayoutRect> m_unstableLayoutViewportRect;
 
     unsigned m_deferSetNeedsLayoutCount;

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

@@ -378 +378 @@
     frameView.setInProgrammaticScroll(programmaticScroll);
 
+    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator " << this << " reconcileScrollingState scrollPosition " << scrollPosition << " programmaticScroll " << programmaticScroll << " stable " << inStableState << " " << scrollingLayerPositionAction);
+
     std::optional<FloatRect> layoutViewportRect;
 
@@ -385 +387 @@
                 frameView.setBaseLayoutViewportOrigin(LayoutPoint(origin.value()), FrameView::TriggerLayoutOrNot::No);
         }, [&frameView, &layoutViewportRect, inStableState, visualViewportEnabled = visualViewportEnabled()](std::optional<FloatRect> overrideRect) {
+            if (!overrideRect)
+                return;
+        
             layoutViewportRect = overrideRect;
-            if (overrideRect && inStableState) {
-                if (visualViewportEnabled)
+            if (visualViewportEnabled) {
+                if (inStableState) {
                     frameView.setLayoutViewportOverrideRect(LayoutRect(overrideRect.value()));
+                    frameView.setUnstableLayoutViewportRect(std::nullopt);
+                } else
+                    frameView.setUnstableLayoutViewportRect(LayoutRect(layoutViewportRect.value()));
+            }
 #if PLATFORM(IOS)
-                else
-                    frameView.setCustomFixedPositionLayoutRect(enclosingIntRect(overrideRect.value()));
+            else if (inStableState)
+                frameView.setCustomFixedPositionLayoutRect(enclosingIntRect(overrideRect.value()));
 #endif
-            }
         }
     );

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

@@ -66 +66 @@
         return;
 
+    LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateFixedNode " << scrollingNodeID() << " updateConstraints with viewport rect " << constraints.viewportRectAtLastLayout());
+
     m_constraints = constraints;
     setPropertyChanged(ViewportConstraints);
@@ -76 +78 @@
         GraphicsLayer* graphicsLayer = static_cast<GraphicsLayer*>(layer());
 
-        LOG_WITH_STREAM(Compositing, stream << "ScrollingStateFixedNode::reconcileLayerPositionForViewportRect setting position of layer " << graphicsLayer->primaryLayerID() << " to " << position);
+        LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateFixedNode " << scrollingNodeID() <<" reconcileLayerPositionForViewportRect " << action << " position of layer " << graphicsLayer->primaryLayerID() << " to " << position);
         
         switch (action) {