Bug 182868 - [iOS] Handle update of scroll layer positions for subframes
Summary: [iOS] Handle update of scroll layer positions for subframes
Status: RESOLVED MOVED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
: 193653 (view as bug list)
Depends on: 173833 183040 192303
Blocks: 149264 180002
  Show dependency treegraph
 
Reported: 2018-02-16 03:45 PST by Frédéric Wang (:fredw)
Modified: 2019-02-21 08:09 PST (History)
5 users (show)

See Also:


Attachments
Patch (23.94 KB, patch)
2018-02-16 05:17 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS (includes 182785 and 173833) (156.34 KB, patch)
2018-02-16 05:18 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Testcase (frame with padding/border/margin) (559 bytes, text/html)
2018-02-20 07:38 PST, Frédéric Wang (:fredw)
no flags Details
Dumped trees for attachment 334271 (3.33 KB, application/zip)
2018-02-20 07:41 PST, Frédéric Wang (:fredw)
no flags Details
Patch (23.88 KB, patch)
2018-02-21 01:53 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (24.87 KB, patch)
2018-02-21 03:29 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (24.73 KB, patch)
2018-03-30 06:42 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of bugs 182785, 173833 and 183040) (24.70 KB, patch)
2018-05-09 23:35 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of bugs 182785, 173833 and 183040) (24.72 KB, patch)
2018-09-05 07:04 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of bugs 182785, 173833 and 183040) (24.75 KB, patch)
2018-11-15 07:14 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS (32.46 KB, patch)
2019-01-22 01:01 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (3.20 MB, application/zip)
2019-01-22 02:59 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-02-16 03:45:12 PST
Extracted from bug 173833.
Comment 1 Frédéric Wang (:fredw) 2018-02-16 05:17:54 PST
Created attachment 334032 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2018-02-16 05:18:33 PST
Created attachment 334033 [details]
Patch for EWS (includes 182785 and  173833)
Comment 3 Frédéric Wang (:fredw) 2018-02-20 07:38:43 PST
Created attachment 334271 [details]
Testcase (frame with padding/border/margin)
Comment 4 Frédéric Wang (:fredw) 2018-02-20 07:41:53 PST
Created attachment 334272 [details]
Dumped trees for attachment 334271 [details]

23:23:00 - smfr: maybe you could draw some ASCII art in a bug to show how the web process layer tree maps into UIScrollVIews for a subframe, across the frame boundary
23:23:11 - smfr: it's critical to get that part right

@Simon: This is how the tree mapping is done for attachment 334271 [details]. I'm attaching an archive with the various dumped trees.

Graphics Layer Tree (Web Process)         UIView (UI Process)

content root (800x600)               WKCompositingView (388x454)
|                                    |
\_RenderView (800x600)               \_WKCompositingView (388x454)
  |                                    |
  \_Page TiledBacking containment      \_WKCompositingView (0x0)
    |                                    |
    \_<iframe> (310x210)                 \_WKCompositingView (310x210)
      |                                    |
      \_overflow controls host             \_WKCompositingView (0x0)
        |                                    |
        \_frame clipping (200x100)           \_WKCompositingView (200x100)
          |                                    |
          \_frame scrolling (200x100)          \_UIScrollView (200x100, content-size: 320x540)
            |                                    |
            \_content root (320x540)             \_WKCompositingView (320x540)
              |                                    |   
               \_RenderView (320x540)              \_WKCompositingView (320x540)
Comment 5 Frédéric Wang (:fredw) 2018-02-20 10:07:30 PST
> 22:56:40 - smfr: RLC owns the m_scrollingLayer so it should be the one to resize it.
> 22:56:50 - smfr: i'm not sure why things work now when it's not getting resized
> 22:57:14 - smfr: maybe just because that layer doesn't really do anything other than add the scroll position
> 22:57:21 - smfr: it could probably be 0 x 0
> 22:57:42 - fredw: I think when it is 0x0 we don't see anything
> 22:57:48 - fredw: that's why I need the size
> 22:57:55 - smfr: that should only be true if it has masksToBounds set
> 23:19:31 - fredw: IIRC it does not have masksToBounds but keeping it to 0x0 does not render anything

So regarding this, as I said masksToBounds is not set on the Web Process side. On the UI process side, the UIScrollView "frame scrolling" is created in RemoteLayerTreeHost::createLayer (Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeHostIOS.mm) with layerProperties's masksToBounds set to false but layerProperties is not used at all in that function. The UIScrollView is created by default with masksToBounds == YES. Forcing [[view layer] setMasksToBounds:NO] does allow to display the content of the frame and it is properly clipped thanks to the parent "frame clipping" view. Hit testing and programmatic scrolling works bu user scrolling fails (probably because the UIScrollView has 0x0 size).

> 23:08:43 - smfr: so it looks like m_scrollLayer is always zero sized on mac

Yes, that's the case now. The patch for bug 173833 changes that for subframes since you asked me try and get rid of #if PLATFORM(IOS).
Comment 6 Frédéric Wang (:fredw) 2018-02-20 10:14:20 PST
> 22:58:28 - fredw: https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp#L1672
> 22:59:14 - fredw: because hasCoordinatedScrolling() returns true, updateScrollLayerPosition() is not called
> 23:00:20 - fredw: on the other hand, we already have some logic to update the scrollLayer in https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp#L405
> 23:00:20 - fredw: on the other hand, we already have some logic to update the scrollLayer in https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp#L405
> 23:00:48 - smfr: right
> 23:01:05 - smfr: the code in AsyncScrollingCoordinator is just to sync up the layers with the scrolling tree after a scroll
> 23:01:29 - smfr: it's not the "primary" layer geometry-setting mechanism
> 23:02:04 - smfr: i'm a bit confused by RenderLayerCompositor::updateScrollLayerPosition()
> 23:02:15 - smfr: not sure which platforms actually use that

Given the above, attachment 334032 [details] performs the setting of the scroll position in RenderLayerCompositor::frameViewDidScroll() (which is also called from FrameView::scrollPositionChanged) not updateScrollLayerPosition(). Also I have to disable the existing code in AsyncScrollingCoordinator.cpp to avoid conflicts.
Comment 7 Frédéric Wang (:fredw) 2018-02-21 01:53:12 PST
Created attachment 334349 [details]
Patch
Comment 8 Frédéric Wang (:fredw) 2018-02-21 03:29:15 PST
Created attachment 334359 [details]
Patch
Comment 9 Frédéric Wang (:fredw) 2018-03-30 06:42:50 PDT
Created attachment 336850 [details]
Patch

Rebasing over the dependent bugs.
Comment 10 Frédéric Wang (:fredw) 2018-05-09 23:35:31 PDT
Created attachment 340072 [details]
Patch (applies on top of bugs  182785, 173833 and 183040)

Rebasing
Comment 11 Frédéric Wang (:fredw) 2018-09-05 07:04:20 PDT
Created attachment 348913 [details]
Patch (applies on top of bugs 182785, 173833 and 183040)
Comment 12 Frédéric Wang (:fredw) 2018-11-15 07:14:16 PST
Created attachment 354930 [details]
Patch (applies on top of bugs 182785, 173833 and 183040)

Rebasing...
Comment 13 Frédéric Wang (:fredw) 2019-01-22 00:57:06 PST
*** Bug 193653 has been marked as a duplicate of this bug. ***
Comment 14 Frédéric Wang (:fredw) 2019-01-22 01:01:32 PST
Created attachment 359723 [details]
Patch for EWS

Rebasing the patch and integrating previous changes from dependent bugs that are not merged yet.

Note: I haven't tested the patch yet, this for EWS.
Comment 15 Build Bot 2019-01-22 02:59:19 PST
Comment on attachment 359723 [details]
Patch for EWS

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

New failing tests:
fast/scrolling/ios/hit-testing-iframe.html
fast/scrolling/ios/mixing-user-and-programmatic-scroll.html
compositing/rtl/rtl-iframe-absolute-overflow.html
fast/scrolling/ios/programmatic-scroll-iframe.html
Comment 16 Build Bot 2019-01-22 02:59:21 PST
Created attachment 359727 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 17 Frédéric Wang (:fredw) 2019-02-05 08:01:26 PST
@Anttik: Out of curiosity, what is the pass rate of the tests in this patch after bug 193665? (I plan to check it again later anyway).
Comment 18 Frédéric Wang (:fredw) 2019-02-21 08:09:37 PST
Remaining tests have been moved to bug 194886, bug 194433 and 194900. And the actual code change is handled elsewhere differently. So I think we can close this bug now.