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 180353 180473 181841 182533 182785
Blocks: 149264 182868 192303
  Show dependency treegraph
 
Reported: 2017-06-26 08:18 PDT by Frédéric Wang (:fredw)
Modified: 2018-12-03 00:43 PST (History)
7 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)
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
Patch (WIP) (26.65 KB, patch)
2017-11-29 06:43 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (29.92 KB, patch)
2017-12-01 08:02 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.77 MB, application/zip)
2017-12-01 08:45 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.41 MB, application/zip)
2017-12-01 09:25 PST, Build Bot
no flags Details
Testcase subframe VS overflow nodes (934 bytes, text/html)
2017-12-06 08:50 PST, Frédéric Wang (:fredw)
no flags Details
Graphic Layer output for subframes (4.67 KB, text/plain)
2017-12-06 08:51 PST, Frédéric Wang (:fredw)
no flags Details
Graphic Layer output for overflow node (2.43 KB, text/plain)
2017-12-06 08:51 PST, Frédéric Wang (:fredw)
no flags Details
Patch (30.52 KB, patch)
2017-12-18 09:21 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (33.82 KB, patch)
2017-12-19 07:02 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (51.45 KB, patch)
2018-01-10 06:40 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (51.46 KB, patch)
2018-01-10 06:43 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.68 MB, application/zip)
2018-01-10 07:12 PST, Build Bot
no flags Details
Patch (52.95 KB, patch)
2018-01-10 07:23 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.24 MB, application/zip)
2018-01-10 08:37 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (2.31 MB, application/zip)
2018-01-10 16:18 PST, Build Bot
no flags Details
Patch (55.17 KB, patch)
2018-01-11 08:00 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (72.80 KB, patch)
2018-01-12 16:11 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (84.00 KB, patch)
2018-01-13 00:15 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.19 MB, application/zip)
2018-01-13 01:46 PST, Build Bot
no flags Details
Patch (84.00 KB, patch)
2018-01-15 00:28 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (use offsetFromRenderer for hit testing) (1.58 KB, patch)
2018-01-18 07:03 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (102.13 KB, patch)
2018-01-19 09:30 PST, Frédéric Wang (:fredw)
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (100.36 KB, patch)
2018-02-06 09:04 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (update position/size in RenderLayerCompositor) (32.45 KB, patch)
2018-02-08 08:43 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (98.53 KB, patch)
2018-02-09 08:29 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Testcase for hit testing (11.21 KB, text/html)
2018-02-09 08:30 PST, Frédéric Wang (:fredw)
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (8.41 MB, application/zip)
2018-02-09 10:04 PST, Build Bot
no flags Details
Patch (98.65 KB, patch)
2018-02-12 02:16 PST, Frédéric Wang (:fredw)
simon.fraser: review-
Details | Formatted Diff | Diff
Patch for EWS (126.97 KB, patch)
2018-02-15 09:31 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (420.56 KB, application/zip)
2018-02-15 10:20 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (2.36 MB, application/zip)
2018-02-15 10:36 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.06 MB, application/zip)
2018-02-15 11:14 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.75 MB, application/zip)
2018-02-15 12:57 PST, Build Bot
no flags Details
Patch for EWS (116.57 KB, patch)
2018-02-15 15:15 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.49 MB, application/zip)
2018-02-15 16:24 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.08 MB, application/zip)
2018-02-15 17:00 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.82 MB, application/zip)
2018-02-15 17:19 PST, Build Bot
no flags Details
Patch for EWS (includes 182785) (133.26 KB, patch)
2018-02-16 02:16 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of bug 182785) (102.61 KB, patch)
2018-02-16 03:49 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of bug 182785) (102.26 KB, patch)
2018-05-09 23:32 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of 182785) (102.55 KB, patch)
2018-09-05 07:02 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (101.35 KB, patch)
2018-09-10 02:55 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (101.92 KB, patch)
2018-11-15 07:02 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.37 MB, application/zip)
2018-11-15 08:10 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.32 MB, application/zip)
2018-11-15 09:06 PST, Build Bot
no flags Details
Patch (103.72 KB, patch)
2018-11-20 03:59 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.44 MB, application/zip)
2018-11-20 05:06 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (2.23 MB, application/zip)
2018-11-20 05:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.34 MB, application/zip)
2018-11-20 06:40 PST, Build Bot
no flags Details
Patch (110.26 KB, patch)
2018-11-20 07:41 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (109.99 KB, patch)
2018-11-30 00:26 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (attempt to remove the changes to windowToContents/contentsToWindow) (101.03 KB, patch)
2018-11-30 00:29 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (109.95 KB, patch)
2018-11-30 11:52 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (attempt to remove the changes to windowToContents/contentsToWindow) (100.99 KB, patch)
2018-11-30 11:53 PST, Frédéric Wang (:fredw)
fred.wang: review?
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)
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).
Comment 19 Frédéric Wang (:fredw) 2017-11-29 06:37:02 PST
Comment on attachment 326991 [details]
Patch

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

Thanks for the feedback Ali, I'll upload my latest changes. Indeed position:fixed is better now but position:sticky no longer works. I need to study this a bit more.

>> 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?

Yes, I think there was a bug with the size. Note that I put three such calls, but I was not sure whether all of them are needed. At the moment it's more a hack, I would need to check it.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3285
>> +#endif
> 
> Is the #if needed here? (Does it hurt to just set the type unconditionally?)

Mmh, I think I don't exactly remember but I believe it was a conservative approach to avoid breaking things. I would need to check.

>> 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.

OK, I copied that from the ScrollingTreeOverflowScrollingNodeIOS class but indeed that breaks the case of !m_scrollingNodeDelegate. Actually, I think I should do that when we have a m_scrollingNodeDelegate too as m_scrollingNodeDelegate->updateLayersAfterAncestorChange does not really make sense here?

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.h:56
>> +    void handleWheelEvent(const WebCore::PlatformWheelEvent&) override { }
> 
> The parent class already defines this the same way.

Removed.

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.h:59
>> +};
> 
> We also need:
> void updateLayersAfterDelegatedScroll(const FloatPoint&) override;
> and make that call m_scrollingNodeDelegate->updateChildNodesAfterScroll().

Done.

>> 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).

Good point, also I think we can just ASSERT(m_scrollingNodeDelegate) since the main frame does not have ancestor (and updateLayersAfterAncestorChange is not implemented for the parent class).

I've also modified ScrollingNode::enclosingFrameNode() so that ScrollingTreeScrollingNodeDelegateIOS::updateChildNodesAfterScroll also use the rect of the frame for ScrollingTreeRemoteFrameScrollingNodeIOS.
Comment 20 Frédéric Wang (:fredw) 2017-11-29 06:43:07 PST
Created attachment 327851 [details]
Patch (WIP)
Comment 21 Frédéric Wang (:fredw) 2017-12-01 08:01:43 PST
Comment on attachment 326991 [details]
Patch

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

I tried to modify AsyncScrollingCoordinator::reconcileScrollingState to match what is done for overflow nodes in RenderLayerBacking::updateGeometry, but I still have sync issues between Web and UI processes.

>>> 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?
> 
> Yes, I think there was a bug with the size. Note that I put three such calls, but I was not sure whether all of them are needed. At the moment it's more a hack, I would need to check it.

Done. This seems to solve the bug I had that sometimes the size is not updated.

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3285
>>> +#endif
>> 
>> Is the #if needed here? (Does it hurt to just set the type unconditionally?)
> 
> Mmh, I think I don't exactly remember but I believe it was a conservative approach to avoid breaking things. I would need to check.

This will choose between the creation of LayerTypeWebLayer or LayerTypeScrollingLayer which behaves differently in the RemoteLayer* classes (currently only used on iOS) and at least in PlayerCALayerCocoa::commonInit so I prefer not too change that on non-iOS platforms for now.
Comment 22 Frédéric Wang (:fredw) 2017-12-01 08:02:28 PST
Created attachment 328092 [details]
Patch
Comment 23 Build Bot 2017-12-01 08:45:13 PST
Comment on attachment 328092 [details]
Patch

Attachment 328092 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5446519

Number of test failures exceeded the failure limit.
Comment 24 Build Bot 2017-12-01 08:45:15 PST
Created attachment 328094 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 25 Build Bot 2017-12-01 09:25:21 PST
Comment on attachment 328092 [details]
Patch

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

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/iframes/overlapped-nested-iframes.html
compositing/iframes/iframe-resize.html
compositing/tiling/tiled-drawing-async-frame-scrolling.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/rtl/rtl-iframe-fixed.html
compositing/iframes/overlapped-iframe-iframe.html
compositing/iframes/overlapped-iframe.html
compositing/visible-rect/iframe-with-layers-outside-viewport.html
compositing/visible-rect/iframe-and-layers.html
compositing/iframes/become-overlapped-iframe.html
compositing/iframes/page-cache-layer-tree.html
compositing/rtl/rtl-iframe-absolute.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/connect-compositing-iframe3.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/leave-compositing-iframe.html
compositing/rtl/rtl-iframe-relative.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/rtl/rtl-iframe-absolute-overflow.html
compositing/iframes/invisible-nested-iframe-show.html
imported/blink/fast/scrolling/fractional-scroll-offset-iframe-fixed-position.html
compositing/iframes/resizer.html
compositing/iframes/connect-compositing-iframe2.html
Comment 26 Build Bot 2017-12-01 09:25:22 PST
Created attachment 328099 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 27 Frédéric Wang (:fredw) 2017-12-06 08:50:36 PST
Created attachment 328573 [details]
Testcase subframe VS overflow nodes
Comment 28 Frédéric Wang (:fredw) 2017-12-06 08:51:22 PST
Created attachment 328574 [details]
Graphic Layer output for subframes
Comment 29 Frédéric Wang (:fredw) 2017-12-06 08:51:44 PST
Created attachment 328575 [details]
Graphic Layer output for overflow node
Comment 30 Frédéric Wang (:fredw) 2017-12-06 08:58:10 PST
(In reply to Simon Fraser (smfr) from comment #2)
> This doesn't seem right. Scrolling tree stuff for frames should be handled
> at the FrameView/AsyncScrollingCoordinator level.

@Simon: In the latest patch, I'm creating the graphic layers in RenderLayerCompositor and updating them in AsyncScrollingCoordinator. I'm trying to copy the code from RenderLayerBacking::updageGeometry() into AsyncScrollingCoordinator::reconcileScrollingState(). Debugging the code and dumping the graphic layer tree of attachment 328573 [details], I'm getting similar result (attachment 328574 [details] and attachment 328575 [details]). However, that still does solve the synchronization issue I'm encountering (and that even makes the rendering worse). What do you think of the current approach? Is there something special I should consider for subframes?
Comment 31 Ali Juma 2017-12-15 14:07:31 PST
Comment on attachment 328092 [details]
Patch

I tried out the latest patch with the examples on https://people.igalia.com/fwang/ios/scrollable-iframes.html.

The problem with position:sticky is that in ScrollingTreeStickyNode::updateLayersAfterAncestorChange, we need to use the current scroll position as the origin of the constrainingRect when we the parent() node is a subframe node. Currently we just use the fixedPositionRect that's passed in as the constrainingRect, but for subframes that rect's origin is the last committed scroll position (for main frames, fixedPositionRect is computed separately, not using the node's fixedPositionRect() method). The code change needed looks something like:

     if (is<ScrollingTreeOverflowScrollingNode>(*parent())) {
     ....
+    } else if (is<ScrollingTreeFrameScrollingNode>(*parent()) && parent()->parent()) {
+        constrainingRect = FloatRect(downcast<ScrollingTreeFrameScrollingNode>(*parent()).scrollPosition(), fixedPositionRect.size());
+        adjustStickyLayer = true;
     } else if (is<ScrollingTreeFrameScrollingNode>(*parent())) {

There's also an issue with content not rendering past a certain scroll offset (e.g. when scrolling the wikipedia example at the bottom of the page, or when zoomed in and scrolling the other examples). That's because of a bug in GraphicsLayerCA::computeVisibleAndCoverageRect, which is causing the boundsOrigin value of a layer (which represents the scroll offset) to have no effect on the layer's coverage rect and on its descendants' visible and coverage rects if the layer with boundsOrigin doesn't maskToBounds (that's why it's fine for overflow scroll -- the layer with non-zero boundsOrigin happens to also maskToBounds). So the visible and coverage rects always had origin (0, 0) regardless of the scroll offset. Here's a change that fixes that:

     if (masksToBounds()) {
     ....
+    } else if (boundsOrigin != FloatPoint()) {
+        state.move(-boundsOrigin.x(), -boundsOrigin.y());
     }

It'd be great to have a layout test for this.

For the issue with programmatic scroll making content disappear:

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.mm:74
> +    ScrollingTreeFrameScrollingNodeIOS::commitStateAfterChildren(stateNode);

This call should happen only when we don't have an m_scrollingNodeDelegate (that is, we shouldn't make this call for subframes). That change fixes programmatic scroll. The problem was that this call leads to ScrollingTreeScrollingNodeDelegateIOS::setScrollLayerPosition, which calls setPosition on its m_scrollLayer, but that layer's position isn't supposed to change when we scroll a subframe (so by changing that layer's position, the contents were getting moved outside the frame's bounds). The call to m_scrollingNodeDelegate->commitStateAfterChildren is what handles updating the UIScrollView's contentOffset with the new scroll position.
Comment 32 Frédéric Wang (:fredw) 2017-12-18 08:19:30 PST
Comment on attachment 328092 [details]
Patch

Thanks Ali. I'll rebase and try to integrate your suggestions.
Comment 33 Frédéric Wang (:fredw) 2017-12-18 09:21:03 PST
Created attachment 329652 [details]
Patch

Here is a new version that integrates Ali's change. Using "parent()" to distinguish main frame VS subframes starts to be a bit hacky, so I've introduced isMainFrame(). I think we are close to have something ready for review (modulo making tests pass and writing new ones of course). The serious remaining issue is the one I've been trying to debug for a while by comparing graphic layers of overflow and subframe nodes: Basically on the Wikipedia page ( last example of https://people.igalia.com/fwang/ios/scrollable-iframes.html ) you can touch the links, the three-bars button to display the menu etc However, if you scroll down the page a bit then the scroll shift is not taken into account and you have to touch these elements at they former locations in order to activate them.

On the programmatic scroll testcase, I also noticed one can scroll the content outside the scrolling bounds but that might just be bug 179735.
Comment 34 Ali Juma 2017-12-18 15:03:52 PST
(In reply to Frédéric Wang (:fredw) from comment #33)

> On the programmatic scroll testcase, I also noticed one can scroll the
> content outside the scrolling bounds but that might just be bug 179735.

For this, I think ScrollingTreeScrollingNodeDelegateIOS::commitStateAfterChildren needs a case for ScrollingStateScrollingNode::RequestedScrollPosition, where it gets the state node’s requestedScrollPosition(), and calls requestedScrollPosition().constrainedBetween() passing the corresponding tree node’s minimum and maximum scroll positions to get the clamped position. Then it can set the clamped position on the UIScrollView (like it does in the ScrollingStateScrollingNode::ScrollPosition case), and then call scrollingTree().scrollingTreeNodeDidScroll with the new position so that the clamped position gets sent to the WebProcess.
Comment 35 Frédéric Wang (:fredw) 2017-12-19 03:38:19 PST
(In reply to Ali Juma from comment #34)
> (In reply to Frédéric Wang (:fredw) from comment #33)
> 
> > On the programmatic scroll testcase, I also noticed one can scroll the
> > content outside the scrolling bounds but that might just be bug 179735.
> 
> For this, I think
> ScrollingTreeScrollingNodeDelegateIOS::commitStateAfterChildren needs a case
> ...

OK, I'll take a look to this. Just checked the testcases and it seems this is another instance of a bug that does not happen with overflow node, so we can not handle it in a preliminary bug.
Comment 36 Frédéric Wang (:fredw) 2017-12-19 07:02:39 PST
Created attachment 329761 [details]
Patch

New version including suggestion from comment 34.
Comment 37 Frédéric Wang (:fredw) 2018-01-10 06:40:52 PST
Created attachment 330895 [details]
Patch
Comment 38 Build Bot 2018-01-10 06:43:01 PST
Attachment 330895 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.mm:85:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.mm:86:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.mm:88:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.mm:89:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 4 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 39 Frédéric Wang (:fredw) 2018-01-10 06:43:04 PST
Created attachment 330896 [details]
Patch
Comment 40 Build Bot 2018-01-10 07:12:18 PST
Comment on attachment 330896 [details]
Patch

Attachment 330896 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6019975

Number of test failures exceeded the failure limit.
Comment 41 Build Bot 2018-01-10 07:12:20 PST
Created attachment 330904 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 42 Frédéric Wang (:fredw) 2018-01-10 07:23:00 PST
Created attachment 330905 [details]
Patch
Comment 43 Build Bot 2018-01-10 08:37:39 PST
Comment on attachment 330905 [details]
Patch

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

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
Comment 44 Build Bot 2018-01-10 08:37:41 PST
Created attachment 330912 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 45 Build Bot 2018-01-10 16:18:15 PST
Comment on attachment 330905 [details]
Patch

Attachment 330905 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6026441

New failing tests:
webgl/1.0.2/conformance/uniforms/uniform-default-values.html
Comment 46 Build Bot 2018-01-10 16:18:17 PST
Created attachment 330984 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 47 Frédéric Wang (:fredw) 2018-01-11 08:00:03 PST
Created attachment 331053 [details]
Patch
Comment 48 Frédéric Wang (:fredw) 2018-01-12 16:11:42 PST
Created attachment 331249 [details]
Patch
Comment 49 Frédéric Wang (:fredw) 2018-01-13 00:15:26 PST
Created attachment 331278 [details]
Patch
Comment 50 Frédéric Wang (:fredw) 2018-01-13 00:27:33 PST
@Simon, @Ali: I uploaded a version for review with test changes. Like for "overflow: auto" nodes, hit testing is not fully implemented but I did not intend to do it in that bug anyway. The scroll offset does not seem to be taken into account for determining the location of a target (e.g. link) when activating it with touch gesture, despite my attempt to set offsetFromRenderer. I'll try to debug it more next week, but maybe this can be handled in a follow-up bug as the patch is already starting to be big.

(In reply to Ali Juma from comment #31)
> There's also an issue with content not rendering past a certain scroll
> offset (e.g. when scrolling the wikipedia example at the bottom of the page,
> or when zoomed in and scrolling the other examples). That's because of a bug
> in GraphicsLayerCA::computeVisibleAndCoverageRect, which is causing the
> boundsOrigin value of a layer (which represents the scroll offset) to have
> no effect on the layer's coverage rect and on its descendants' visible and
> coverage rects if the layer with boundsOrigin doesn't maskToBounds (that's
> why it's fine for overflow scroll -- the layer with non-zero boundsOrigin
> happens to also maskToBounds). So the visible and coverage rects always had
> origin (0, 0) regardless of the scroll offset. Here's a change that fixes
> that:
> 
>      if (masksToBounds()) {
>      ....
> +    } else if (boundsOrigin != FloatPoint()) {
> +        state.move(-boundsOrigin.x(), -boundsOrigin.y());
>      }
> 
> It'd be great to have a layout test for this.

fast/scrolling/ios/scroll-iframe.html now verifies this case. However, I wonder whether the scrolling layers of subframes should just behave the same as overflow node and maskToBounds?
Comment 51 Build Bot 2018-01-13 01:46:35 PST
Comment on attachment 331278 [details]
Patch

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

New failing tests:
fast/scrolling/ios/scroll-iframe.html
Comment 52 Build Bot 2018-01-13 01:46:36 PST
Created attachment 331280 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 53 Frédéric Wang (:fredw) 2018-01-15 00:28:02 PST
Created attachment 331322 [details]
Patch

Same patch but adjusting copyright year & delay for scroll-iframe.html
Comment 54 Ali Juma 2018-01-15 13:59:43 PST
Comment on attachment 331322 [details]
Patch

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

> LayoutTests/fast/scrolling/ios/scroll-iframe.html:48
> +             setTimeout(() => { testRunner.notifyDone() }, 3000);

Is the setTimeout here because there's a delay between the swipe finishing and the new content being rendered, or because the swipe is animated so we don't know when it finishes? Would it make sense to file a bug for adding whatever test API we're missing so that we don't need to use setTimeout?
Comment 55 Ali Juma 2018-01-15 14:04:14 PST
(In reply to Frédéric Wang (:fredw) from comment #50)
> @Simon, @Ali: I uploaded a version for review with test changes. Like for
> "overflow: auto" nodes, hit testing is not fully implemented but I did not
> intend to do it in that bug anyway. The scroll offset does not seem to be
> taken into account for determining the location of a target (e.g. link) when
> activating it with touch gesture, despite my attempt to set
> offsetFromRenderer. I'll try to debug it more next week, but maybe this can
> be handled in a follow-up bug as the patch is already starting to be big.

Leaving the hit-testing work for a follow-up sounds like a good idea to me. The patch seems to be in pretty good shape I think.

> > It'd be great to have a layout test for this.
> 
> fast/scrolling/ios/scroll-iframe.html now verifies this case. However, I
> wonder whether the scrolling layers of subframes should just behave the same
> as overflow node and maskToBounds?

If you mean combining m_clipLayer and m_scrollLayer in RenderLayerCompositor, one issue to figure out is that on non-iOS, the scroll offset is stored as m_scrollLayer's position, but m_clipLayer's position is used to account for contentInset and for RTL scrollbars (see RenderLayerCompositor::positionForClipLayer). So you'd need to untangle that before merging the two layers.
Comment 56 Frédéric Wang (:fredw) 2018-01-15 23:06:29 PST
Comment on attachment 331322 [details]
Patch

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

>> LayoutTests/fast/scrolling/ios/scroll-iframe.html:48
>> +             setTimeout(() => { testRunner.notifyDone() }, 3000);
> 
> Is the setTimeout here because there's a delay between the swipe finishing and the new content being rendered, or because the swipe is animated so we don't know when it finishes? Would it make sense to file a bug for adding whatever test API we're missing so that we don't need to use setTimeout?

After the swipe gesture, the scrolling of the frame starts, slowly decelerates and stops. The scrollbars disappear when the final position is reached. Currently, we can't know exactly what will be the final position (that's why the tests are not as accurate as in the programming case) nor when scrolling will stop (that's why there is a delay). I wanted to move getSwipeUIScript() to resource/ui-helper.js in any case. Maybe we can improve the API a bit so that we can specify a target and detect when it stops scrolling. Not sure if we can do better for the final scroll position without recalculating exactly inertia and so on.
Comment 57 Frédéric Wang (:fredw) 2018-01-18 07:03:50 PST
Created attachment 331618 [details]
Patch (use offsetFromRenderer for hit testing)

(In reply to Ali Juma from comment #55)
> (In reply to Frédéric Wang (:fredw) from comment #50)
> > @Simon, @Ali: I uploaded a version for review with test changes. Like for
> > "overflow: auto" nodes, hit testing is not fully implemented but I did not
> > intend to do it in that bug anyway. The scroll offset does not seem to be
> > taken into account for determining the location of a target (e.g. link) when
> > activating it with touch gesture, despite my attempt to set
> > offsetFromRenderer. I'll try to debug it more next week, but maybe this can
> > be handled in a follow-up bug as the patch is already starting to be big.
> 
> Leaving the hit-testing work for a follow-up sounds like a good idea to me.
> The patch seems to be in pretty good shape I think.

With this small additional patch that takes into account the offsetFromRenderer, hit testing seems to be working for me. So I guess I can integrate that change and add more tests for hit testing. Note however that, like for overflow nodes, hit testing is performed using the graphic layers in the Web Process, not just using the scrolling tree in the UI Process. Hence it is a bit different from the effort made for bug 171667. I'm not sure this is really what we want in the long term, but I guess it's good enough for a feature under a development flag. In any case, I have several doubts about the tree adjustments made here (as said above I mostly tried to copy the logic of overflow nodes), so it would be good to get feedback from Simon ;-)
Comment 58 Frédéric Wang (:fredw) 2018-01-19 09:30:17 PST
Created attachment 331743 [details]
Patch
Comment 59 Simon Fraser (smfr) 2018-02-05 13:30:59 PST
Comment on attachment 331743 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:4461
> +#if PLATFORM(IOS)
> +            if (!frame().isMainFrame()) {
> +                if (auto* renderView = frame().contentRenderer()) {
> +                    if (auto* scrolledContentsLayer = renderView->compositor().rootContentLayer())
> +                        point.move(roundedIntSize(scrolledContentsLayer->offsetFromRenderer()));
> +                }
> +            }
> +#endif

Is this here to handle async scrolling? Normally point conversion uses the current state of the DOM, and these offsets should already be reflected in RenderLayer scroll offsets.

> Source/WebCore/page/FrameView.cpp:4492
> +#if PLATFORM(IOS)
> +            if (!frame().isMainFrame()) {
> +                if (auto* renderView = frame().contentRenderer()) {
> +                    if (auto* scrolledContentsLayer = renderView->compositor().rootContentLayer())
> +                        point.move(-roundedIntSize(scrolledContentsLayer->offsetFromRenderer()));
> +                }
> +            }
> +#endif

Ditto.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:188
> +#if PLATFORM(IOS)
> +    if (auto* scrollLayer = scrollLayerForFrameView(frameView))
> +        scrollLayer->setSize(frameView.sizeForVisibleContent());
> +#endif

Seems wrong to set layer size here; layer size should be set in RenderLayerBacking::updateGeometry or similar.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:445
> +    scrollLayer->setPosition(FloatPoint());
> +    FloatSize oldScrollLayerOffset = scrollLayer->offsetFromRenderer();
> +    scrollLayer->setOffsetFromRenderer(FloatSize());

This should also happen near RLB::updateGeometry()

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:495
> +#if PLATFORM(IOS)
> +    IntSize scrollSize = frameView.contentsSize();
> +    scrolledContentsLayer->setPosition(FloatPoint());
> +    if (scrollSize != scrolledContentsLayer->size() || scrollOffsetChanged)
> +        scrolledContentsLayer->setNeedsDisplay();
> +    scrolledContentsLayer->setSize(scrollSize);
> +    LayoutSize scrolledContentsOffset = -toLayoutSize(frameView.scrollPosition());
> +    scrolledContentsLayer->setOffsetFromRenderer(scrolledContentsOffset, GraphicsLayer::DontSetNeedsDisplay);
> +#endif

Again, I don't think setting geometry should be done here.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:656
> +#if PLATFORM(IOS)
> +    auto* scrolledContentsLayer = rootContentLayerForFrameView(frameView);
> +    ASSERT(scrolledContentsLayer);
> +    scrollLayer->setBoundsOrigin(frameView.scrollPosition());
> +    LayoutSize scrolledContentsOffset = -toLayoutSize(frameView.scrollPosition());
> +    scrolledContentsLayer->setOffsetFromRenderer(scrolledContentsOffset, GraphicsLayer::DontSetNeedsDisplay);
> +#else

Ditto.

> Source/WebCore/page/scrolling/ScrollingTreeFrameScrollingNode.h:42
> +    virtual bool isMainFrame() { return m_isMainFrame; }

Method should be const.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1378
> +    } else if (boundsOrigin != FloatPoint())

!boundsOrigin.isZero()

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3295
> +            m_scrollLayer = GraphicsLayer::create(graphicsLayerFactory(), *this, GraphicsLayer::Type::Scrolling);

I don't think we should use GraphicsLayer::Type::Scrolling for the main frame. Does this code run for the main frame?

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.mm:58
> +void ScrollingTreeRemoteFrameScrollingNodeIOS::setIsMainFrame(bool mainFrame)
> +{
> +    if (mainFrame != isMainFrame())
> +        m_scrollingNodeDelegate.reset(mainFrame ? nullptr : new ScrollingTreeScrollingNodeDelegateIOS(*this));
> +}

I don' think a scrolling node would ever dynamically change between a main frame and non-main-frame node, so I think the "isMainFrame" state should come in through the constructor.
Comment 60 Frédéric Wang (:fredw) 2018-02-05 13:51:10 PST
Comment on attachment 331743 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:4461
>> +#endif
> 
> Is this here to handle async scrolling? Normally point conversion uses the current state of the DOM, and these offsets should already be reflected in RenderLayer scroll offsets.

This is to handle hit testing after user scroll. Without that, the scroll offset is not taken into account for hit testing. See comment 50 and test clickElementInsideFrameAfterUserScroll.

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:188
>> +#endif
> 
> Seems wrong to set layer size here; layer size should be set in RenderLayerBacking::updateGeometry or similar.

mmh, I thought you mean in comment 2 that we should not use RenderLayerBacking? Now I'm confused...

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3295
>> +            m_scrollLayer = GraphicsLayer::create(graphicsLayerFactory(), *this, GraphicsLayer::Type::Scrolling);
> 
> I don't think we should use GraphicsLayer::Type::Scrolling for the main frame. Does this code run for the main frame?

I don't think it's called for the main frame, but will check again.

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.mm:58
>> +}
> 
> I don' think a scrolling node would ever dynamically change between a main frame and non-main-frame node, so I think the "isMainFrame" state should come in through the constructor.

I agree, however I think passing this property to the constructor made things a bit more complicated because we had to introduce new node type. See attachment 323670 [details].
Comment 61 Simon Fraser (smfr) 2018-02-05 14:34:39 PST
(In reply to Frédéric Wang (:fredw) from comment #60)
> Comment on attachment 331743 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331743&action=review
> 
> >> Source/WebCore/page/FrameView.cpp:4461
> >> +#endif
> > 
> > Is this here to handle async scrolling? Normally point conversion uses the current state of the DOM, and these offsets should already be reflected in RenderLayer scroll offsets.
> 
> This is to handle hit testing after user scroll. Without that, the scroll
> offset is not taken into account for hit testing. See comment 50 and test
> clickElementInsideFrameAfterUserScroll.

Is this only an issue on the first touch that stops a scroll? We already get that wrong, but it should be fixed separately. If it's every hit test even when stationary, them something's wrong.

> >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:188
> >> +#endif
> > 
> > Seems wrong to set layer size here; layer size should be set in RenderLayerBacking::updateGeometry or similar.
> 
> mmh, I thought you mean in comment 2 that we should not use
> RenderLayerBacking? Now I'm confused...

The only place we should be setting layer sizes and positions is in RLB::updateGeometry(). What I objected to in that early patch was code in RenderLayer that was about Frame scrolling.

> >> Source/WebCore/rendering/RenderLayerCompositor.cpp:3295
> >> +            m_scrollLayer = GraphicsLayer::create(graphicsLayerFactory(), *this, GraphicsLayer::Type::Scrolling);
> > 
> > I don't think we should use GraphicsLayer::Type::Scrolling for the main frame. Does this code run for the main frame?
> 
> I don't think it's called for the main frame, but will check again.
> 
> >> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNodeIOS.mm:58
> >> +}
> > 
> > I don' think a scrolling node would ever dynamically change between a main frame and non-main-frame node, so I think the "isMainFrame" state should come in through the constructor.
> 
> I agree, however I think passing this property to the constructor made
> things a bit more complicated because we had to introduce new node type. See
> attachment 323670 [details].

I think new types are preferable (maybe call it Subframe* rather than NonMain*).
Comment 62 Frédéric Wang (:fredw) 2018-02-06 09:04:09 PST
Created attachment 333179 [details]
Patch

Rebasing on top of bug 182533.

(In reply to Simon Fraser (smfr) from comment #59)
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1378
> > +    } else if (boundsOrigin != FloatPoint())
> 
> !boundsOrigin.isZero()

It looks like we have FloatPoint::zero() but not isZero().
Comment 63 Frédéric Wang (:fredw) 2018-02-07 06:21:35 PST
Comment on attachment 331743 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3295
>>>> +            m_scrollLayer = GraphicsLayer::create(graphicsLayerFactory(), *this, GraphicsLayer::Type::Scrolling);
>>> 
>>> I don't think we should use GraphicsLayer::Type::Scrolling for the main frame. Does this code run for the main frame?
>> 
>> I don't think it's called for the main frame, but will check again.
> 
> I think new types are preferable (maybe call it Subframe* rather than NonMain*).

For the main frame, RenderLayerCompositor::requiresScrollLayer returns false on iOS so we don't use GraphicsLayer::Type::Scrolling:
https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp#L2736
Comment 64 Frédéric Wang (:fredw) 2018-02-07 07:22:55 PST
Comment on attachment 331743 [details]
Patch

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

>>>> Source/WebCore/page/FrameView.cpp:4461
>>>> +#endif
>>> 
>>> Is this here to handle async scrolling? Normally point conversion uses the current state of the DOM, and these offsets should already be reflected in RenderLayer scroll offsets.
>> 
>> This is to handle hit testing after user scroll. Without that, the scroll offset is not taken into account for hit testing. See comment 50 and test clickElementInsideFrameAfterUserScroll.
> 
> Is this only an issue on the first touch that stops a scroll? We already get that wrong, but it should be fixed separately. If it's every hit test even when stationary, them something's wrong.

Is there a bug report for the non-stationary case? The bug here happens for the stationary case too, but maybe that's because I'm still doing things wrong with the layer hierarchy.

>>>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:188
>>>> +#endif
>>> 
>>> Seems wrong to set layer size here; layer size should be set in RenderLayerBacking::updateGeometry or similar.
>> 
>> mmh, I thought you mean in comment 2 that we should not use RenderLayerBacking? Now I'm confused...
> 
> The only place we should be setting layer sizes and positions is in RLB::updateGeometry(). What I objected to in that early patch was code in RenderLayer that was about Frame scrolling.

So regarding this, my initial patches used RenderLayerBacking's "scrolling container" layer (created in RenderLayerBacking::updateScrollingLayers, called from RenderLayerBacking::updateConfiguration). In order to create the layer for subframes, I ensured that usesAcceleratedScrolling() on m_owningLayer returns true. If that's a problem, I can directly put the special case for subframes in RenderLayerBacking, but it does not seem that all will be in FrameView/AsyncFrameScrolling...

Then because of your comment, I understood I should instead use RenderLayerCompositor's "frame scrolling" layer (created in RenderLayerCompositor::ensureRootLayer()). That's why I modified the function to create the layer for subframes. I initially performed the geometry update in RenderLayerCompositor, but after suggestions by Ali I moved it to AsyncScrollingCoordinator, which seemed consistent with your will to have things in FrameView/AsyncScrollingCoordinator.

So should I use RenderLayerBacking's "scrolling container" or RenderLayerCompositor's "frame scrolling" in order to get the UIScrollView? And how/where would you suggest to create/update the layer?
Comment 65 Simon Fraser (smfr) 2018-02-07 17:32:19 PST
(In reply to Frédéric Wang (:fredw) from comment #64)
> Comment on attachment 331743 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331743&action=review
> 
> >>>> Source/WebCore/page/FrameView.cpp:4461
> >>>> +#endif
> >>> 
> >>> Is this here to handle async scrolling? Normally point conversion uses the current state of the DOM, and these offsets should already be reflected in RenderLayer scroll offsets.
> >> 
> >> This is to handle hit testing after user scroll. Without that, the scroll offset is not taken into account for hit testing. See comment 50 and test clickElementInsideFrameAfterUserScroll.
> > 
> > Is this only an issue on the first touch that stops a scroll? We already get that wrong, but it should be fixed separately. If it's every hit test even when stationary, them something's wrong.
> 
> Is there a bug report for the non-stationary case?

Not here in bugzilla, no.

> The bug here happens for
> the stationary case too, but maybe that's because I'm still doing things
> wrong with the layer hierarchy.
> 
> >>>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:188
> >>>> +#endif
> >>> 
> >>> Seems wrong to set layer size here; layer size should be set in RenderLayerBacking::updateGeometry or similar.
> >> 
> >> mmh, I thought you mean in comment 2 that we should not use RenderLayerBacking? Now I'm confused...
> > 
> > The only place we should be setting layer sizes and positions is in RLB::updateGeometry(). What I objected to in that early patch was code in RenderLayer that was about Frame scrolling.
> 
> So regarding this, my initial patches used RenderLayerBacking's "scrolling
> container" layer (created in RenderLayerBacking::updateScrollingLayers,
> called from RenderLayerBacking::updateConfiguration). In order to create the
> layer for subframes, I ensured that usesAcceleratedScrolling() on
> m_owningLayer returns true. If that's a problem, I can directly put the
> special case for subframes in RenderLayerBacking, but it does not seem that
> all will be in FrameView/AsyncFrameScrolling...
> 
> Then because of your comment, I understood I should instead use
> RenderLayerCompositor's "frame scrolling" layer (created in
> RenderLayerCompositor::ensureRootLayer()). That's why I modified the
> function to create the layer for subframes. I initially performed the
> geometry update in RenderLayerCompositor, but after suggestions by Ali I
> moved it to AsyncScrollingCoordinator, which seemed consistent with your
> will to have things in FrameView/AsyncScrollingCoordinator.
> 
> So should I use RenderLayerBacking's "scrolling container" or
> RenderLayerCompositor's "frame scrolling" in order to get the UIScrollView?
> And how/where would you suggest to create/update the layer?

I think RenderLayerCompositor should have a std::unique_ptr<GraphicsLayer> m_scrollingLayer which is non-null for scrollable iframes. It's not appropriate for RLB to have this, since the root of the subtree that a RLB knows about is the primary m_graphicsLayer, which is the scrolled contents for a frame (ignoring m_ancestorClippingLayer/m_contentsContainmentLayer).

So RLC needs to make this scrolling layer when it needs one. It's geometry can be updated in RLC::frameViewDidChangeSize().

Actually maybe this is already RLC::m_scrollLayer (created based on RenderLayerCompositor::requiresScrollLayer() behavior) so maybe most of this is already there. RenderLayerCompositor::updateScrollCoordinationForThisFrame() already pushes that layer onto the frame scrolling tree node.

Does that work?
Comment 66 Frédéric Wang (:fredw) 2018-02-08 08:43:28 PST
Created attachment 333378 [details]
Patch (update position/size in RenderLayerCompositor)

(In reply to Simon Fraser (smfr) from comment #65)
> I think RenderLayerCompositor should have a std::unique_ptr<GraphicsLayer>
> m_scrollingLayer which is non-null for scrollable iframes. It's not
> appropriate for RLB to have this, since the root of the subtree that a RLB
> knows about is the primary m_graphicsLayer, which is the scrolled contents
> for a frame (ignoring m_ancestorClippingLayer/m_contentsContainmentLayer).
> 
> So RLC needs to make this scrolling layer when it needs one. It's geometry
> can be updated in RLC::frameViewDidChangeSize().
> 
> Actually maybe this is already RLC::m_scrollLayer (created based on
> RenderLayerCompositor::requiresScrollLayer() behavior) so maybe most of this
> is already there.
> RenderLayerCompositor::updateScrollCoordinationForThisFrame() already pushes
> that layer onto the frame scrolling tree node.
> 
> Does that work?

So here is an alternative patch that performs update of size & position in RenderLayerCompositor. Indeed it does work too, however it's a bit more artificial & hacky: The current code is designed in a way that if there is a scrolling coordinator for the frame then such coordinator manages geometry update (see comment in RenderLayerCompositor::frameViewDidScroll for example). That's the case here since we have a RemoteScrollingCoordinator and RemoteScrollingCoordinator::coordinatesScrollingForFrameView returns true, so the patch has to work around that behavior to make things work. I believe Ali's suggestion in comment 18 to move things to AsyncScrollingCoordinator was actually sensible... Why do you want to perform geometry update in RenderLayerCompositor?
Comment 67 Frédéric Wang (:fredw) 2018-02-09 08:29:03 PST
Created attachment 333488 [details]
Patch
Comment 68 Frédéric Wang (:fredw) 2018-02-09 08:30:55 PST
Created attachment 333489 [details]
Testcase for hit testing
Comment 69 Frédéric Wang (:fredw) 2018-02-09 08:46:10 PST
(In reply to Frédéric Wang (:fredw) from comment #68)
> Created attachment 333489 [details]
> Testcase for hit testing

Attachment 333489 [details] is what I used to debug hit testing. For example, we arrive at

ScrollView::windowToContents
WebCore::documentPointForWindowPoint
WebCore::EventHandler::handleMousePressEvent
WebCore::EventHandler::passMousePressEventToSubframe
WebCore::EventHandler::handleMousePressEvent

where the subframe's scrollPosition is not taken into account when delegatesScrolling() == true (which is the case here). The previous patches used convert.From/To.ContainingView to add the offsetFromRenderer == scrollPosition. Now, I'm just skipping the delegatesScrolling() case so that viewToContents and contentsToView add the necessary offset. However, I'm still not really sure it is the proper way to handle this...
Comment 70 Build Bot 2018-02-09 10:03:58 PST
Comment on attachment 333488 [details]
Patch

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

New failing tests:
fast/visual-viewport/ios/stable-update-with-keyboard.html
fast/visual-viewport/ios/min-scale-greater-than-one.html
Comment 71 Build Bot 2018-02-09 10:04:00 PST
Created attachment 333498 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 72 Frédéric Wang (:fredw) 2018-02-12 02:16:25 PST
Created attachment 333586 [details]
Patch

This only does the hit testing adjustment in WebCore::documentPointForWindowPoint.
Comment 73 Simon Fraser (smfr) 2018-02-12 13:44:13 PST
Comment on attachment 333586 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:1616
> +#if PLATFORM(IOS)
> +    // ScrollView::windowToContents does not take scroll position of subframes, so we do it here.
> +    if (!frame.isMainFrame())
> +        return view->viewToContents(view->windowToContents(windowPoint));
> +#endif

It would be best to avoid this #ifdef if we can. There is a problem currently where delegatesScrolling() currently causes frame scroll positions (maybe only for the main frame) to be ignored, but we should really fix that because it's the cause of a number of bugs.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:181
> +#if PLATFORM(IOS)
> +    if (auto* scrollLayer = scrollLayerForFrameView(frameView))
> +        scrollLayer->setSize(frameView.sizeForVisibleContent());
> +#endif

I still think it's wrong to set layer sizes here. This should be in RenderLayerCompositor (where we don't have any code that sets the size of the scroll layer, but should, probably in RenderLayerCompositor::frameViewDidChangeSize()).

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:439
> +    ASSERT(scrolledContentsLayer);
> +    scrollLayer->setPosition(FloatPoint());
> +    FloatSize oldScrollLayerOffset = scrollLayer->offsetFromRenderer();
> +    scrollLayer->setOffsetFromRenderer(FloatSize());
> +    bool scrollOffsetChanged = oldScrollLayerOffset != scrollLayer->offsetFromRenderer();

This stuff should be in RLC or RLB.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:487
> +    IntSize scrollSize = frameView.contentsSize();
> +    scrolledContentsLayer->setPosition(FloatPoint());
> +    if (scrollSize != scrolledContentsLayer->size() || scrollOffsetChanged)
> +        scrolledContentsLayer->setNeedsDisplay();
> +    scrolledContentsLayer->setSize(scrollSize);
> +    LayoutSize scrolledContentsOffset = -toLayoutSize(frameView.scrollPosition());
> +    scrolledContentsLayer->setOffsetFromRenderer(scrolledContentsOffset, GraphicsLayer::DontSetNeedsDisplay);

Needs to move elsewhere. RLB is the only class that should be calling setOffsetFromRenderer()

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:642
> +    scrolledContentsLayer->setOffsetFromRenderer(scrolledContentsOffset, GraphicsLayer::DontSetNeedsDisplay);

Needs to move elsewhere. RLB is the only class that should be calling setOffsetFromRenderer()

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3296
> +#if PLATFORM(IOS)
> +            m_scrollLayer = GraphicsLayer::create(graphicsLayerFactory(), *this, GraphicsLayer::Type::Scrolling);
> +#else

This kind of thing should not be a platform #ifdef. The #ifdef should reflect the specific reason why the code needs to differ.

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp:117
>  #if PLATFORM(IOS)

This is another undesirable platform #ifdef.
Comment 74 Frédéric Wang (:fredw) 2018-02-15 09:31:55 PST
Created attachment 333906 [details]
Patch for EWS
Comment 75 Build Bot 2018-02-15 10:20:15 PST
Comment on attachment 333906 [details]
Patch for EWS

Attachment 333906 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6519397

Number of test failures exceeded the failure limit.
Comment 76 Build Bot 2018-02-15 10:20:17 PST
Created attachment 333914 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 77 Build Bot 2018-02-15 10:36:55 PST
Comment on attachment 333906 [details]
Patch for EWS

Attachment 333906 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6519540

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/iframes/connect-compositing-iframe3.html
compositing/iframes/overlapped-iframe.html
compositing/visible-rect/iframe-with-layers-outside-viewport.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/visible-rect/iframe-and-layers.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/page-cache-layer-tree.html
compositing/iframes/resizer.html
compositing/repaint/iframes/compositing-iframe-scroll-repaint.html
compositing/iframes/connect-compositing-iframe2.html
compositing/iframes/become-overlapped-iframe.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/iframes/overlapped-iframe-iframe.html
compositing/repaint/iframes/compositing-iframe-with-fixed-background-doc-repaint.html
Comment 78 Build Bot 2018-02-15 10:36:57 PST
Created attachment 333915 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 79 Build Bot 2018-02-15 11:14:50 PST
Comment on attachment 333906 [details]
Patch for EWS

Attachment 333906 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6519778

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/overlapped-iframe.html
compositing/visible-rect/iframe-with-layers-outside-viewport.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/visible-rect/iframe-and-layers.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/page-cache-layer-tree.html
compositing/iframes/resizer.html
compositing/repaint/iframes/compositing-iframe-scroll-repaint.html
compositing/iframes/connect-compositing-iframe2.html
compositing/repaint/iframes/compositing-iframe-with-fixed-background-doc-repaint.html
compositing/iframes/connect-compositing-iframe3.html
compositing/iframes/become-composited-nested-iframes.html
compositing/iframes/overlapped-iframe-iframe.html
compositing/iframes/become-overlapped-iframe.html
Comment 80 Build Bot 2018-02-15 11:14:52 PST
Created attachment 333920 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 81 Build Bot 2018-02-15 12:57:12 PST
Comment on attachment 333906 [details]
Patch for EWS

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

New failing tests:
fast/scrolling/ios/programmatic-scroll-iframe.html
fast/scrolling/ios/mixing-user-and-programmatic-scroll.html
compositing/iframes/scrolling-iframe.html
Comment 82 Build Bot 2018-02-15 12:57:14 PST
Created attachment 333932 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 83 Frédéric Wang (:fredw) 2018-02-15 15:15:57 PST
Created attachment 333955 [details]
Patch for EWS
Comment 84 Build Bot 2018-02-15 16:24:40 PST
Comment on attachment 333955 [details]
Patch for EWS

Attachment 333955 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6524912

New failing tests:
compositing/repaint/iframes/compositing-iframe-scroll-repaint.html
compositing/repaint/iframes/compositing-iframe-with-fixed-background-doc-repaint.html
Comment 85 Build Bot 2018-02-15 16:24:42 PST
Created attachment 333965 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 86 Build Bot 2018-02-15 16:59:59 PST
Comment on attachment 333955 [details]
Patch for EWS

Attachment 333955 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6525035

New failing tests:
compositing/repaint/iframes/compositing-iframe-scroll-repaint.html
compositing/repaint/iframes/compositing-iframe-with-fixed-background-doc-repaint.html
Comment 87 Build Bot 2018-02-15 17:00:00 PST
Created attachment 333971 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 88 Build Bot 2018-02-15 17:19:25 PST
Comment on attachment 333955 [details]
Patch for EWS

Attachment 333955 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6525667

New failing tests:
tiled-drawing/scrolling/frames/fixed-inside-frame.html
compositing/tiling/tiled-drawing-async-frame-scrolling.html
tiled-drawing/tile-coverage-iframe-to-zero-coverage.html
Comment 89 Build Bot 2018-02-15 17:19:27 PST
Created attachment 333972 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 90 Frédéric Wang (:fredw) 2018-02-16 02:16:06 PST
Created attachment 334027 [details]
Patch for EWS (includes 182785)
Comment 91 Frédéric Wang (:fredw) 2018-02-16 03:49:51 PST
Created attachment 334030 [details]
Patch (applies on top of bug 182785)

This patch addresses the review comments. I moved the update of scroll position in the follow-up bug 182868.
Comment 92 Simon Fraser (smfr) 2018-04-09 08:05:42 PDT
(In reply to Frédéric Wang (:fredw) from comment #91)
> Created attachment 334030 [details]
> Patch (applies on top of bug 182785)
> 
> This patch addresses the review comments. I moved the update of scroll
> position in the follow-up bug 182868.

I think adding back the fixed version should be done in order to fix one of the bugs, not as part of adding a new feature. You'll need to test UIWebView/WebView/WKWebView when changing each call site to the fixed version of the function.
Comment 93 Frédéric Wang (:fredw) 2018-04-09 08:15:55 PDT
(In reply to Simon Fraser (smfr) from comment #92)
> (In reply to Frédéric Wang (:fredw) from comment #91)
> I think adding back the fixed version should be done in order to fix one of
> the bugs, not as part of adding a new feature. You'll need to test
> UIWebView/WebView/WKWebView when changing each call site to the fixed
> version of the function.

OK that makes sense but note that because frames are not scrollable some bugs are not "visible" yet so I can not fix them before implementing the new feature. And if I don't fix the bugs here, I have to move new tests to follow-up bugs (this is what I had to do my moving parts of this bug into bug 182868).
Comment 94 Frédéric Wang (:fredw) 2018-05-09 23:32:30 PDT
Created attachment 340070 [details]
Patch (applies on top of bug 182785)

Rebasing
Comment 95 Frédéric Wang (:fredw) 2018-09-05 07:02:15 PDT
Created attachment 348911 [details]
Patch (applies on top of 182785)

Rebasing...
Comment 96 Frédéric Wang (:fredw) 2018-09-10 02:55:29 PDT
Created attachment 349305 [details]
Patch

New version, taking https://bugs.webkit.org/show_bug.cgi?id=184302#c8 into account.
Comment 97 Frédéric Wang (:fredw) 2018-09-10 08:24:48 PDT
Comment on attachment 349305 [details]
Patch

@smfr: review ping (this no longer introduces a deprecated version)
Comment 98 Frédéric Wang (:fredw) 2018-11-15 07:02:42 PST
Created attachment 354928 [details]
Patch

Rebasing patch...
Comment 99 Build Bot 2018-11-15 08:10:48 PST
Comment on attachment 354928 [details]
Patch

Attachment 354928 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10003986

New failing tests:
compositing/repaint/iframes/compositing-iframe-scroll-repaint.html
compositing/repaint/iframes/compositing-iframe-with-fixed-background-doc-repaint.html
Comment 100 Build Bot 2018-11-15 08:10:50 PST
Created attachment 354936 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 101 Build Bot 2018-11-15 09:06:05 PST
Comment on attachment 354928 [details]
Patch

Attachment 354928 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10004215

New failing tests:
compositing/repaint/iframes/compositing-iframe-scroll-repaint.html
compositing/repaint/iframes/compositing-iframe-with-fixed-background-doc-repaint.html
Comment 102 Build Bot 2018-11-15 09:06:09 PST
Created attachment 354941 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 103 Frédéric Wang (:fredw) 2018-11-20 03:59:41 PST
Created attachment 355334 [details]
Patch
Comment 104 Build Bot 2018-11-20 05:06:44 PST
Comment on attachment 355334 [details]
Patch

Attachment 355334 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10087517

New failing tests:
compositing/iframes/resize-from-zero-size.html
compositing/repaint/iframes/compositing-iframe-scroll-repaint.html
compositing/iframes/remove-reinsert-webview-with-iframe.html
compositing/repaint/iframes/compositing-iframe-with-fixed-background-doc-repaint.html
Comment 105 Build Bot 2018-11-20 05:06:47 PST
Created attachment 355335 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 106 Build Bot 2018-11-20 05:59:17 PST
Comment on attachment 355334 [details]
Patch

Attachment 355334 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10087593

New failing tests:
compositing/iframes/resize-from-zero-size.html
compositing/repaint/iframes/compositing-iframe-scroll-repaint.html
compositing/iframes/remove-reinsert-webview-with-iframe.html
compositing/repaint/iframes/compositing-iframe-with-fixed-background-doc-repaint.html
Comment 107 Build Bot 2018-11-20 05:59:20 PST
Created attachment 355337 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 108 Build Bot 2018-11-20 06:40:46 PST
Comment on attachment 355334 [details]
Patch

Attachment 355334 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10087973

New failing tests:
compositing/iframes/resize-from-zero-size.html
compositing/iframes/remove-reinsert-webview-with-iframe.html
Comment 109 Build Bot 2018-11-20 06:40:49 PST
Created attachment 355340 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 110 Frédéric Wang (:fredw) 2018-11-20 07:41:03 PST
Created attachment 355345 [details]
Patch
Comment 111 Frédéric Wang (:fredw) 2018-11-22 09:08:15 PST
Comment on attachment 355345 [details]
Patch

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

@smfr: review ping?

Unfortunately, it does not seem possible to take the change of coordinateChangeShouldIgnoreScrollPosition() out of this patch without seriously breaking existing tests... As explained in bug 184297 comment 13, this should only affect frames in WK2 though.

> Source/WebCore/platform/ScrollView.cpp:935
>  

I tried to split these coordinateChangeShouldIgnoreScrollPosition() changes out f this patch. ios/scroll-iframe.html still passes and the one test in hit-testing-iframe.html to check "a click event is consumed by an element inside the frame, after a user scroll" fails (as expected). That could be fine, however, I also get big size changes for some fixed-inside-frame.html/tile-coverage-iframe-to-zero-coverage.hml on iOS and some reorganization of scrolling hierarchy for some compositing/rtl/rtl-iframe* tests on macOS. So this is more serious than the small subframe resize of the current patch...

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1421
> +        state.move(-boundsOrigin.x(), -boundsOrigin.y());

This is tested by one test in fast/scrolling/ios/scroll-iframe.html
Comment 112 Frédéric Wang (:fredw) 2018-11-30 00:26:44 PST
Created attachment 356154 [details]
Patch

Rebasing
Comment 113 Frédéric Wang (:fredw) 2018-11-30 00:29:36 PST
Created attachment 356155 [details]
Patch (attempt to remove the changes to windowToContents/contentsToWindow)
Comment 114 Frédéric Wang (:fredw) 2018-11-30 11:52:01 PST
Created attachment 356214 [details]
Patch
Comment 115 Frédéric Wang (:fredw) 2018-11-30 11:53:48 PST
Created attachment 356216 [details]
Patch (attempt to remove the changes to windowToContents/contentsToWindow)
Comment 116 Frédéric Wang (:fredw) 2018-12-03 00:43:47 PST
Comment on attachment 356216 [details]
Patch (attempt to remove the changes to windowToContents/contentsToWindow)

@smfr: OK, it looks like it's actually possible to move the changes to windowToContents/contentsToWindow out of this patch. I've opened bug 192303 as a follow-up patch.

(Source/WebCore/ChangeLog should be updated)