Bug 173833 – [iOS] Adjust layer hierarchy to handle frame scrolling

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Frames

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 313912 [details]
Create UIScrollView for frames

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 316540 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

Rebasing...

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 317250 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

[Frédéric Wang (:fredw)]

Created attachment 319857 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 323891 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

[Frédéric Wang (:fredw)]

Created attachment 325355 [details]
Patch (relies on RenderLayerCompositor's "frame scrolling" layer)

Rebasing...

[Frédéric Wang (:fredw)]

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.

[Build Bot]

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.

[Build Bot]

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

[Ali Juma]

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 327851 [details]
Patch (WIP)

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 328092 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 328573 [details]
Testcase subframe VS overflow nodes

[Frédéric Wang (:fredw)]

Created attachment 328574 [details]
Graphic Layer output for subframes

[Frédéric Wang (:fredw)]

Created attachment 328575 [details]
Graphic Layer output for overflow node

[Frédéric Wang (:fredw)]

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

[Ali Juma]

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.

[Frédéric Wang (:fredw)]

Comment on attachment 328092 [details]
Patch

Thanks Ali. I'll rebase and try to integrate your suggestions.

[Frédéric Wang (:fredw)]

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.

[Ali Juma]

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

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 329761 [details]
Patch

New version including suggestion from comment 34.

[Frédéric Wang (:fredw)]

Created attachment 330895 [details]
Patch

[EWS Watchlist]

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.

[Frédéric Wang (:fredw)]

Created attachment 330896 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 330905 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 331053 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 331249 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 331278 [details]
Patch

[Frédéric Wang (:fredw)]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 331322 [details]
Patch

Same patch but adjusting copyright year & delay for scroll-iframe.html

[Ali Juma]

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?

[Ali Juma]

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 331743 [details]
Patch

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

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

[Simon Fraser (smfr)]

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

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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?

[Simon Fraser (smfr)]

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

[Frédéric Wang (:fredw)]

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?

[Frédéric Wang (:fredw)]

Created attachment 333488 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 333489 [details]
Testcase for hit testing

[Frédéric Wang (:fredw)]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 333586 [details]
Patch

This only does the hit testing adjustment in WebCore::documentPointForWindowPoint.

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

Created attachment 333906 [details]
Patch for EWS

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 333955 [details]
Patch for EWS

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 334027 [details]
Patch for EWS (includes 182785)

[Frédéric Wang (:fredw)]

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.

[Simon Fraser (smfr)]

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

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 340070 [details]
Patch (applies on top of bug 182785)

Rebasing

[Frédéric Wang (:fredw)]

Created attachment 348911 [details]
Patch (applies on top of 182785)

Rebasing...

[Frédéric Wang (:fredw)]

Created attachment 349305 [details]
Patch

New version, taking https://bugs.webkit.org/show_bug.cgi?id=184302#c8 into account.

[Frédéric Wang (:fredw)]

Comment on attachment 349305 [details]
Patch

@smfr: review ping (this no longer introduces a deprecated version)

[Frédéric Wang (:fredw)]

Created attachment 354928 [details]
Patch

Rebasing patch...

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 355334 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 355345 [details]
Patch

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 356154 [details]
Patch

Rebasing

[Frédéric Wang (:fredw)]

Created attachment 356155 [details]
Patch (attempt to remove the changes to windowToContents/contentsToWindow)

[Frédéric Wang (:fredw)]

Created attachment 356214 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 356216 [details]
Patch (attempt to remove the changes to windowToContents/contentsToWindow)

[Frédéric Wang (:fredw)]

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)

[Frédéric Wang (:fredw)]

Created attachment 358318 [details]
Patch

Rebasing...

[Frédéric Wang (:fredw)]

Comment on attachment 358318 [details]
Patch

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

@smfr: Review ping?

> Source/WebCore/ChangeLog:14
> +        In order to properly implement hit testing, we modify windowToContents(const IntPoint&) (and for

ChangeLog should be updated as this is now handled in the separate bug 192303 (whose landing is blocked by the present bug 173833).

> Source/WebKit/ChangeLog:14
> +        In order to properly implement hit testing, we modify windowToContents(const IntPoint&) (and for

Ditto.

[Frédéric Wang (:fredw)]

Comment on attachment 358318 [details]
Patch

Most of this patch has been landed in bug 193539 and bug 193650. I'll extract the remaining tests and bug fixed and move them to bug 182868.

[Frédéric Wang (:fredw)]

*** This bug has been marked as a duplicate of bug 193539 ***