Bug 172914 – Add ParentRelativeScrollableRect to ScrollingCoordinator::ScrollingGeometry

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

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

This is necessary to perform hit testing in the scrolling tree for bug 171667.

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

Created attachment 312000 [details]
Patch

[Build Bot]

Comment on attachment 312000 [details]
Patch

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

New failing tests:
tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor.html
tiled-drawing/scrolling/frames/fixed-inside-frame.html
tiled-drawing/scrolling/frames/coordinated-frame.html
tiled-drawing/scrolling/frames/coordinated-frame-lose-scrolling-ancestor.html
tiled-drawing/scrolling/frames/coordinated-frame-in-fixed.html

[Build Bot]

Created attachment 312001 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312000 [details]
Patch

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

New failing tests:
fast/scrolling/ios/remove-scrolling-role.html

[Build Bot]

Created attachment 312002 [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.5

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

Created attachment 312004 [details]
Patch

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

@Simon: Can you please review that patch?

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

Created attachment 313395 [details]
Patch (test for iframes boxes)

> 2017/06/14
> 20:18:52 - smfr : so let’s go forward with the parent offset thing in the scrolling tree
> 20:19:05 - smfr : we’ll also need to be careful that we’re handling events in teh correct rectangle for each scroller
> 20:19:16 - smfr : not sure if the scroller bounds that we have now are the right rect
> 20:19:27 - smfr : e.g. test with iframes which have margin,border and padding

OK, I finally had time to check that. Currently, iframe padding box (including scroll bars) is the area that can receive mouse wheel event. However, in attachment 312321 [details] I'm using scrollableAreaSize which is only the iframe padding box (excluding scroll bars). Moreover, the offset calculated here does not take into account the margin+border of the iframe. Other values like (visible) contents size do not seem to be what we want. Hence it seems we'll have to add more parameters or to adjust the calculation. I'm attaching a simple HTML test with its dumped scrolling tree.

[Radar WebKit Bug Importer]

<rdar://problem/34215550>

[Simon Fraser (smfr)]

Comment on attachment 312004 [details]
Patch

I think we should store a rect for each node (to handle borders/padding correctly), and I guess each rect could be relative to its parent node?

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

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 312004 [details]
> Patch
> 
> I think we should store a rect for each node (to handle borders/padding
> correctly), and I guess each rect could be relative to its parent node?

So you are suggesting to replace this "offset in parent" property with a rect, right?

[Simon Fraser (smfr)]

Right, yes.

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

Created attachment 320018 [details]
Patch (WIP)

[Build Bot]

Comment on attachment 320018 [details]
Patch (WIP)

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

New failing tests:
fast/scrolling/scrolling-tree-iframe-boxes.html

[Build Bot]

Created attachment 320029 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320018 [details]
Patch (WIP)

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

New failing tests:
fast/scrolling/scrolling-tree-iframe-boxes.html

[Build Bot]

Created attachment 320031 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320018 [details]
Patch (WIP)

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

New failing tests:
fast/scrolling/scrolling-tree-iframe-boxes.html

[Build Bot]

Created attachment 320033 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320018 [details]
Patch (WIP)

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

New failing tests:
fast/scrolling/scrolling-tree-includes-frame.html
fast/scrolling/scrolling-tree-iframe-boxes.html
fast/scrolling/ios/remove-scrolling-role.html

[Build Bot]

Created attachment 320036 [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.5

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

Created attachment 320273 [details]
Patch (WIP)

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

Created attachment 320541 [details]
Patch

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

Created attachment 320626 [details]
Patch

[Build Bot]

Comment on attachment 320626 [details]
Patch

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

New failing tests:
tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor.html

[Build Bot]

Created attachment 320630 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Simon Fraser (smfr)]

Comment on attachment 320626 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:45
> +        RectInParent,

Maybe we call this ParentRelativeScrollableRect, since it's only the part that should respond to scroll events.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3776
> +        result.rectInParent.moveBy(currLayer->location());

I think this is too naive. It should use something like RenderLayer::convertToLayerCoords(), which may need to change to say if there were any intermediate transforms (we may have existing code like this somewhere).

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

(In reply to Simon Fraser (smfr) from comment #27)
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:3776
> > +        result.rectInParent.moveBy(currLayer->location());
> 
> I think this is too naive. It should use something like
> RenderLayer::convertToLayerCoords(), which may need to change to say if
> there were any intermediate transforms (we may have existing code like this
> somewhere).

I thought we wanted to ignore transforms in a first step, but IIUC at the end we would need to calculate the matrix (product of transforms) and rect (maybe just its size)?

[Simon Fraser (smfr)]

(In reply to Frédéric Wang (:fredw) from comment #28)
> (In reply to Simon Fraser (smfr) from comment #27)
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:3776
> > > +        result.rectInParent.moveBy(currLayer->location());
> > 
> > I think this is too naive. It should use something like
> > RenderLayer::convertToLayerCoords(), which may need to change to say if
> > there were any intermediate transforms (we may have existing code like this
> > somewhere).
> 
> I thought we wanted to ignore transforms in a first step, but IIUC at the
> end we would need to calculate the matrix (product of transforms) and rect
> (maybe just its size)?

I think we should handle translations and scales (i.e. axis-aligned transforms). If any frame is transformed in other ways, we need to throw the entire page into slow scrolling (or do something smarter with bounding rects).

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

(In reply to Simon Fraser (smfr) from comment #29)
> I think we should handle translations and scales (i.e. axis-aligned
> transforms).

Ah, right I remember now. OK, we should handle those here.

> If any frame is transformed in other ways, we need to throw the
> entire page into slow scrolling (or do something smarter with bounding
> rects).

I had opened bug 173354 for that.

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

Created attachment 320918 [details]
Patch

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

Created attachment 321814 [details]
Patch

Small modification to use RenderLayer::convertToLayerCoords

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

Created attachment 321815 [details]
Patch

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

Created attachment 325353 [details]
Patch

Rebasing...

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

Created attachment 355484 [details]
Patch

Rebasing. Some tests are flacky, with "reachable contents size" being randomly output or not. Need to debug this...

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

Created attachment 355485 [details]
Patch

[EWS Watchlist]

Comment on attachment 355485 [details]
Patch

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

New failing tests:
tiled-drawing/scrolling/frames/fixed-inside-frame.html

[EWS Watchlist]

Created attachment 355512 [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 355485 [details]
Patch

Attachment 355485 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10119003

New failing tests:
webanimations/leak-document-with-web-animation.html
fast/scrolling/scrolling-tree-iframe-parent-relative-scrollable-rect.html

[EWS Watchlist]

Created attachment 355513 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

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

Created attachment 355514 [details]
Patch

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

Created attachment 355517 [details]
Patch

[EWS Watchlist]

Comment on attachment 355517 [details]
Patch

Attachment 355517 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10120465

New failing tests:
webanimations/leak-document-with-web-animation.html
fast/scrolling/scrolling-tree-iframe-parent-relative-scrollable-rect.html

[EWS Watchlist]

Created attachment 355526 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 355517 [details]
Patch

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

New failing tests:
fast/scrolling/ios/change-scrollability-on-content-resize-nested.html
fast/scrolling/ios/remove-scrolling-role.html
media/no-fullscreen-when-hidden.html
fast/scrolling/ios/change-scrollability-on-content-resize.html

[EWS Watchlist]

Created attachment 355527 [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

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

Created attachment 355633 [details]
Patch

[EWS Watchlist]

Comment on attachment 355633 [details]
Patch

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

New failing tests:
media/no-fullscreen-when-hidden.html

[EWS Watchlist]

Created attachment 355669 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Simon Fraser (smfr)]

I've been working on some code to update the scrolling tree during a new GraphicsLayer tree walk post-flush in RenderLayerCompositor (though I'm also toying with the idea of doing it during RenderLayerCompositor::updateBackingAndHierarchy()). The advantage is that we'll be able to keep track of the scrolling-tree ancestor, and maybe compute geometry during that existing walk.

So it would be great if you could split this patch and upload a patch with just the scrolling tree changes to include the parent-relative scrollable rect.

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

(In reply to Simon Fraser (smfr) from comment #50)
> I've been working on some code to update the scrolling tree during a new
> GraphicsLayer tree walk post-flush in RenderLayerCompositor (though I'm also
> toying with the idea of doing it during
> RenderLayerCompositor::updateBackingAndHierarchy()). The advantage is that
> we'll be able to keep track of the scrolling-tree ancestor, and maybe
> compute geometry during that existing walk.
>

Great to hear that.
 
> So it would be great if you could split this patch and upload a patch with
> just the scrolling tree changes to include the parent-relative scrollable
> rect.

Sure. So IIUC, you mean removing the changes to inRenderLayerCompositor* as well as the associated LayoutTests update?

[Simon Fraser (smfr)]

(In reply to Frédéric Wang (:fredw) from comment #51)
> (In reply to Simon Fraser (smfr) from comment #50)
> > I've been working on some code to update the scrolling tree during a new
> > GraphicsLayer tree walk post-flush in RenderLayerCompositor (though I'm also
> > toying with the idea of doing it during
> > RenderLayerCompositor::updateBackingAndHierarchy()). The advantage is that
> > we'll be able to keep track of the scrolling-tree ancestor, and maybe
> > compute geometry during that existing walk.
> >
> 
> Great to hear that.
>  
> > So it would be great if you could split this patch and upload a patch with
> > just the scrolling tree changes to include the parent-relative scrollable
> > rect.
> 
> Sure. So IIUC, you mean removing the changes to inRenderLayerCompositor* as
> well as the associated LayoutTests update?

Right.

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

Created attachment 355872 [details]
Patch

Extracting only the set/get/dump APIs.

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

Created attachment 355873 [details]
Patch (tentative layout tests adjustments)

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

@smfr: I've extracted relevant changes in  attachment 355872 [details] (code) and attachment 355873 [details] (layout tests). The test failure on iOS seems unrelated, see bug 192088

[WebKit Commit Bot]

Comment on attachment 355872 [details]
Patch

Rejecting attachment 355872 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 355872, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=355872&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=172914&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 355872 from bug 172914.
Fetching: https://bugs.webkit.org/attachment.cgi?id=355872
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebCore/ChangeLog' is out of date
W: 73100f3605242fdc3c69d247b9ab0bbf1b71ca01 and refs/remotes/origin/master differ, using rebase:
:040000 040000 065eee271eac953bd2a6b0d881d0a2e1a1b94ac8 0e9cf8475f83a4372cc2a8703579faac5683073a M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebCore/ChangeLog' is out of date
W: 73100f3605242fdc3c69d247b9ab0bbf1b71ca01 and refs/remotes/origin/master differ, using rebase:
:040000 040000 065eee271eac953bd2a6b0d881d0a2e1a1b94ac8 0e9cf8475f83a4372cc2a8703579faac5683073a M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   25552c067c9..cb3f7e0b42d  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 238638 = 25552c067c9607b197375e5b239990c1cbc021c2
r238639 = c0ebda636f1d838e44ce2d48ed4a5774d2053adf
r238640 = cb3f7e0b42d592cea3ba7e26e2a895fd0098aacc
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
	M	Tools/TestWebKitAPI/Tests/WebKitCocoa/SafeBrowsing.mm
	M	Tools/ChangeLog
r238641 = 0dfb93794b73475ceee7a825335cd99ba8b1ab6a (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/10186730

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

Created attachment 355997 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 355997 [details]:

inspector/model/remote-object-api.html bug 192144 (author: [e-mail])
media/modern-media-controls/media-documents/media-document-invalid.html bug 192145 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 355997 [details]:

http/tests/webgl/1.0.2/texSubImage2DHTML.html bug 192146 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 355997 [details]
Patch for landing

Clearing flags on attachment: 355997

Committed r238665: <https://trac.webkit.org/changeset/238665>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.