Bug 173833 - [iOS] Adjust layer hierarchy to handle frame scrolling
Summary: [iOS] Adjust layer hierarchy to handle frame scrolling
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 174097 174130 174134
Blocks: 149264
  Show dependency treegraph
 
Reported: 2017-06-26 08:18 PDT by Frédéric Wang (:fredw)
Modified: 2017-11-24 16:06 PST (History)
4 users (show)

See Also:


Attachments
Create UIScrollView for frames (1.96 KB, patch)
2017-06-27 07:47 PDT, Frédéric Wang (:fredw)
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (relies on RenderLayerBacking's scrolling container) (24.28 KB, patch)
2017-07-05 07:42 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (22.06 KB, patch)
2017-07-06 23:44 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (22.04 KB, patch)
2017-07-27 02:47 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (23.48 KB, patch)
2017-07-28 06:59 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (25.25 KB, patch)
2017-08-02 08:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (23.06 KB, patch)
2017-08-04 08:50 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (22.86 KB, patch)
2017-09-04 09:50 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (21.29 KB, patch)
2017-10-12 09:23 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (RenderLayerCompositor, NonMainFrame) (40.12 KB, patch)
2017-10-13 07:51 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (23.33 KB, patch)
2017-10-16 05:57 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (relies on RenderLayerCompositor's "frame scrolling" layer) (24.63 KB, patch)
2017-10-30 09:32 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (24.57 KB, patch)
2017-11-15 09:37 PST, Frédéric Wang (:fredw)
fred.wang: review?
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (338.87 KB, application/zip)
2017-11-15 12:09 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2017-06-26 08:18:33 PDT
22:56:07 - fredw : My final question was about UIScrollView
22:56:29 - fredw : for overflow:auto you use LayerTypeScrollingLayer which creates UIScrollView
22:57:01 - fredw : but it seems that for frames we'll use LayerTypeTiledBackingTileLayer which currently creates WKCompositingView (which is a UIView)
22:57:25 - fredw : what kind of adjustment should be done here?
22:57:37 - smfr : LayerTypeTiledBackingTileLayer will live inside the scrolling layer
22:57:52 - smfr : so scrolling iframes will need some special treatment for iOS
22:58:13 - smfr : on iOS WkWebView has a UIScrollView inside it already, so the root UIScrollVIew is taken care of
22:58:43 - smfr : for subframes, we'll probably want a LayerTypeScrollingLayer which contains a LayerTypeTiledBackingTileLayer
Comment 1 Frédéric Wang (:fredw) 2017-06-27 07:47:24 PDT
Created attachment 313912 [details]
Create UIScrollView for frames
Comment 2 Simon Fraser (smfr) 2017-06-27 11:17:34 PDT
Comment on attachment 313912 [details]
Create UIScrollView for frames

This doesn't seem right. Scrolling tree stuff for frames should be handled at the FrameView/AsyncScrollingCoordinator level.
Comment 3 Frédéric Wang (:fredw) 2017-06-29 09:23:49 PDT
(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 313912 [details]
> Create UIScrollView for frames
> 
> This doesn't seem right. Scrolling tree stuff for frames should be handled
> at the FrameView/AsyncScrollingCoordinator level.

@Simon: So as I see RenderLayerCompositor sets the scrolling layer by calling
i) ScrollingCoordinator::updateOverflowScrollingNode with the RenderLayerBacking::scrollingLayer() for the overflow node.
ii) ScrollingCoordinator::updateFrameScrollingNode with RenderLayerCompositor::m_scrollLayer for the frame node.

My patch forces the creation of a RenderLayerBacking::scrollingLayer for frames, which indeed does not seem right... IIUC, I should do something similar to RenderLayerCompositor::ensureRootLayer to properly set m_scrollLayer (with a GraphicsLayer::Type::Scrolling)?

But I think I'm missing why I need to change FrameView/AsyncScrollingCoordinator... I see that AsyncScrollingCoordinator::scrollLayerForFrameView and FrameView::layerForScrolling already returns RenderLayerCompositor::scrollLayer so would that be enough to do the change in RenderLayerCompositor?
Comment 4 Frédéric Wang (:fredw) 2017-07-05 07:42:17 PDT
Created attachment 314603 [details]
Patch (relies on RenderLayerBacking's scrolling container)

So after the refactoring of ScrollingStateOverflowScrollingNodeIOS, it is possible to do this relatively small patch for iOS iframe scrolling. However, IIUC the changes in Source/WebCore/rendering to create the layer hierarchy is still not what we really want. So I just uploaded it to discuss the approach.
Comment 5 Frédéric Wang (:fredw) 2017-07-06 23:44:18 PDT
Created attachment 314820 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 313912 [details]
> Create UIScrollView for frames
> 
> This doesn't seem right. Scrolling tree stuff for frames should be handled
> at the FrameView/AsyncScrollingCoordinator level.

@Simon: So this is the same as attachment 314603 [details], but I am no longer forcing the creation of a scrolling layer in RenderLayerBacking. Instead, I'm just modifying the already-created RenderLayerCompositor's "frame scrolling" layer so that it implies the creation of an UIScrollView. That approach seems to work too but I don't know whether that's closer to what you had in mind.
Comment 6 Frédéric Wang (:fredw) 2017-07-27 02:47:37 PDT
Created attachment 316540 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

Rebasing...
Comment 7 Frédéric Wang (:fredw) 2017-07-28 06:59:09 PDT
Created attachment 316636 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

Updated to take into account Fixed and Sticky nodes. However, for now we see the same artifacts as in bug 154399.
Comment 8 Frédéric Wang (:fredw) 2017-08-02 08:34:42 PDT
Created attachment 316960 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

(In reply to Frédéric Wang (:fredw) from comment #7)
> Created attachment 316636 [details]
> Patch (relies on RenderLayerCompositor's "frame scrolling" layer)
> 
> Updated to take into account Fixed and Sticky nodes. However, for now we see
> the same artifacts as in bug 154399.

Another update after the progress in bug 154399 to eliminate the flickering bug.
Comment 9 Frédéric Wang (:fredw) 2017-08-04 08:50:25 PDT
Created attachment 317250 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)
Comment 10 Frédéric Wang (:fredw) 2017-09-04 09:50:00 PDT
Created attachment 319857 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)
Comment 11 Frédéric Wang (:fredw) 2017-10-12 09:23:06 PDT
Created attachment 323531 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

Rebasing ; I moved the changes to Source/WebCore/page/scrolling/ScrollingTree.cpp into bug 178211.
Comment 12 Frédéric Wang (:fredw) 2017-10-13 07:51:57 PDT
Created attachment 323670 [details]
Patch (RenderLayerCompositor, NonMainFrame)

This is an alternative approach, relying on RenderLayerCompositor but only creating a special NonMainFrameScrollingNodeIOS class for non-main frame so that latter class is more simple. It does not really seem to solve existing bugs.
Comment 13 Frédéric Wang (:fredw) 2017-10-16 05:57:55 PDT
Created attachment 323891 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)
Comment 14 Frédéric Wang (:fredw) 2017-10-30 09:32:03 PDT
Created attachment 325355 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

Rebasing...
Comment 15 Frédéric Wang (:fredw) 2017-11-15 09:37:08 PST
Created attachment 326991 [details]
Patch

I'm rebasing the patch. I'm asking for review to get some feedback on the approach, even if I'm aware of some serious bugs regarding sync between UI and Web processes. See also bug 179125 and bug 179172 for some comparison with overflow scrolling.
Comment 16 Build Bot 2017-11-15 12:09:48 PST
Comment on attachment 326991 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 17 Build Bot 2017-11-15 12:09:50 PST
Created attachment 327009 [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.6
Comment 18 Ali Juma 2017-11-24 16:06:15 PST
Comment on attachment 326991 [details]
Patch

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

Thanks for working on this! I experimented a bit with this patch and have a few questions/comments.

I was mostly experimenting with https://people.igalia.com/fwang/ios/scrollable-iframes.html

I noticed that if I scroll an iframe all the way, then pinch zoom (say zoom in and then zoom out a bit), the iframe can entirely (or partially) disappear (looks like it's winding up at an invalid scroll offset). So perhaps non-zero scroll offsets aren't getting scaled correctly when there's a page scale change.

For the jittering in the position:fixed example, I think there are a few issues. First, in ScrollingTreeScrollingNodeDelegateIOS::updateChildNodesAfterScroll, when scrollingNode() is a frame scrolling node, we need to use that node as |frameScrollingNode| rather than the enclosingFrameNode(), so that we use the frame's own fixedPositionRect rather than the parent frame's rect. After making that change and the changes suggested below in ScrollingTreeRemoteFrameScrollingNodeIOS.{h, cpp}, I no longer see any jitter when scrolling the position:fixed example.

I also tried the three Visual Viewport API layout tests on iOS that had been failing because of iframe issues, and two of them now pass (viewport-unscaled-size-iframe.html still fails, but that seems to be because the test is expecting scrollable iframes to have non-overlay scrollbars, so it might be a bad test rather than a problem with this patch).

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1621
> +            m_scrollLayer->setSize(frameView.sizeForVisibleContent());

Is this needed only so that the ScrollingNode gets the right scrollable area size? Could this happen directly in AsyncScrollingCoordinator instead (say in ::frameViewLayoutUpdated)? How does the main frame node get the right value currently?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3285
> +#endif

Is the #if needed here? (Does it hurt to just set the type unconditionally?)

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.h:52
> +    void updateLayersAfterViewportChange(const WebCore::FloatRect& fixedPositionRect, double scale) override { }

I think we at least need the logic from the parent class here. It seems to be used for updating fixed pos and sticky pos things with the new fixed pos rect after a commit from the WebProcess. We might also need to call m_scrollingNodeDelegate->updateLayersAfterAncestorChange.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.h:56
> +    void handleWheelEvent(const WebCore::PlatformWheelEvent&) override { }

The parent class already defines this the same way.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.h:59
> +};

We also need:
void updateLayersAfterDelegatedScroll(const FloatPoint&) override;
and make that call m_scrollingNodeDelegate->updateChildNodesAfterScroll().

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.mm:82
> +        m_scrollingNodeDelegate->updateLayersAfterAncestorChange(changedNode, fixedPositionRect, cumulativeDelta);

This needs to pass the frame's own fixedPositionRect (i.e. this->fixedPositionRect()) rather than the one it receives (which is for the parent frame).