Changeset 219320 in webkit


Ignore:
Timestamp:
Jul 10, 2017, 8:40:59 PM (8 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:
2 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

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

  • trunk/Source/WebCore/ChangeLog

    r219319 r219320  
     12017-07-10  Simon Fraser  <simon.fraser@apple.com>
     2
     3        [WK2 iOS] REGRESSION (r216803) During momentum scroll, getBoundingClientRect returns wrong coordinates (missing images on pinterest, elle.com and many other sites)
     4        https://bugs.webkit.org/show_bug.cgi?id=174286
     5        rdar://problem/32864180
     6
     7        Reviewed by Dean Jackson.
     8
     9        r216803 made getBoundingClientRects relative to the layout viewport, but when scrolling we
     10        only update that on stable viewport updates (at the end of the scroll). This meant that during
     11        unstable updates, getBoundingClientRects() used a "frozen" viewport origin so things on-screen
     12        would appear to be off-screen, causing sites to fail to dynamically load images etc. when
     13        scrolling.
     14
     15        Fix by pushing an optional "unstable" layout viewport rect onto FrameView, which gets used by
     16        FrameView::documentToClientOffset(). This is cleared when we do a stable update.
     17
     18        This is a short-term solution. Longer term, I would prefer to always call setLayoutViewportOverrideRect(),
     19        but fix the scrolling tree logic to work correctly in this case.
     20
     21        Add a bit more scrolling logging.
     22
     23        Test: fast/visual-viewport/ios/get-bounding-client-rect-unstable.html
     24
     25        * page/FrameView.cpp:
     26        (WebCore::FrameView::setUnstableLayoutViewportRect):
     27        (WebCore::FrameView::documentToClientOffset):
     28        * page/FrameView.h:
     29        * page/scrolling/AsyncScrollingCoordinator.cpp:
     30        (WebCore::AsyncScrollingCoordinator::reconcileScrollingState):
     31        * page/scrolling/ScrollingStateFixedNode.cpp:
     32        (WebCore::ScrollingStateFixedNode::updateConstraints):
     33        (WebCore::ScrollingStateFixedNode::reconcileLayerPositionForViewportRect):
     34
    1352017-07-10  John Wilander  <wilander@apple.com>
    236

  • trunk/Source/WebCore/page/FrameView.cpp

    r219121 r219320  
    19191919}
    19201920
     1921void FrameView::setUnstableLayoutViewportRect(std::optional<LayoutRect> rect)
     1922{
     1923    if (rect == m_unstableLayoutViewportRect)
     1924        return;
     1925
     1926    m_unstableLayoutViewportRect = rect;
     1927}
     1928
    19211929LayoutSize FrameView::baseLayoutViewportSize() const
    19221930{
    … …  
    49064914FloatSize FrameView::documentToClientOffset() const
    49074915{
    4908     FloatSize clientOrigin = frame().settings().visualViewportEnabled() ? -toFloatSize(layoutViewportRect().location()) : -toFloatSize(visibleContentRect().location());
     4916    FloatSize clientOrigin;
     4917   
     4918    if (frame().settings().visualViewportEnabled())
     4919        clientOrigin = -toFloatSize(m_unstableLayoutViewportRect ? m_unstableLayoutViewportRect.value().location() : layoutViewportRect().location());
     4920    else
     4921        clientOrigin = -toFloatSize(visibleContentRect().location());
    49094922
    49104923    // Layout and visual viewports are affected by page zoom, so we need to factor that out.

  • trunk/Source/WebCore/page/FrameView.h

    r218982 r219320  
    259259    // Used with delegated scrolling (i.e. iOS).
    260260    WEBCORE_EXPORT void setLayoutViewportOverrideRect(std::optional<LayoutRect>);
     261    // If set, overrides m_layoutViewportOverrideRect. Only used during unstable scroll updates, so that "client" coordinates are
     262    // computed with an origin that changes along with the scroll position.
     263    // FIXME: This is ugly; would be better to be able to make setLayoutViewportOverrideRect() callable during unstable updates.
     264    WEBCORE_EXPORT void setUnstableLayoutViewportRect(std::optional<LayoutRect>);
    261265
    262266    // These are in document coordinates, unaffected by page scale (but affected by zooming).
    … …  
    834838    LayoutPoint m_layoutViewportOrigin;
    835839    std::optional<LayoutRect> m_layoutViewportOverrideRect;
     840    std::optional<LayoutRect> m_unstableLayoutViewportRect;
    836841
    837842    unsigned m_deferSetNeedsLayoutCount;

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

    r217737 r219320  
    378378    frameView.setInProgrammaticScroll(programmaticScroll);
    379379
     380    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator " << this << " reconcileScrollingState scrollPosition " << scrollPosition << " programmaticScroll " << programmaticScroll << " stable " << inStableState << " " << scrollingLayerPositionAction);
     381
    380382    std::optional<FloatRect> layoutViewportRect;
    381383
    … …  
    385387                frameView.setBaseLayoutViewportOrigin(LayoutPoint(origin.value()), FrameView::TriggerLayoutOrNot::No);
    386388        }, [&frameView, &layoutViewportRect, inStableState, visualViewportEnabled = visualViewportEnabled()](std::optional<FloatRect> overrideRect) {
     389            if (!overrideRect)
     390                return;
     391       
    387392            layoutViewportRect = overrideRect;
    388             if (overrideRect && inStableState) {
    389                 if (visualViewportEnabled)
     393            if (visualViewportEnabled) {
     394                if (inStableState) {
    390395                    frameView.setLayoutViewportOverrideRect(LayoutRect(overrideRect.value()));
     396                    frameView.setUnstableLayoutViewportRect(std::nullopt);
     397                } else
     398                    frameView.setUnstableLayoutViewportRect(LayoutRect(layoutViewportRect.value()));
     399            }
    391400#if PLATFORM(IOS)
    392                 else
    393                     frameView.setCustomFixedPositionLayoutRect(enclosingIntRect(overrideRect.value()));
     401            else if (inStableState)
     402                frameView.setCustomFixedPositionLayoutRect(enclosingIntRect(overrideRect.value()));
    394403#endif
    395             }
    396404        }
    397405    );

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

    r216478 r219320  
    6666        return;
    6767
     68    LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateFixedNode " << scrollingNodeID() << " updateConstraints with viewport rect " << constraints.viewportRectAtLastLayout());
     69
    6870    m_constraints = constraints;
    6971    setPropertyChanged(ViewportConstraints);
    … …  
    7678        GraphicsLayer* graphicsLayer = static_cast<GraphicsLayer*>(layer());
    7779
    78         LOG_WITH_STREAM(Compositing, stream << "ScrollingStateFixedNode::reconcileLayerPositionForViewportRect setting position of layer " << graphicsLayer->primaryLayerID() << " to " << position);
     80        LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateFixedNode " << scrollingNodeID() <<" reconcileLayerPositionForViewportRect " << action << " position of layer " << graphicsLayer->primaryLayerID() << " to " << position);
    7981       
    8082        switch (action) {
Note: See TracChangeset for help on using the changeset viewer.