Note: this also causes us to scroll on the compositor when we have a non-composited position: fixed layer, for e.g.

By the way, I just came across this TODO:

// TODO(pdr): Set the scroll node's non_fast_scrolling_region value.

here:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/graphics/compositing/property_tree_manager.cc?l=306

Is that still relevant?

We discussed how to fix this in the meeting today and David's going to add it to his plate. (We can re-assign back if needed)

So the issue here is that we don't populate the non-fast scroll region in ScrollNodes on the CC side with BGPT (without BGPT they get populated when we build property trees). The LayerImpl has the correct non-fast scroll region on it but we read the ScrollNode when deciding whether to scroll on impl or main.

It's set on the cc::Layer from ScrollingCoordinator::SetShouldHandleScrollGestureOnMainThreadRegion which happens in UpdateAfterPaint. I think this method should check the BGPT flag and set the MainThreadRegion on the cc ScrollNode if it's enabled (using a new method on PaintArtifactCompositor? Or just directly create a PropertyTreeManager?).

Alternatively, we could either copy the regions from LayerImpl to ScrollNode on tree activation. Or get the LayerImpl from the ScrollNode and check that. Though it doesn't seem like there's a good way to go from ScrollNode to LayerImpl, we could pass the LayerImpl into TryScroll.

IMHO, we're trying to get away from using LayerImpls so I think setting the region on ScrollNodes directly from Blink is the right long-term fix. pdr@, WDYT?

Nice investigation!

There's another project, PaintTouchActionRects, that's using paint to generate touch-action hit testing data for setting on layer.h, instead of using ScrollingCoordinator. It's looking like the PaintTouchActionRects design will be able to handle non-fast regions on layers too, so moving in that direction may be the best approach**.

WDYT of removing the cc scroll node's non_fast_scrollable_region member and just relying on the layer/layer_impl's non_fast_scrollable_region? This will work for the current code and BlinkGenPropertyTrees, and will be SPV2-compatible as long as the PaintTouchActionRects approach succeeds.

In terms of implementation: We could add a function, LayerTreeImpl::GetScrollLayerByElementId(ElementId), to look up the LayerImpl from the scroll node's element_id in TryScroll. This would probably need to be backed by a map from ElementId to scrolling layer, similar to LayerTreeHost's element_layers_map_ (which should be element_scrolling_layers_map, see  https://crbug.com/702777 ).




** You are right that we're moving a lot of data off layers and onto the property trees, but we recently learned that hit testing actually does belong on layers. One reason for this is that hit testing requires order. Another reason is that the property tree nodes are not granular enough for general hit test rects. For example:
<div transform>
  <div effect1 + event handler>
  <div effect2>
</div>
Lets say this produces 2 layers, one for each effect div. With hit test rects on transform nodes, we do not have enough information to put an event handler on only one layer.

Scrolling involves both hierarchy (scroll tree) and hit testing which is why it is a special case where we can assume the 1:1 mapping between the layers and scroll nodes.

Got it, thanks for the info.

Ok, using LayerImpl's sgtm. That should work pre-BGPT too so we don't have to gate removing the Region from the ScrollNode on the BGPT flag, right?

> In terms of implementation: We could add a function, LayerTreeImpl::GetScrollLayerByElementId(ElementId)...

TryScroll is the only user of ScrollNode's MainThreadScrollRegion and it's only caller has a LayerImpl that it converts to a ScrollNode. So I think we can avoid adding additional maps (for now, at least) and just pass the LayerImpl (and get the ScrollNode, needed for other reasons, inside TryScroll).

Ok, if that sounds good I'll start making that change.

Re: "Ok, using LayerImpl's sgtm. That should work pre-BGPT too so we don't have to gate removing the Region from the ScrollNode on the BGPT flag, right?"

Yeah, this refactoring will be for BGPT and !BGPT.

Re: "TryScroll is the only user of ScrollNode's MainThreadScrollRegion and it's only caller has a LayerImpl that it converts to a ScrollNode. So I think we can avoid adding additional maps (for now, at least) and just pass the LayerImpl (and get the ScrollNode, needed for other reasons, inside TryScroll)."

(nit: I think you mean non_fast_scrollable_region and not MainThreadScrollRegion)

The callsite in LayerTreeHostImpl::FindScrollNodeForDeviceViewportPoint does have a layer_impl but the code then iterates up the scroll tree. Pre-layer-lists, this could be thought of as an ancestor walk of the layer_impls. Because of this, I think the element_id->layer_impl map will be needed. We could do a simple implementation of LayerTreeImpl::GetScrollLayerByElementId that iterates through all layers and returns the first layer with the correct element id. This would make LayerTreeHostImpl::FindScrollNodeForDeviceViewportPoint O(|layers|^2) though.

Right, I glanced at it and missed the loop. I'll use the map approach then.

(and yes, I did mean non_fast_scrollable_region :)

 Issue 883444  has been merged into this issue.

I've got a CL that fixes this but I'm wondering if it may be better to keep a layer_id field in ScrollNode.

I'm worried about the complexity of keeping a hash map in LTI up to date. For example, there's methods to change the ElementId on a layer, we'd have to manage the map there as well as when a layer becomes scrollable. It looks like a layer can't unbecome scrollable?

By contrast, it seems like it would be less error-prone and more performant to just tack on the layer_id to a scroll_node. Is there a reason we want to avoid that? WDYT?

I have both versions implemented so it's just a matter of preference.

I'd prefer to not put a layer_id on scroll node because it would be a difference between the blink and cc property nodes which would be non-trivial to resolve if we merge the datastructures. Because of this, I'm leaning towards the map approach, but if the other approach is slow/complex I could be convinced.

I'm hoping the code will look similar to the map logic in LayerTreeHost, plus the scrollable check that you point out. Maybe we can add some DCHECKS that layers don't become unscrollable?

Ok, that makes sense. I'll send you a patch later today.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a19ae010b1c420cb3650a29a72a5ef6ac93f627

commit 4a19ae010b1c420cb3650a29a72a5ef6ac93f627
Author: David Bokan <bokan@chromium.org>
Date: Sat Sep 15 13:59:27 2018

[blink-gen-property-trees] Fix NonFastScrollRegion

This CL fixes non-fast-scroll-region when blink-gen-property-trees is
turned on.

NonFastScrollRegions are added onto a cc::Layer to specify that
scrolling within the region must be handled on the main thread (for
example, becauser there's a non-composited scroller there).

Pre-BGPT, these were copied from the cc::Layer to the cc::ScrollNode
during property tree building. With BGPT, the property trees are built
in Blink but the NonFastScrollRegion is now only stored on the
cc::Layer. See disucssion in the bug for the reasoning.

This patch makes the compositor read the regions off the cc::Layer
rather than the ScrollNode. This requires the ability to get a Layer
given a ScrollNode. We add a map in LayerTreeImpl from element_id to
scrollable LayerImpls for this reason.

Bug:  877576 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Id9f545d50b7a90177c21b60668a6d93f3afb2106
Reviewed-on: https://chromium-review.googlesource.com/1223248
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591586}
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/layers/layer_impl.cc
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/property_tree.h
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/scroll_node.cc
[modify] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/cc/trees/scroll_node.h
[add] https://crrev.com/4a19ae010b1c420cb3650a29a72a5ef6ac93f627/third_party/WebKit/LayoutTests/fast/scrolling/scroll-non-composited-scroller.html