Bug 193665 – [iOS] Tiles not created in large scrollable iframes

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Antti Koivisto]

Visible rect updates don't propagate correctly.

[Antti Koivisto]

Created attachment 359724 [details]
patch

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

Comment on attachment 359724 [details]
patch

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

Some of these changes regarding scroll offset adjustments seem similar to what I have extracted in attachment 359723 [details] from my old patches.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:455
>          if (insetClipLayer)

Probably we need to update AsyncScrollingCoordinator::updateScrollLayerPosition too.

[Antti Koivisto]

> Probably we need to update
> AsyncScrollingCoordinator::updateScrollLayerPosition too.

Yeah, though it would be better to land that change with a test that verifies it.

[Antti Koivisto]

Created attachment 359729 [details]
patch

[Antti Koivisto]

Created attachment 359730 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 359724 [details]
patch

I need to think about this a little bit. Would be nice to have macOS and iOS be similar enough that we don't have to have platform #ifdefs here.

[Antti Koivisto]

We should clarify which layer moves during scrolling. The regular code paths assume that the scrollLayer (in scrolling tree and elsewhere) is the one that moves. UIScrollView code paths assume scrollLayer is the UIScrollView (so just clips) and the scrolledContentsLayer is the one that moves.

The code would be much clearer and require fewer exceptions if we (for example) made the clip layer to be the UIScrollView and let scrollLayer be its moving child layer.

This would need to be matched in both overflow and frame code.

[Simon Fraser (smfr)]

Comment on attachment 359730 [details]
patch

Do we want to land this, or fix the scrollLayer/scrolledContentsLayer confusion first?

[Antti Koivisto]

Let me take a look at fixing the confusion, at least for the frames.

[Antti Koivisto]

Created attachment 360596 [details]
wip

This adds a concept of "scroll container layer" to the scrolling tree and uses it for frames. This layer then maps to UIScrollView when async scrolling is enabled. In this setup "scroll layer" is always the layer that moves during scrolling. This matches the existing pure layer based scrolling code and so requires way fewer hacks.

Scroll container layer replaces the existing clip layer as it serves that function as well.

[Simon Fraser (smfr)]

Comment on attachment 360596 [details]
wip

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

> Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h:-109
> -    // This is a clipping layer that will scroll with the page for all y-delta scroll values between 0
> -    // and topContentInset(). Once the y-deltas get beyond the content inset point, this layer no longer
> -    // needs to move. If the topContentInset() is 0, this layer does not need to move at all. This is
> -    // only used on the Mac.

Do we understand the history here, and why your removal doesn't break something?

> Source/WebCore/rendering/RenderLayerCompositor.h:564
> +    // Layer that moves and clips its content layer.
> +    RefPtr<GraphicsLayer> m_scrollContainerLayer;

The "that moves" is confusing. How does this layer move its content? The moving is done by setting bounds origin or position on m_scrollLayer, right? This layer doesn't control that.

> Source/WebCore/rendering/RenderLayerCompositor.h:566
> +    // Layer that moves during scrolling.
>      RefPtr<GraphicsLayer> m_scrollLayer;

Can we call this scrolledContentsLayer? That would be more consistent with RenderLayerBacking names.

[Antti Koivisto]

> Do we understand the history here, and why your removal doesn't break
> something?

Nothing is really removed here scrollContainerLayer just also serves as insetClipLayer. All the logic around that is unchanged.

> The "that moves" is confusing. How does this layer move its content? The
> moving is done by setting bounds origin or position on m_scrollLayer, right?
> This layer doesn't control that.

Yeah, bad comment.

> > Source/WebCore/rendering/RenderLayerCompositor.h:566
> > +    // Layer that moves during scrolling.
> >      RefPtr<GraphicsLayer> m_scrollLayer;
> 
> Can we call this scrolledContentsLayer? That would be more consistent with
> RenderLayerBacking names.

There is existing scrolledContentsLayer in scrolling tree though, which does not map to this layer.

[Simon Fraser (smfr)]

Comment on attachment 360596 [details]
wip

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

>> Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h:-109
>> -    // only used on the Mac.
> 
> Do we understand the history here, and why your removal doesn't break something?

This was added in https://trac.webkit.org/r168244

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:59
> +        ScrollContainerLayer,

We should rename the layer on the base class to be just "layer", not "ScrollLayer", and we should actually set it to the main graphicsLayer() for scrolling nodes (but the scrolling tree may never use it).

>> Source/WebCore/rendering/RenderLayerCompositor.h:564
>> +    RefPtr<GraphicsLayer> m_scrollContainerLayer;
> 
> The "that moves" is confusing. How does this layer move its content? The moving is done by setting bounds origin or position on m_scrollLayer, right? This layer doesn't control that.

For iOS, the moving is done by setting boundsOrigin on this layer. For macOS, we set the position of m_scrollLayer. I think the best option is to hide that difference, and just make sure that code that moves layers around has access to both these layers.

[EWS Watchlist]

Comment on attachment 360596 [details]
wip

Attachment 360596 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10955383

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html

[EWS Watchlist]

Created attachment 360604 [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.13.6

[Antti Koivisto]

Created attachment 361002 [details]
patch

[Antti Koivisto]

Created attachment 361006 [details]
patch

[EWS Watchlist]

Comment on attachment 361006 [details]
patch

Attachment 361006 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11014492

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
scrollingcoordinator/scrolling-tree/fixed-inside-frame.html
editing/pasteboard/smart-paste-007.html
editing/pasteboard/smart-paste-008.html

[EWS Watchlist]

Created attachment 361010 [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.13.6

[Antti Koivisto]

Created attachment 361020 [details]
patch

[Antti Koivisto]

editing/pasteboard failures are unrelated to the patch

[Simon Fraser (smfr)]

Comment on attachment 361020 [details]
patch

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

r=me but at least rename the setScrollPositionToScrollingLayer/syncScrollPositionToScrollingLayer functions, and maybe move them.

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=193665

And rdar://problem/47743603.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:473
> +void AsyncScrollingCoordinator::setScrollPositionToScrollingLayer(FrameView& frameView)

setScrollPositionForScrollingLayer? Setting the position "to" a layer doesn't make sense. Also we're trying to avoid the term "scrolling layer".

For overflow, I implemented this equivalent in RenderLayerBacking::setLocationOfScrolledContents(ScrollOffset scrollOffset, ScrollingLayerPositionAction setOrSync). Maybe this code should live in RenderLayerCompositor?

Another option is to give the ScrollingStateTreeNodes this responsibility.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:485
> +void AsyncScrollingCoordinator::syncScrollPositionToScrollingLayer(FrameView& frameView)

Also needs better name/better home.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:219
> +    if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::ScrollContainerLayer))
> +        m_scrollLayer = scrollingStateNode.scrollContainerLayer();

Much less confusing.

[Antti Koivisto]

Comment on attachment 361020 [details]
patch

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

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:473
>> +void AsyncScrollingCoordinator::setScrollPositionToScrollingLayer(FrameView& frameView)
> 
> setScrollPositionForScrollingLayer? Setting the position "to" a layer doesn't make sense. Also we're trying to avoid the term "scrolling layer".
> 
> For overflow, I implemented this equivalent in RenderLayerBacking::setLocationOfScrolledContents(ScrollOffset scrollOffset, ScrollingLayerPositionAction setOrSync). Maybe this code should live in RenderLayerCompositor?
> 
> Another option is to give the ScrollingStateTreeNodes this responsibility.

I combined the set/sync functions and just called it reconcileScrollPosition. Maybe the shared function should be a static function hanging from say GraphicsLayer and taking both the container and contents layers as parameters?

[Antti Koivisto]

Created attachment 361040 [details]
patch

[WebKit Commit Bot]

Comment on attachment 361040 [details]
patch

Clearing flags on attachment: 361040

Committed r240916: <https://trac.webkit.org/changeset/240916>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47779728>