Overlay scrollbars must respect ancestor clip

Overlay scrollbars must respect ancestor clip

The current reparenting logic for overlay scrollbars for compositor-
driven scrollable areas does not account for ancestor clips. The result
is that the scrollbars can escape clips that need to affect them.

Consider the following stacking tree. (The '-' characters in the
following trees should be ignored. I've only added them so that
my indenting doesn't get stripped).

+ stacking-context
- + overflow-hidden-clipper
- + overflow-scroller
- + fixed-positioned-descendant-of-the-scroller
- + relative-positioned-descendant-of-the-scroller

Where, in tree order, these are arranged like this
+ stacking-context
- + overflow-hidden-clipper
--- + overflow-scroller
----- + fixed-positioned-descendant-of-the-scroller
----- + relative-positioned-descendant-of-the-scroller

Since overflow-scroller is clipped by overflow-hidden-clipper, a layer
that is not an ancestor in the stacking tree, it gets an ancestor
clipping layer. If the overflow controls are parented under
overflow-scroller's m_graphicsLayer, everything works fine: the
controls inherit the clip from the ancestor clipping layer.

Unfortunately, if we are using overlay scrollbars, we can't hang
the scrollbars under m_graphicsLayer because they'd stack behind
fixed-positioned-descendant-of-the-scroller and
relative-positioned-descendant-of-the-scroller. In order to get things
stacking correctly, we have to reparent the overflow controls like
this:

+ stacking-context
- + overflow-hidden-clipper
- + overflow-scroller
- + fixed-positioned-descendant-of-the-scroller
- + relative-positioned-descendant-of-the-scroller
- + <overflow controls for overflow-scroller>

If we reparent the controls like this in the graphics layer tree,
the overflow controls will no longer be a descendant of the
ancestor clipping layer that applies the clip for m_graphicsLayer,
unfortunately. In order to get the overflow controls clipped
correctly, we need to ensure that they have a clipping layer
above them. This CL adds m_overflowControlsClippingLayer for this
purpose.

BUG=393926
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178980

[Ian Vollick, 2014-07-15 20:54:36]

On 2014/07/15 20:02:45, Ian Vollick wrote:

Please hold off on reviewing this. I'm hoping I can share some intermediate
values w/ the code that computes the ancestor clipping layer's geometry.

[Ian Vollick, 2014-07-16 03:57:57]

On 2014/07/15 20:54:36, Ian Vollick wrote:
> On 2014/07/15 20:02:45, Ian Vollick wrote:
> 
> Please hold off on reviewing this. I'm hoping I can share some intermediate
> values w/ the code that computes the ancestor clipping layer's geometry.

Ugh, I don't think I can reuse the ancestor clipping layer code or intermediate
values since the compositing container could be different. Feel free to take a
look now, and please let me know if you see an opportunity for reuse.

[abarth-chromium, 2014-07-16 04:12:40]

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1797: LayoutRect
parentClipRect =
m_owningLayer.clipper().backgroundClipRect(clipRectsContext).rect();
This call is super expensive.  :(

[abarth-chromium, 2014-07-16 04:13:53]

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1794: // inaccurate
results (and trips the ASSERTS in RenderLayerClipper).
This comment isn't true.  We don't cache any clip rects like this.

[Ian Vollick, 2014-07-16 04:21:34]

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1794: // inaccurate
results (and trips the ASSERTS in RenderLayerClipper).
On 2014/07/16 04:13:53, abarth wrote:
> This comment isn't true.  We don't cache any clip rects like this.

This comment was lifted from the ancestor clip layer geometry update code. Maybe
that comment is also stale?

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1797: LayoutRect
parentClipRect =
m_owningLayer.clipper().backgroundClipRect(clipRectsContext).rect();
On 2014/07/16 04:12:40, abarth wrote:
> This call is super expensive.  :(

I know :( I was really hoping to reuse values from the ancestor clip layer here.
It might be possible, but I got scared off by the prospect of having different
compositing containers.

[abarth-chromium, 2014-07-16 05:28:13]

This CL is incorrect.

detachLayerForOverflowControls is called by GraphicsLayerTreeBuilder::rebuild,
which won't always get called when the values this function now reads are
modified.

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/397713004/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1794: // inaccurate
results (and trips the ASSERTS in RenderLayerClipper).
We can delete the other instance of the comment if you like.

The difference is that the other call site is executed many, many times per
frame on a typical page.

[Ian Vollick, 2014-07-16 06:45:40]

On 2014/07/16 05:28:13, abarth wrote:
> This CL is incorrect.
> 
> detachLayerForOverflowControls is called by GraphicsLayerTreeBuilder::rebuild,
> which won't always get called when the values this function now reads are
> modified.

Oof, thank you, that's a great point. The clipping-related properties of the
overflow host layer will, I think, need to be modified whenever the ancestor
clipping layer's properties are updated. I'll post a patch that does this
tomorrow.

[Ian Vollick, 2014-07-23 20:55:25]

On 2014/07/16 06:45:40, Ian Vollick wrote:
> On 2014/07/16 05:28:13, abarth wrote:
> > This CL is incorrect.
> > 
> > detachLayerForOverflowControls is called by
GraphicsLayerTreeBuilder::rebuild,
> > which won't always get called when the values this function now reads are
> > modified.
> 
> Oof, thank you, that's a great point. The clipping-related properties of the
> overflow host layer will, I think, need to be modified whenever the ancestor
> clipping layer's properties are updated. I'll post a patch that does this
> tomorrow.

In this latest cl, the geometric properties that affect the overflow controls
position and clip are updated with the other CLM layers (in particular, whenever
the ancestor clipping layer's geometry changes).

With that change, detachLayerForOverflowControls the layers (which is done in
GraphicsLayerTreeBuilder::rebuild) has no geometric side effects which, as Adam
pointed out, would not have been updated often enough.

I've added layout tests for clipped, reparented overlay scrollbars (both
reflected
and not reflected) as well as unclipped scrollbars with an offset from renderer.

It took some doing, I uncovered a number of blocking bugs as I put the
layout tests together, but they oughta pass now (fingers crossed).
 - https://codereview.chromium.org/409193002/
 - https://codereview.chromium.org/402283003/

[hartmanng, 2014-07-24 15:57:36]

https://codereview.chromium.org/397713004/diff/200001/LayoutTests/compositing...
File
LayoutTests/compositing/overflow/reflected-overlay-scrollbars-should-respect-ancestor-clip.html
(right):

https://codereview.chromium.org/397713004/diff/200001/LayoutTests/compositing...
LayoutTests/compositing/overflow/reflected-overlay-scrollbars-should-respect-ancestor-clip.html:8:
overflow: hidden;
nit: this indentation doesn't match the rest of the file. Same goes for #clipper
in the -expected.html

https://codereview.chromium.org/397713004/diff/200001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/397713004/diff/200001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:750: static
FloatPoint relativePositionInTransformedAncestorSpace(const RenderLayer* layer,
const RenderLayer* compositingStackingContext)
I find this name a bit confusing. When I read it, I assume this function will
basically be returning the value you call 'localOffsetToTransformedAncestor'.
But maybe that's just me?

My best suggestion is something like
offsetFromCompositingStackingContextInTransformedAncestorSpace, but that's
getting pretty verbose.

https://codereview.chromium.org/397713004/diff/200001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:850:
reflectionCompositedLayerMapping->updateGraphicsLayerGeometry(&m_owningLayer,
&m_owningLayer, layersNeedingPaintInvalidation);
Why is m_owningLayer necessarily the correct compositingStackingContext in this
case?

[Ian Vollick, 2014-07-24 17:39:09]

https://codereview.chromium.org/397713004/diff/200001/LayoutTests/compositing...
File
LayoutTests/compositing/overflow/reflected-overlay-scrollbars-should-respect-ancestor-clip.html
(right):

https://codereview.chromium.org/397713004/diff/200001/LayoutTests/compositing...
LayoutTests/compositing/overflow/reflected-overlay-scrollbars-should-respect-ancestor-clip.html:8:
overflow: hidden;
On 2014/07/24 15:57:35, hartmanng wrote:
> nit: this indentation doesn't match the rest of the file. Same goes for
#clipper
> in the -expected.html

Done. I've cleaned up the indentation in all the layout test files.

https://codereview.chromium.org/397713004/diff/200001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/397713004/diff/200001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:750: static
FloatPoint relativePositionInTransformedAncestorSpace(const RenderLayer* layer,
const RenderLayer* compositingStackingContext)
On 2014/07/24 15:57:35, hartmanng wrote:
> I find this name a bit confusing. When I read it, I assume this function will
> basically be returning the value you call 'localOffsetToTransformedAncestor'.
> But maybe that's just me?
> 
> My best suggestion is something like
> offsetFromCompositingStackingContextInTransformedAncestorSpace, but that's
> getting pretty verbose.

Yeah, the name's crappy. I've sidestepped the issue by inlining this function.
It's not used anywhere else.

https://codereview.chromium.org/397713004/diff/200001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:850:
reflectionCompositedLayerMapping->updateGraphicsLayerGeometry(&m_owningLayer,
&m_owningLayer, layersNeedingPaintInvalidation);
On 2014/07/24 15:57:35, hartmanng wrote:
> Why is m_owningLayer necessarily the correct compositingStackingContext in
this
> case?

It's a great question. Having a reflection makes you a stacking context. (See
https://www.webkit.org/blog/182/css-reflections/).

The reflected* layout tests is to cover this case.

[hartmanng, 2014-07-24 18:28:45]

lgtm, but please wait for Adam to sign off on it

[chrishtr, 2014-07-24 18:44:57]

https://codereview.chromium.org/397713004/diff/240001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.h (right):

https://codereview.chromium.org/397713004/diff/240001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.h:399: // can remain
ignorant of the layers above them and still work correctly.
Starting with a basic question..

Can you give me a concrete example? In particular, I don't yet understand why
the controls would be clipped by something other than the thing which clips
m_graphicsLayer content. Also please diagram
the layers for this situation, similar to the comment on lines 404-408.

[Ian Vollick, 2014-07-24 19:09:45]

On 2014/07/24 18:44:57, chrishtr wrote:
>
https://codereview.chromium.org/397713004/diff/240001/Source/core/rendering/c...
> File Source/core/rendering/compositing/CompositedLayerMapping.h (right):
> 
>
https://codereview.chromium.org/397713004/diff/240001/Source/core/rendering/c...
> Source/core/rendering/compositing/CompositedLayerMapping.h:399: // can remain
> ignorant of the layers above them and still work correctly.
> Starting with a basic question..
> 
> Can you give me a concrete example? In particular, I don't yet understand why
> the controls would be clipped by something other than the thing which clips
> m_graphicsLayer content. Also please diagram
> the layers for this situation, similar to the comment on lines 404-408.

I've added a detailed explanation of the scenario that the layout tests in this
CL cover to the patch description. I've also updated the diagrams to show where
the overflow controls are placed.

[chrishtr, 2014-07-25 18:00:34]

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.h (right):

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.h:327: //        +
m_overflowControlsClippingLayer [OPTIONAL] // *The overflow controls may need to
be repositioned in the
Is it positioned here, or is it a sibling of m_graphicsLayer? From reading the
CL description, I thought the whole reason for this CL was that
this layer can't be positioned below m_graphicsLayer.

Also: I can see how m_overflowControlsHostLayer can't be below m_graphicsLayer
because it needs to paint after everything, but why can't it
be a sibling of m_graphicsLayer? And if it is a sibling, won't
m_ancestorClippingLayer clip it correctly?

My understanding is that the ancestor clipper should clip both the contents of
the overflow-scroll layer and its scroll bars, so from the
point of view of what the ancestor should clip, scroll bars are not special.

[chrishtr, 2014-07-25 18:00:46]

BTW thank you for the very clear CL description.

[Ian Vollick, 2014-07-25 18:19:51]

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.h (right):

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.h:327: //        +
m_overflowControlsClippingLayer [OPTIONAL] // *The overflow controls may need to
be repositioned in the
On 2014/07/25 18:00:33, chrishtr wrote:
> Is it positioned here, or is it a sibling of m_graphicsLayer? From reading the
> CL description, I thought the whole reason for this CL was that
> this layer can't be positioned below m_graphicsLayer.
> 
> Also: I can see how m_overflowControlsHostLayer can't be below m_graphicsLayer
> because it needs to paint after everything, but why can't it
> be a sibling of m_graphicsLayer? And if it is a sibling, won't
> m_ancestorClippingLayer clip it correctly?
> 
> My understanding is that the ancestor clipper should clip both the contents of
> the overflow-scroll layer and its scroll bars, so from the
> point of view of what the ancestor should clip, scroll bars are not special.

If the overflow controls need to be reparented (and this CLM has an ancestor
clipping layer), they will not be a sibling of m_graphicsLayer, they will be a
sibling of m_ancestorClippingLayer.

Some more detail:

If the controls need to be reparented, that implies that the scroller does not
form a stacking context and that it has some scrolling content that succeeds it
in paint order. This scrolling content will have its own CLM which will be a
sibling of this CLM. In particular, this scrolling content will _not_ be below
the ancestor clipping (scroll parent magic will ensure that it gets clipped
correctly in the compositor). The reparented overflow controls will need to
stack above this scrolling content, so it will succeed the last bit of scrolling
content's CLM. Here's a diagram of the final arrangement after reparenting.

+ stacking context
  + ancestor-clipping-layer (of scroller)
     + graphics-layer (of scroller)
        + ...
  + graphics-layer (of scrolled content that stacks above the scroller)
  + overflow-control-clip-layer
     + overflow-control-host-layer

The overflow controls aren't below the ancestor clipping layer, so we can't rely
on it for clipping.

[chrishtr, 2014-07-25 21:04:45]

lgtm

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1014:
m_overflowControlsClippingLayer->addChild(m_overflowControlsHostLayer.get());
ASSERT(m_overflowControlsHostLayer)

[chrishtr, 2014-07-25 21:06:16]

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:556: bool needsToReparentOverflowControls()
const;
On reflection, this is weird to put on RenderLayer, since it's specific to
graphics layer hierarchies.
Instead let's put it on RenderLayerCompositor?

Also please add a comment as this concept is confusing.

[Ian Vollick, 2014-07-26 01:54:58]

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:556: bool needsToReparentOverflowControls()
const;
On 2014/07/25 21:06:15, chrishtr wrote:
> On reflection, this is weird to put on RenderLayer, since it's specific to
> graphics layer hierarchies.
> Instead let's put it on RenderLayerCompositor?
> 
> Also please add a comment as this concept is confusing.

Yeah, now that you mention it, I agree that RenderLayer isn't a good home for
this fn. I thought that it fit well in CompositedLayerMapping, though, so I've
stuck it there and added an explanatory comment. Please let me know if you think
that this still doesn't make sense in the CLM.

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/397713004/diff/280001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1014:
m_overflowControlsClippingLayer->addChild(m_overflowControlsHostLayer.get());
On 2014/07/25 21:04:44, chrishtr wrote:
> ASSERT(m_overflowControlsHostLayer)

Done.

[Ian Vollick, 2014-07-26 01:58:02]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-07-26 01:58:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vollick@chromium.org/397713004/300001

[commit-bot: I haz the power, 2014-07-26 02:58:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19049)

[commit-bot: I haz the power, 2014-07-26 05:14:21]

Change committed as 178980