Bug 174538 – getBoundingClientRects not updated for programmatic scrolls

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

In current iOS builds, focusing the search bar on baidu.com causes the page to scroll to the wrong offset.

This is s side-effect of getBoundingClientRect being wrong after a programmatic scroll.

http://trac.webkit.org/changeset/219320 was an earlier, but misguided attempt to fix a related problem.

[Simon Fraser (smfr)]

rdar://problem/33049012

[Simon Fraser (smfr)]

I am going to revert r219320 and fix it in a different way. That revision added a shadow layout viewport origin that was updated during unstable scrolls. However, it did not get updated during programmatic scrolls (which originate in the web process).

A better approach is to just always update the layout viewport so that client coordinates in the web process are always computed with an up-to-date layout viewport origin. However, this has to be done in a way that doesn't break the scrolling tree.

The scrolling tree is sensitive to the layout viewport, because that rect is used when computing fixed and sticky constraints. We need to preserve the relationship between the "layout position at last layout" and "viewport at last layout" when updating constraints. However, there are code paths that update the scrolling state tree after a layout viewport change, but without a corresponding GraphicsLayer position change; this includes the updateScrollCoordinatedLayersAfterFlushIncludingSubframes() that happens on every layer flush.

Thus we need to store a "snapshot" of the layout viewport rect that was used to compute layer positions for scrolling tree nodes, and use that snapshot when re-computing viewport constraints. Secondarily, we must update that snapshot when we slam in new layer positions via the set/sync code path in reconcileViewportConstrainedLayerPositions(), since that's another situation in which layer positions are being update with a specific viewport rect.

[Simon Fraser (smfr)]

Created attachment 315567 [details]
Patch

[Tim Horton]

Comment on attachment 315567 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315567&action=review

> Source/WebCore/page/FrameView.cpp:4912
> +    FloatSize clientOrigin = frame().settings().visualViewportEnabled() ? -toFloatSize(layoutViewportRect().location()) : -toFloatSize(visibleContentRect().location());

It is possible to rearrange this to duplicate less code between the conditions :)

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:510
> +    LOG_WITH_STREAM(Scrolling, stream << getpid() << " AsyncScrollingCoordinator::reconcileViewportConstrainedLayerPositions for viewport rect " << viewportRect);

Logging that goes to os_log usually gets the pid in one of the earlier columns... I guess you're duplicating it here so it shows up if you're attached/watching the console you launched from?

[Simon Fraser (smfr)]

Created attachment 315884 [details]
Patch

[Simon Fraser (smfr)]

I discovered some issues with the previous patch, namely jumpy top bars on some sites (nytimes.com). In fixing that, discovered that we do compositing updates where we may not update layer positions for all fixed elements, so the approach of snapshotting a layout viewport on the scrollling coordinator was not valid.

New approach is to have RenderLayerCompositor not recompute everything on all scrolling and fixed nodes when it update scrolling state; that way, we only update fixed/sticky constraints when we've just computed layer positions, so we know the layer position/viewport rect relationship is valid.

Also discovered issues with bottom-fixed bars, related to updating the layout viewport during interactive obscured inset changing (i.e. when toolbars animate in and out), since the viewport height is changing then.

Also revealed an issue where, after zoom, we would consider fixed-bottom bars outside the document bounds, and therefore make them not composited so they disappear. Turns out this was caused by FrameView::unscaledMaximumScrollPosition() on iOS returning a value that was zoom-sensitive, despite the name (filed https://bugs.webkit.org/show_bug.cgi?id=174648).

[Build Bot]

Comment on attachment 315884 [details]
Patch

Attachment 315884 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4145851

New failing tests:
fast/dom/elementFromPoint-relative-to-viewport.html

[Build Bot]

Created attachment 315891 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Simon Fraser (smfr)]

https://trac.webkit.org/r219668