Bug 171667

Created attachment 309057 [details] WIP Patch This is a first patch to address the points: > 1. Creating the right layer hierarchy with tiled backing store for the iframe contents, as we do for the main frame. > 4. Not putting iframe's ScrollableAreas into the non-fast scrollable region This is the layer tree I get for attachment 307586 [details] after applying this patch: layer 0x110b5f138 at (0,0) size 1039x644 (composited, bounds=at (0,0) size 1039x644, drawsContent=1, paints into ancestor=0) RenderView 0x1113dad00 at (0,0) size 1039x644 (needs layout: child) positive z-order list(1) layer 0x110b5f4e0 at (0,0) size 1039x166 RenderBlock 0x110a627e0 {HTML} at (0,0) size 1039x166 (needs layout: child) RenderBody 0x110a62c60 {BODY} at (8,16) size 1023x142 (needs layout: child) RenderBlock 0x110a62d80 {P} at (0,0) size 1023x18 RenderText 0x110ae49c0 {#text} at (0,0) size 211x18 text run at (0,0) width 211: "This iframe should be scrollable:" RenderBlock (anonymous) 0x110a62ea0 at (0,34) size 1023x108 (needs layout: child) RenderText 0x110ae4b40 {#text} at (0,0) size 0x0 normal flow list(1) layer 0x110b5f750 at (0,0) size 0x0 RenderIFrame 0x110b556f0 {IFRAME} at (0,0) size 304x104 [border: (2px inset #000000)] (needs layout: self) layer 0x110b5fd68 at (0,0) size 300x1016 (composited, bounds=at (0,0) size 300x1016, drawsContent=1, paints into ancestor=0) RenderView 0x1113f6d00 at (0,0) size 300x100 (layout overflow 0,0 300x1016) positive z-order list(1) layer 0x110b5fea0 at (0,0) size 300x1016 RenderBlock 0x110a62a20 {HTML} at (0,0) size 300x1016 RenderBody 0x110ae8360 {BODY} at (8,8) size 284x1000 RenderBlock 0x110ae8480 {DIV} at (0,0) size 284x1000 Also, the frame is no longer drawn as a non-fast scrollable region when one enables debug overlay. However, this breaks scrolling of the iframe for now :-) @Simon: I'm not sure the scrolling nodes are properly created yet, but I had one doubt. Again, IIUC the class hierarchy (https://people.igalia.com/fwang/ios/class-hierarchy/), each frame has its own Page with its own ScrollingCoordinator and hence their own Scrolling(State)Trees. So I am not sure I understand to which tree the ScrollingStateFrameScrollingNodes should belong? BTW, I also noticed this comment in RenderLayerCompositor::updateBacking, which might be related: // At this time, the ScrollingCoordinator only supports the top-level frame. if (layer.isRootLayer() && isMainFrameCompositor()) {

Created attachment 309151 [details] WIP Patch New version, trying to get the correct scrolling tree. This is what I get for attachment 307586 [details]: (Frame scrolling node (scrollable area size 924 465) (contents size 924 465) (layout viewport at (0,0) size 924x465) (min layout viewport origin (0,0)) (max layout viewport origin (0,0)) (fixed behavior: stick to viewport) (children 1 (Frame scrolling node (scrollable area size 285 100) (contents size 285 1016) (layout viewport at (0,0) size 285x100) (min layout viewport origin (0,0)) (max layout viewport origin (0,916)) (fixed behavior: stick to viewport) ) ) )

Created attachment 309154 [details] testcase

> 1. Creating the right layer hierarchy with tiled backing store for the iframe contents, as we do for the main frame. > 2. Creating ScrollingStateFrameScrollingNodes for them > 4. Not putting iframe's ScrollableAreas into the non-fast scrollable region These points are handled by attachment 309151 [details]. > 3. Making sure the scrolling tree handles nested ScrollingStateFrameScrollingNodes properly I am not sure what is expected here, but the scrolling tree in comment 2 looks good to me and I did not find any specific problem with nested ScrollingStateFrameScrollingNodes for now. > 5. Teaching the scrolling thread to direct a mouse wheel at the appropriate ScrollingTreeFrameScrollingNode So this is going to be the next task for me next week. The main problem is that ScrollingTree::handleWheelEvent sends the wheel event to the root of the scrolling tree. I tested a quick hack to forward the event to its children: - if (m_rootNode) + if (m_rootNode) { + for (auto& child : *m_rootNode->children()) { + downcast<ScrollingTreeScrollingNode>(*child).handleWheelEvent(wheelEvent); + } downcast<ScrollingTreeScrollingNode>(*m_rootNode).handleWheelEvent(wheelEvent); + } and this makes the iframes wheel-scroll correctly. So it seems the only remaining thing is to find the element that should receive the event. There is some logic in EventHandler::handleWheelEvent EventHandlerMac's EventHandler::platformPrepareForWheelEvents and findEnclosingScrollableContainer that should probably be shared i.e. we find the candidate to scroll and use the ThreadedScrollingTree (if it is fast-scrollabled) or delegate to the main thread otherwise (as currently always done). @Simon: Any suggestion about how to do this refactoring is welcomed ;-) > 6. Making sure that scroll latching works I tried it on the testcase with my little hack above and that seems to work too ;-)

(In reply to Frédéric Wang (:fredw) from comment #4) > > 1. Creating the right layer hierarchy with tiled backing store for the iframe contents, as we do for the main frame. > > 2. Creating ScrollingStateFrameScrollingNodes for them > > 4. Not putting iframe's ScrollableAreas into the non-fast scrollable region > > These points are handled by attachment 309151 [details]. > > > 3. Making sure the scrolling tree handles nested ScrollingStateFrameScrollingNodes properly > > I am not sure what is expected here, but the scrolling tree in comment 2 > looks good to me and I did not find any specific problem with nested > ScrollingStateFrameScrollingNodes for now. This is things like making sure the following work: * fast-scrolling iframe gaining or losing a position:fixed ancestor * fast-scrolling iframe inside fast-scrolling overflow:scroll (doesn't happen on Mac now, but will happen on iOS, and on Mac when overflow becomes fast-scrollable) * fast-scrolling iframe gains or loses fast-scrolling overflow:scroll ancestor > > 5. Teaching the scrolling thread to direct a mouse wheel at the appropriate ScrollingTreeFrameScrollingNode > > So this is going to be the next task for me next week. The main problem is > that ScrollingTree::handleWheelEvent sends the wheel event to the root of > the scrolling tree. I tested a quick hack to forward the event to its > children: > > - if (m_rootNode) > + if (m_rootNode) { > + for (auto& child : *m_rootNode->children()) { > + > downcast<ScrollingTreeScrollingNode>(*child).handleWheelEvent(wheelEvent); > + } > > downcast<ScrollingTreeScrollingNode>(*m_rootNode). > handleWheelEvent(wheelEvent); > + } > > and this makes the iframes wheel-scroll correctly. So it seems the only > remaining thing is to find the element that should receive the event. There > is some logic in > > EventHandler::handleWheelEvent > EventHandlerMac's EventHandler::platformPrepareForWheelEvents and > findEnclosingScrollableContainer > > that should probably be shared i.e. we find the candidate to scroll and use > the ThreadedScrollingTree (if it is fast-scrollabled) or delegate to the > main thread otherwise (as currently always done). @Simon: Any suggestion > about how to do this refactoring is welcomed ;-) I haven't really thought about this, but it's basically a hit-test on the scrolling tree. We'll have to make sure that transformed frames work OK, and those with elements on top from the parent frame. > > 6. Making sure that scroll latching works > > I tried it on the testcase with my little hack above and that seems to work > too ;-)

(In reply to Simon Fraser (smfr) from comment #5) > This is things like making sure the following work: > * fast-scrolling iframe gaining or losing a position:fixed ancestor > * fast-scrolling iframe inside fast-scrolling overflow:scroll (doesn't > happen on Mac now, but will happen on iOS, and on Mac when overflow becomes > fast-scrollable) > * fast-scrolling iframe gains or loses fast-scrolling overflow:scroll > ancestor I see. More things to test :-) > I haven't really thought about this, but it's basically a hit-test on the > scrolling tree. We'll have to make sure that transformed frames work OK, and > those with elements on top from the parent frame. Right, however the code I mentioned does more work (for example if an iframe is already scrolled to its maximum in the scroll direction, then we have to forward the scroll to another scrollable element e.g. an ancestor). Also, I believe if we have a non-fast scrollable elements inside a fast-scrollable element (if that's ever possible) then we should not select that fast-scrollable element from the scrolling tree but instead redirect the scroll event to the non-fast scrollable using the main thread.

@Simon: I have some more questions on this: > I haven't really thought about this, but it's basically a hit-test on the scrolling tree. We'll have to make sure that transformed frames work OK, and those with elements on top from the parent frame. 1) I'm not sure I understand how to perform the hit-test. My initial thought was to do like the slow path (EventHandler::handleWheelEvent and EventHandlerMac's EventHandler::platformPrepareForWheelEvents and findEnclosingScrollableContainer) but it's probably not a good idea (or even permitted) to check the DOM tree here. Using only the scrolling tree, I don't see much data available but ScrollingStateFrameScrollingNode::m_eventTrackingRegions to detect whether the mouse position is in a frame. It will probably not work for general scrolling nodes though. Was the plan to attach more data into the scrolling tree for that purpose? 2) I see FIXME comments in the following functions mentioned handling SynchronousScrollingReasons for more than the main frame: ScrollingCoordinator::updateSynchronousScrollingReasons ScrollingCoordinator::replaySessionStateDidChange and there are already data in the corresponding Scrolling{Tree|State}FrameScrollingNode. Do you what's the plan here and what should be done exactly? It looks like this is a kind of refactoring that could be done in a preliminary step/bug.

Created attachment 309595 [details] WIP Patch

Attachment 309595 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/RenderLayerCompositor.h:463: The parameter name "layer" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 309595 [details] WIP Patch Attachment 309595 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3711637 Number of test failures exceeded the failure limit.

Created attachment 309598 [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 on attachment 309595 [details] WIP Patch Attachment 309595 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3711764 New failing tests: compositing/iframes/scrolling-iframe.html

Created attachment 309602 [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.11.6

Created attachment 309610 [details] Patch > 2) I see FIXME comments in the following functions mentioned handling SynchronousScrollingReasons for more than the main frame: I'm updating this patch to hopefully address this FIXME. Preliminary refactoring moved to bug 171923.

Comment on attachment 309610 [details] Patch Attachment 309610 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3762691 Number of test failures exceeded the failure limit.

Created attachment 310386 [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 on attachment 309610 [details] Patch Attachment 309610 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3762790 New failing tests: compositing/iframes/scrolling-iframe.html

Created attachment 310392 [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.11.6

Comment on attachment 309610 [details] Patch Attachment 309610 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3763253 New failing tests: compositing/iframes/scrolling-iframe.html

Created attachment 310399 [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.11.6

Created attachment 311133 [details] Patch

> 18:45:47 - smfr : fredw: i think ScrollingTreeScrollingNode will need an offset from its parent or something > 18:46:37 - fredw : smfr: OK, that makese sense. IIRC the size is already available. Not sure about the transforms you mentioned in the past, or even the "z-index" > 18:47:32 - smfr : fredw: we probabty should just bail out of there’s a non-axis-aligned transform > 18:47:53 - smfr : handling overlapping elements will be tricky; we’ll need to compute overlap somewhere > 18:48:35 - fredw : smfr: bail out = you mean use the non-fast path, right? > 18:48:59 - smfr : right @Simon: So I just uploaded a new patch that adds an "offset from parent" value. I was wondering what size should I use ScrollableTreeScrollingNode::m_reachableContentsSize?

Created attachment 311348 [details] Patch This patch implements basic support but there are still many things to do, for example: 1) do not pass the scrolling event to a box scrolled at maximum 2) transformations 3) overlapping I discussed a bit with a colleague working on scrolling in Servo. For 2), it seems we can just apply the transform to the mouse position in order to move to the child's coordinate (similarly to what is done for the offsetInParent). For 3), Servo uses the display list which provides a better browsing order to get the clicked child.

Created attachment 311463 [details] Patch do not pass the scrolling event to a box scrolled at maximum

Attachment 311463 [details] did not pass style-queue: ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:70: The parameter name "wheelEvent" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 311463 [details] Patch Attachment 311463 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3836006 Number of test failures exceeded the failure limit.

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

Created attachment 311465 [details] testcase More test cases.

Created attachment 311466 [details] Patch

Created attachment 311503 [details] Patch (do not use async scrolling when there are transformed nodes) This is an attempt to avoid async scrolling when there are transformed nodes, but it does not work.

Comment on attachment 311503 [details] Patch (do not use async scrolling when there are transformed nodes) Moved to bug 173354.

Created attachment 312873 [details] Patch Attachment 311466 [details] has been split into "Depends on" bugs, so the only remaining change here is the defaultScrollingTreeIncludesFrames setting.

Created attachment 313839 [details] Test Patch to enable async frame scrolling

Comment on attachment 313839 [details] Test Patch to enable async frame scrolling minibrowser and iOS11 have UI to enable async scrolling, so this patch is no longer useful.

This is complete now.