Bug 171667 – [Mac] Make frames fast-scrollable

• Component: Frames
• Status: RESOLVED CONFIGURATION CHANGED

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

From bug 149264 comment 17, this involves:
> 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
> 3. Making sure the scrolling tree handles nested ScrollingStateFrameScrollingNodes properly
> 4. Not putting iframe's ScrollableAreas into the non-fast scrollable region
> 5. Teaching the scrolling thread to direct a mouse wheel at the appropriate ScrollingTreeFrameScrollingNode
> 6. Making sure that scroll latching works
> 7. Probably more

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

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()) {

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

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)
    )
  )
)

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

Created attachment 309154 [details]
testcase

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

> 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 ;-)

[Simon Fraser (smfr)]

(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 ;-)

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

(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.

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

@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.

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

Created attachment 309595 [details]
WIP Patch

[Build Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

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

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

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

Created attachment 311133 [details]
Patch

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

> 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?

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

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.

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

Created attachment 311463 [details]
Patch

do not pass the scrolling event to a box scrolled at maximum

[Build Bot]

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.

[Build Bot]

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.

[Build Bot]

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

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

Created attachment 311465 [details]
testcase

More test cases.

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

Created attachment 311466 [details]
Patch

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

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.

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

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

Moved to bug 173354.

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

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.

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

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

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

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.

[Simon Fraser (smfr)]

This is complete now.