Bug 193665

Summary: [iOS] Tiles not created in large scrollable iframes
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: ScrollingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cmarcelo, commit-queue, ews-watchlist, fred.wang, jamesr, luiz, simon.fraser, tonikitoo, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149264    
Attachments:
Description Flags
patch
none
patch
none
patch
none
wip
ews-watchlist: commit-queue-
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
patch
none
patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
patch
simon.fraser: review+
patch none

Description Antti Koivisto 2019-01-22 00:59:34 PST
Visible rect updates don't propagate correctly.
Comment 1 Antti Koivisto 2019-01-22 01:13:41 PST
Created attachment 359724 [details]
patch
Comment 2 Frédéric Wang (:fredw) 2019-01-22 03:41:01 PST
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.
Comment 3 Antti Koivisto 2019-01-22 04:13:09 PST
> Probably we need to update
> AsyncScrollingCoordinator::updateScrollLayerPosition too.

Yeah, though it would be better to land that change with a test that verifies it.
Comment 4 Antti Koivisto 2019-01-22 04:23:18 PST
Created attachment 359729 [details]
patch
Comment 5 Antti Koivisto 2019-01-22 04:28:45 PST
Created attachment 359730 [details]
patch
Comment 6 Simon Fraser (smfr) 2019-01-22 08:34:47 PST
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.
Comment 7 Antti Koivisto 2019-01-22 11:29:21 PST
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.
Comment 8 Simon Fraser (smfr) 2019-01-22 12:55:24 PST
Comment on attachment 359730 [details]
patch

Do we want to land this, or fix the scrollLayer/scrolledContentsLayer confusion first?
Comment 9 Antti Koivisto 2019-01-22 14:09:26 PST
Let me take a look at fixing the confusion, at least for the frames.
Comment 10 Antti Koivisto 2019-01-30 10:47:57 PST
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.
Comment 11 Simon Fraser (smfr) 2019-01-30 11:26:11 PST
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.
Comment 12 Antti Koivisto 2019-01-30 11:44:26 PST
> 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.
Comment 13 Simon Fraser (smfr) 2019-01-30 11:45:10 PST
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.
Comment 14 EWS Watchlist 2019-01-30 12:53:22 PST
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
Comment 15 EWS Watchlist 2019-01-30 12:53:24 PST
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
Comment 16 Antti Koivisto 2019-02-03 02:05:31 PST
Created attachment 361002 [details]
patch
Comment 17 Antti Koivisto 2019-02-03 02:53:07 PST
Created attachment 361006 [details]
patch
Comment 18 EWS Watchlist 2019-02-03 04:58:44 PST
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
Comment 19 EWS Watchlist 2019-02-03 04:58:46 PST
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
Comment 20 Antti Koivisto 2019-02-03 10:02:17 PST
Created attachment 361020 [details]
patch
Comment 21 Antti Koivisto 2019-02-03 10:04:24 PST
editing/pasteboard failures are unrelated to the patch
Comment 22 Simon Fraser (smfr) 2019-02-03 21:32:06 PST
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.
Comment 23 Antti Koivisto 2019-02-03 22:57:48 PST
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?
Comment 24 Antti Koivisto 2019-02-03 23:11:22 PST
Created attachment 361040 [details]
patch
Comment 25 WebKit Commit Bot 2019-02-03 23:49:38 PST
Comment on attachment 361040 [details]
patch

Clearing flags on attachment: 361040

Committed r240916: <https://trac.webkit.org/changeset/240916>
Comment 26 WebKit Commit Bot 2019-02-03 23:49:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2019-02-03 23:51:01 PST
<rdar://problem/47779728>