Summary: | getBoundingClientRects not updated for programmatic scrolls | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, buildbot, simon.fraser, thorton, webkit-bug-importer, zalan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2017-07-14 23:27:18 PDT
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. Created attachment 315567 [details]
Patch
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? Created attachment 315884 [details]
Patch
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). 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 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
|