Bug 172914 - Add ParentRelativeScrollableRect to ScrollingCoordinator::ScrollingGeometry
Summary: Add ParentRelativeScrollableRect to ScrollingCoordinator::ScrollingGeometry
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 176684
Blocks: 171667 172917
  Show dependency treegraph
 
Reported: 2017-06-05 01:23 PDT by Frédéric Wang (:fredw)
Modified: 2017-10-30 09:16 PDT (History)
5 users (show)

See Also:


Attachments
Patch (22.25 KB, patch)
2017-06-05 01:57 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.29 MB, application/zip)
2017-06-05 02:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (18.15 MB, application/zip)
2017-06-05 03:05 PDT, Build Bot
no flags Details
Patch (31.04 KB, patch)
2017-06-05 04:41 PDT, Frédéric Wang (:fredw)
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (test for iframes boxes) (15.11 KB, patch)
2017-06-20 06:03 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (WIP) (42.23 KB, patch)
2017-09-06 08:17 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.33 MB, application/zip)
2017-09-06 09:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.56 MB, application/zip)
2017-09-06 09:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (2.13 MB, application/zip)
2017-09-06 10:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.30 MB, application/zip)
2017-09-06 10:12 PDT, Build Bot
no flags Details
Patch (WIP) (34.81 KB, patch)
2017-09-08 10:08 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (39.28 KB, patch)
2017-09-12 06:20 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (39.28 KB, patch)
2017-09-13 02:05 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.50 MB, application/zip)
2017-09-13 03:20 PDT, Build Bot
no flags Details
Patch (41.80 KB, patch)
2017-09-15 09:58 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (41.92 KB, patch)
2017-09-26 07:26 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (41.98 KB, patch)
2017-09-26 08:12 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (41.88 KB, patch)
2017-10-30 09:16 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2017-06-05 01:23:49 PDT
This is necessary to perform hit testing in the scrolling tree for bug 171667.
Comment 1 Frédéric Wang (:fredw) 2017-06-05 01:57:00 PDT
Created attachment 312000 [details]
Patch
Comment 2 Build Bot 2017-06-05 02:51:46 PDT
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
Comment 3 Build Bot 2017-06-05 02:51:47 PDT
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
Comment 4 Build Bot 2017-06-05 03:05:34 PDT
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
Comment 5 Build Bot 2017-06-05 03:05:36 PDT
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
Comment 6 Frédéric Wang (:fredw) 2017-06-05 04:41:33 PDT
Created attachment 312004 [details]
Patch
Comment 7 Frédéric Wang (:fredw) 2017-06-05 05:28:14 PDT
@Simon: Can you please review that patch?
Comment 8 Frédéric Wang (:fredw) 2017-06-20 06:03:51 PDT
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.
Comment 9 Radar WebKit Bug Importer 2017-09-01 14:24:15 PDT
<rdar://problem/34215550>
Comment 10 Simon Fraser (smfr) 2017-09-05 08:46:29 PDT
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?
Comment 11 Frédéric Wang (:fredw) 2017-09-05 09:33:03 PDT
(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?
Comment 12 Simon Fraser (smfr) 2017-09-05 11:13:02 PDT
Right, yes.
Comment 13 Frédéric Wang (:fredw) 2017-09-06 08:17:00 PDT
Created attachment 320018 [details]
Patch (WIP)
Comment 14 Build Bot 2017-09-06 09:43:56 PDT
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
Comment 15 Build Bot 2017-09-06 09:43:58 PDT
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
Comment 16 Build Bot 2017-09-06 09:49:16 PDT
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
Comment 17 Build Bot 2017-09-06 09:49:18 PDT
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
Comment 18 Build Bot 2017-09-06 10:00:34 PDT
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
Comment 19 Build Bot 2017-09-06 10:00:36 PDT
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
Comment 20 Build Bot 2017-09-06 10:12:54 PDT
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
Comment 21 Build Bot 2017-09-06 10:12:56 PDT
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
Comment 22 Frédéric Wang (:fredw) 2017-09-08 10:08:40 PDT
Created attachment 320273 [details]
Patch (WIP)
Comment 23 Frédéric Wang (:fredw) 2017-09-12 06:20:46 PDT
Created attachment 320541 [details]
Patch
Comment 24 Frédéric Wang (:fredw) 2017-09-13 02:05:35 PDT
Created attachment 320626 [details]
Patch
Comment 25 Build Bot 2017-09-13 03:20:14 PDT
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
Comment 26 Build Bot 2017-09-13 03:20:16 PDT
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
Comment 27 Simon Fraser (smfr) 2017-09-13 13:31:31 PDT
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).
Comment 28 Frédéric Wang (:fredw) 2017-09-13 13:52:49 PDT
(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)?
Comment 29 Simon Fraser (smfr) 2017-09-13 13:54:08 PDT
(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).
Comment 30 Frédéric Wang (:fredw) 2017-09-13 13:56:47 PDT
(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.
Comment 31 Frédéric Wang (:fredw) 2017-09-15 09:58:46 PDT
Created attachment 320918 [details]
Patch
Comment 32 Frédéric Wang (:fredw) 2017-09-26 07:26:39 PDT
Created attachment 321814 [details]
Patch

Small modification to use RenderLayer::convertToLayerCoords
Comment 33 Frédéric Wang (:fredw) 2017-09-26 08:12:14 PDT
Created attachment 321815 [details]
Patch
Comment 34 Frédéric Wang (:fredw) 2017-10-30 09:16:10 PDT
Created attachment 325353 [details]
Patch

Rebasing...