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-09-04 09:50 PDT (History)
2 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

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)