- Revision
- 219320
- Author
- [email protected]
- Date
- 2017-07-10 20:40:59 -0700 (Mon, 10 Jul 2017)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (219319 => 219320)
--- trunk/LayoutTests/ChangeLog 2017-07-11 02:52:07 UTC (rev 219319)
+++ trunk/LayoutTests/ChangeLog 2017-07-11 03:40:59 UTC (rev 219320)
@@ -1,3 +1,14 @@
+2017-07-10 Simon Fraser <[email protected]>
+
+ [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 <[email protected]>
Resource Load Statistics: Prune statistics in orders of importance
Added: trunk/LayoutTests/fast/visual-viewport/ios/get-bounding-client-rect-unstable-expected.txt (0 => 219320)
--- trunk/LayoutTests/fast/visual-viewport/ios/get-bounding-client-rect-unstable-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/visual-viewport/ios/get-bounding-client-rect-unstable-expected.txt 2017-07-11 03:40:59 UTC (rev 219320)
@@ -0,0 +1,9 @@
+PASS boundingClientRect.top is expectedTop
+Doing unstable scroll
+PASS boundingClientRect.top is expectedTop
+Finishing scroll
+PASS boundingClientRect.top is expectedTop
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/visual-viewport/ios/get-bounding-client-rect-unstable.html (0 => 219320)
--- trunk/LayoutTests/fast/visual-viewport/ios/get-bounding-client-rect-unstable.html (rev 0)
+++ trunk/LayoutTests/fast/visual-viewport/ios/get-bounding-client-rect-unstable.html 2017-07-11 03:40:59 UTC (rev 219320)
@@ -0,0 +1,78 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+
+<html>
+<head>
+ <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+ <script src=""
+ <script>
+ jsTestIsAsync = true;
+
+ function simulateScrollingStart(offsetTop)
+ {
+ return `(function() {
+ uiController.stableStateOverride = false;
+ uiController.immediateScrollToOffset(0, ${offsetTop});
+ uiController.doAfterPresentationUpdate(function() {
+ uiController.uiScriptComplete();
+ });
+ })()`;
+ }
+
+ function simulateScrollingEnd(offsetTop)
+ {
+ return `(function() {
+ uiController.stableStateOverride = true;
+ uiController.doAfterNextStablePresentationUpdate(function() {
+ uiController.uiScriptComplete();
+ });
+ })()`;
+ }
+
+ var boundingClientRect;
+ var expectedTop;
+ function checkBoundingClientRect(top)
+ {
+ var target = document.getElementById('target');
+ boundingClientRect = target.getBoundingClientRect();
+ expectedTop = top;
+ shouldBe('boundingClientRect.top', 'expectedTop');
+ }
+
+ function doTest()
+ {
+ checkBoundingClientRect(350);
+ debug('Doing unstable scroll');
+ testRunner.runUIScript(simulateScrollingStart(500), function() {
+ checkBoundingClientRect(-150);
+
+ debug('Finishing scroll');
+ testRunner.runUIScript(simulateScrollingEnd(), function() {
+ checkBoundingClientRect(-150);
+ finishJSTest();
+ });
+ });
+ }
+
+ window.addEventListener('load', doTest, false);
+ </script>
+ <style>
+ body {
+ height: 5000px;
+ }
+ #target {
+ position: absolute;
+ top: 350px;
+ left: 20px;
+ height: 200px;
+ width: 200px;
+ background-color: gray;
+ }
+ </style>
+</head>
+<body>
+
+ <div id="target"></div>
+
+ <script src=""
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (219319 => 219320)
--- trunk/Source/WebCore/ChangeLog 2017-07-11 02:52:07 UTC (rev 219319)
+++ trunk/Source/WebCore/ChangeLog 2017-07-11 03:40:59 UTC (rev 219320)
@@ -1,3 +1,37 @@
+2017-07-10 Simon Fraser <[email protected]>
+
+ [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 <[email protected]>
Resource Load Statistics: Prune statistics in orders of importance
Modified: trunk/Source/WebCore/page/FrameView.cpp (219319 => 219320)
--- trunk/Source/WebCore/page/FrameView.cpp 2017-07-11 02:52:07 UTC (rev 219319)
+++ trunk/Source/WebCore/page/FrameView.cpp 2017-07-11 03:40:59 UTC (rev 219320)
@@ -1918,6 +1918,14 @@
setViewportConstrainedObjectsNeedLayout();
}
+void FrameView::setUnstableLayoutViewportRect(std::optional<LayoutRect> rect)
+{
+ if (rect == m_unstableLayoutViewportRect)
+ return;
+
+ m_unstableLayoutViewportRect = rect;
+}
+
LayoutSize FrameView::baseLayoutViewportSize() const
{
return renderView() ? renderView()->size() : size();
@@ -4905,7 +4913,12 @@
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.
return clientOrigin.scaled(1 / frame().pageZoomFactor());
Modified: trunk/Source/WebCore/page/FrameView.h (219319 => 219320)
--- trunk/Source/WebCore/page/FrameView.h 2017-07-11 02:52:07 UTC (rev 219319)
+++ trunk/Source/WebCore/page/FrameView.h 2017-07-11 03:40:59 UTC (rev 219320)
@@ -258,6 +258,10 @@
// If set, overrides the default "m_layoutViewportOrigin, size of initial containing block" rect.
// 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).
WEBCORE_EXPORT LayoutRect layoutViewportRect() const;
@@ -833,6 +837,7 @@
LayoutPoint m_layoutViewportOrigin;
std::optional<LayoutRect> m_layoutViewportOverrideRect;
+ std::optional<LayoutRect> m_unstableLayoutViewportRect;
unsigned m_deferSetNeedsLayoutCount;
bool m_setNeedsLayoutWasDeferred;
Modified: trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp (219319 => 219320)
--- trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp 2017-07-11 02:52:07 UTC (rev 219319)
+++ trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp 2017-07-11 03:40:59 UTC (rev 219320)
@@ -377,6 +377,8 @@
bool oldProgrammaticScroll = frameView.inProgrammaticScroll();
frameView.setInProgrammaticScroll(programmaticScroll);
+ LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator " << this << " reconcileScrollingState scrollPosition " << scrollPosition << " programmaticScroll " << programmaticScroll << " stable " << inStableState << " " << scrollingLayerPositionAction);
+
std::optional<FloatRect> layoutViewportRect;
WTF::switchOn(layoutViewportOriginOrOverrideRect,
@@ -384,15 +386,21 @@
if (origin)
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
- }
}
);
Modified: trunk/Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp (219319 => 219320)
--- trunk/Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp 2017-07-11 02:52:07 UTC (rev 219319)
+++ trunk/Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp 2017-07-11 03:40:59 UTC (rev 219320)
@@ -65,6 +65,8 @@
if (m_constraints == constraints)
return;
+ LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateFixedNode " << scrollingNodeID() << " updateConstraints with viewport rect " << constraints.viewportRectAtLastLayout());
+
m_constraints = constraints;
setPropertyChanged(ViewportConstraints);
}
@@ -75,7 +77,7 @@
if (layer().representsGraphicsLayer()) {
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) {
case ScrollingLayerPositionAction::Set:
