Compositor hit-testing considers scrollable layers that scroll a drawn RSLL

Compositor hit-testing considers scrollable layers that scroll a drawn RSLL

Compositor should include a scrollable layer in hit testing if it scrolls at
least one drawn RSSL layer which can be itself, its descendant, or its scroll
descendant.

BUG=455439
Committed: https://crrev.com/6cfcc368c8c389a534441b491eab3fbe606b2321
Cr-Commit-Position: refs/heads/master@{#319496}

[majidvp, 2015-02-10 18:14:12]

majidvp@chromium.org changed reviewers:
+ vollick@chromium.org

[Ian Vollick, 2015-02-24 02:58:24]

On 2015/02/10 18:14:13, majidvp wrote:

My recollection of why I was insisting on scrolling a drawn RSLL member was to
1) ensure that when I ported to hit testing to layer trees from RSLL I didn't
break stuff, and 2) to restrict scrolling to cases where the visuals would be
updated.

I could believe that 2) is a bad restriction, but I'm curious about your
particular case. IIUC, you've got an iframe that can't scroll because it reports
that it doesn't scroll a drawn RSLL member. Presumably visuals _would_ scroll if
things were working correctly? That suggests to me that
ScrollsAnyDrawnRenderSurfaceLayerListMember might be busted for iframes? If you
could attach a dump of the layer tree, that'd really help.

[majidvp, 2015-02-24 22:51:31]

On 2015/02/24 02:58:24, vollick wrote:
> On 2015/02/10 18:14:13, majidvp wrote:
> 
> My recollection of why I was insisting on scrolling a drawn RSLL member was to
> 1) ensure that when I ported to hit testing to layer trees from RSLL I didn't
> break stuff, and 2) to restrict scrolling to cases where the visuals would be
> updated.
> 
> I could believe that 2) is a bad restriction, but I'm curious about your
> particular case. IIUC, you've got an iframe that can't scroll because it
reports
> that it doesn't scroll a drawn RSLL member. Presumably visuals _would_ scroll
if
> things were working correctly? That suggests to me that
> ScrollsAnyDrawnRenderSurfaceLayerListMember might be busted for iframes? If
you
> could attach a dump of the layer tree, that'd really help.

Here is the dump of layer tree with some additional info related to
ScrollsAnyDrawnRenderSurfaceLayerListMember:
https://code.google.com/p/chromium/issues/attachmentText?id=455439&aid=455439...

The layer of interest is #27 (and maybe #29) which belongs to the iframe.
It fails the ScrollsAnyDrawnRenderSurfaceLayerListMember tests but it is
scrolling layer.

The current implementation of  ScrollsAnyDrawnRenderSurfaceLayerListMember only
passes
a non-drawn but scrollable layer if it has an immediate children in the drawn
RSLL member. 
To me it seems that in the iframe case, the drawn RSLL is not its child but its
grand child?!

Here is the test page: http://jsbin.com/jusedi/quiet

[majidvp, 2015-03-04 15:00:39]

On 2015/02/24 22:51:31, majidvp wrote:
> On 2015/02/24 02:58:24, vollick wrote:
> > On 2015/02/10 18:14:13, majidvp wrote:
> > 
> > My recollection of why I was insisting on scrolling a drawn RSLL member was
to
> > 1) ensure that when I ported to hit testing to layer trees from RSLL I
didn't
> > break stuff, and 2) to restrict scrolling to cases where the visuals would
be
> > updated.
> > 
> > I could believe that 2) is a bad restriction, but I'm curious about your
> > particular case. IIUC, you've got an iframe that can't scroll because it
> reports
> > that it doesn't scroll a drawn RSLL member. Presumably visuals _would_
scroll
> if
> > things were working correctly? That suggests to me that
> > ScrollsAnyDrawnRenderSurfaceLayerListMember might be busted for iframes? If
> you
> > could attach a dump of the layer tree, that'd really help.
> 
> Here is the dump of layer tree with some additional info related to
> ScrollsAnyDrawnRenderSurfaceLayerListMember:
>
https://code.google.com/p/chromium/issues/attachmentText?id=455439&aid=455439...
> 
> The layer of interest is #27 (and maybe #29) which belongs to the iframe.
> It fails the ScrollsAnyDrawnRenderSurfaceLayerListMember tests but it is
> scrolling layer.
> 
> The current implementation of  ScrollsAnyDrawnRenderSurfaceLayerListMember
only
> passes
> a non-drawn but scrollable layer if it has an immediate children in the drawn
> RSLL member. 
> To me it seems that in the iframe case, the drawn RSLL is not its child but
its
> grand child?!
> 
> Here is the test page: http://jsbin.com/jusedi/quiet


Below are the relevant layers for composited iframe with their
scrollability and drawn properties. I wonder if layer #28 (RenderView)
should be marked as scrollable instead of layer #27  (Scrolling Layer). 
When I enable '--root-layer-scrolls' the issue is resolved because 
the RenderView layer will parent a subtree that includes
all internal iframe layers. So if that is going to land soon we may be
able to get away without this change.


Regarding 2) to restrict scrolling to cases where the visuals would be
updated.

FindFirstScrollingLayerThatIsHitByPoint is only used once and it is to
find the top most scrolling layer. The result is not actually scrolled
but only used to detect it is a grandparent of the the current top
most layer. If it is not then it is an edge case where CC defers
scrolling to blink. If we remove this restriction at worst it will
results in more cases where scrolling goes through the slow path.
Is it safe to assume it will be quite rare to have a hidden scrollable
overlayed with another layer which is not a direct descendant of it?

29 RenderPartObject IFRAME, reasons(Is an accelerated iFrame)
  bounds: (404, 404)
  scrollable: false, drawn: true
  25 Overflow Controls Host Layer, (hidden)
    bounds: (400, 400)
    scrollable: false, drawn: false
    26 LocalFrame Clipping Layer, (hidden)
      bounds: (400, 400)
      scrollable: false, drawn: false
      27 LocalFrame Scrolling Layer, (hidden)
        bounds: (1008, 1016)
        scrollable: true, drawn: false
        24 Content Root Layer, (hidden)
          bounds: (1008, 1016)
          draw_transform:
          scrollable: false, drawn: false
          28 RenderView #document, reasons(Is the root layer)
            bounds: (1008, 1016)
            scrollable: false, drawn: true

[majidvp, 2015-03-06 18:49:53]

On 2015/03/04 15:00:39, majidvp wrote:
> On 2015/02/24 22:51:31, majidvp wrote:
> > On 2015/02/24 02:58:24, vollick wrote:
> > > On 2015/02/10 18:14:13, majidvp wrote:
> > > 
> > > My recollection of why I was insisting on scrolling a drawn RSLL member
was
> to
> > > 1) ensure that when I ported to hit testing to layer trees from RSLL I
> didn't
> > > break stuff, and 2) to restrict scrolling to cases where the visuals would
> be
> > > updated.
> > > 
> > > I could believe that 2) is a bad restriction, but I'm curious about your
> > > particular case. IIUC, you've got an iframe that can't scroll because it
> > reports
> > > that it doesn't scroll a drawn RSLL member. Presumably visuals _would_
> scroll
> > if
> > > things were working correctly? That suggests to me that
> > > ScrollsAnyDrawnRenderSurfaceLayerListMember might be busted for iframes?
If
> > you
> > > could attach a dump of the layer tree, that'd really help.
> > 
> > Here is the dump of layer tree with some additional info related to
> > ScrollsAnyDrawnRenderSurfaceLayerListMember:
> >
>
https://code.google.com/p/chromium/issues/attachmentText?id=455439&aid=455439...
> > 
> > The layer of interest is #27 (and maybe #29) which belongs to the iframe.
> > It fails the ScrollsAnyDrawnRenderSurfaceLayerListMember tests but it is
> > scrolling layer.
> > 
> > The current implementation of  ScrollsAnyDrawnRenderSurfaceLayerListMember
> only
> > passes
> > a non-drawn but scrollable layer if it has an immediate children in the
drawn
> > RSLL member. 
> > To me it seems that in the iframe case, the drawn RSLL is not its child but
> its
> > grand child?!
> > 
> > Here is the test page: http://jsbin.com/jusedi/quiet
> 
> 
> Below are the relevant layers for composited iframe with their
> scrollability and drawn properties. I wonder if layer #28 (RenderView)
> should be marked as scrollable instead of layer #27  (Scrolling Layer). 
> When I enable '--root-layer-scrolls' the issue is resolved because 
> the RenderView layer will parent a subtree that includes
> all internal iframe layers. So if that is going to land soon we may be
> able to get away without this change.
> 
> 
> Regarding 2) to restrict scrolling to cases where the visuals would be
> updated.
> 
> FindFirstScrollingLayerThatIsHitByPoint is only used once and it is to
> find the top most scrolling layer. The result is not actually scrolled
> but only used to detect it is a grandparent of the the current top
> most layer. If it is not then it is an edge case where CC defers
> scrolling to blink. If we remove this restriction at worst it will
> results in more cases where scrolling goes through the slow path.
> Is it safe to assume it will be quite rare to have a hidden scrollable
> overlayed with another layer which is not a direct descendant of it?
> 
> 29 RenderPartObject IFRAME, reasons(Is an accelerated iFrame)
>   bounds: (404, 404)
>   scrollable: false, drawn: true
>   25 Overflow Controls Host Layer, (hidden)
>     bounds: (400, 400)
>     scrollable: false, drawn: false
>     26 LocalFrame Clipping Layer, (hidden)
>       bounds: (400, 400)
>       scrollable: false, drawn: false
>       27 LocalFrame Scrolling Layer, (hidden)
>         bounds: (1008, 1016)
>         scrollable: true, drawn: false
>         24 Content Root Layer, (hidden)
>           bounds: (1008, 1016)
>           draw_transform:
>           scrollable: false, drawn: false
>           28 RenderView #document, reasons(Is the root layer)
>             bounds: (1008, 1016)
>             scrollable: false, drawn: true

volick: PTAL.

[Ian Vollick, 2015-03-06 18:56:16]

https://codereview.chromium.org/868363004/diff/80001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/868363004/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:1456: for (std::set<LayerImpl*>::const_iterator it =
Iterating over the children caused the brittleness you're fixing, no? Could we
just return layer->draw_properties().layer_or_descendant_is_draw ||
layer->scroll_children() and skip this?

[majidvp, 2015-03-06 19:43:03]

https://codereview.chromium.org/868363004/diff/80001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/868363004/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:1456: for (std::set<LayerImpl*>::const_iterator it =
On 2015/03/06 18:56:16, vollick wrote:
> Iterating over the children caused the brittleness you're fixing, no? Could we
> just return layer->draw_properties().layer_or_descendant_is_draw ||
> layer->scroll_children() and skip this?

The issue was due to fact that layer was only checking itself and its
scroll children. It should have been checking itself, its descendant,
and its scroll descendant. Actually in the specific case I was testing
there was not any scroll_children() involved at all. The problem was 
caused by iframe scrolling layer being hidden itself while it
had a drawn descendant.

If we check only existence of scroll_children we may risk having 
false positive when scroll_children is actually not drawn.

[Ian Vollick, 2015-03-06 19:44:27]

On 2015/03/06 19:43:03, majidvp wrote:
>
https://codereview.chromium.org/868363004/diff/80001/cc/trees/layer_tree_impl.cc
> File cc/trees/layer_tree_impl.cc (right):
> 
>
https://codereview.chromium.org/868363004/diff/80001/cc/trees/layer_tree_impl...
> cc/trees/layer_tree_impl.cc:1456: for (std::set<LayerImpl*>::const_iterator it
=
> On 2015/03/06 18:56:16, vollick wrote:
> > Iterating over the children caused the brittleness you're fixing, no? Could
we
> > just return layer->draw_properties().layer_or_descendant_is_draw ||
> > layer->scroll_children() and skip this?
> 
> The issue was due to fact that layer was only checking itself and its
> scroll children. It should have been checking itself, its descendant,
> and its scroll descendant. Actually in the specific case I was testing
> there was not any scroll_children() involved at all. The problem was 
> caused by iframe scrolling layer being hidden itself while it
> had a drawn descendant.
> 
> If we check only existence of scroll_children we may risk having 
> false positive when scroll_children is actually not drawn.

I'm sorry, I misread your cl. This is totally reasonable. lgtm.

[majidvp, 2015-03-06 20:02:59]

On 2015/03/06 19:44:27, vollick wrote:
> On 2015/03/06 19:43:03, majidvp wrote:
> >
>
https://codereview.chromium.org/868363004/diff/80001/cc/trees/layer_tree_impl.cc
> > File cc/trees/layer_tree_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/868363004/diff/80001/cc/trees/layer_tree_impl...
> > cc/trees/layer_tree_impl.cc:1456: for (std::set<LayerImpl*>::const_iterator
it
> =
> > On 2015/03/06 18:56:16, vollick wrote:
> > > Iterating over the children caused the brittleness you're fixing, no?
Could
> we
> > > just return layer->draw_properties().layer_or_descendant_is_draw ||
> > > layer->scroll_children() and skip this?
> > 
> > The issue was due to fact that layer was only checking itself and its
> > scroll children. It should have been checking itself, its descendant,
> > and its scroll descendant. Actually in the specific case I was testing
> > there was not any scroll_children() involved at all. The problem was 
> > caused by iframe scrolling layer being hidden itself while it
> > had a drawn descendant.
> > 
> > If we check only existence of scroll_children we may risk having 
> > false positive when scroll_children is actually not drawn.
> 
> I'm sorry, I misread your cl. This is totally reasonable. lgtm.

Thanks. Just updated the CL description to make it more accurate.

[majidvp, 2015-03-06 20:40:55]

The CQ bit was checked by majidvp@chromium.org

[commit-bot: I haz the power, 2015-03-06 20:41:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/868363004/80001

[commit-bot: I haz the power, 2015-03-06 20:46:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-06 20:47:35]

Patchset 5 (id:??) landed as
https://crrev.com/6cfcc368c8c389a534441b491eab3fbe606b2321
Cr-Commit-Position: refs/heads/master@{#319496}