Fix bottom/right fixed-pos with non-overlay scrollbars.

Fix bottom/right fixed-pos with non-overlay scrollbars.

Since Blink positions fixed-pos elements against the clip-rect,
adjusting them against the true viewport size (which includes the
non-overlay scrollbars) was incorrect.  Moreover, scrollbar layer size
isn't correct here because custom scrollbars and non-impl-scrolled
scrollbars don't have them attached to the root scroll layer (and this
likely is causing bugs in the max scroll offset calculation that haven't
been noticed yet).

The cleanest way to deal with this is to get additional information from
Blink about what it believes the viewport size is including the
non-overlay scrollbars (see https://codereview.chromium.org/21084007 ).
This is passed in via the bounds of a layer I call the ViewportLayer.
CC determines it by looping up the ancestors of RootContainerLayer, in
order for the logic to work regardless of additional intermediate layers
that Blink might later add (page scale layer, etc).

BUG=263172

[aelias_OOO_until_Jul13, 2013-07-29 21:42:14]

Hi Enne, let me know what you think of this approach.  If it looks good, I'll
write some tests.

[enne (OOO), 2013-07-30 02:32:17]

+1 to setting bounds of layers in Blink to convey more information to the
compositor.

However, I'm a little confused on some details here...

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc#n...
cc/trees/layer_tree_impl.cc:252: max_scroll.set_x(max_scroll.x() +
NonOverlayScrollbarThickness(VERTICAL));
This code seems like it'd be cleaner if it set
RenderLayerCompositor::m_scrollLayer's size to the right size rather than making
assumptions from other layers.

The advantage of that is for me is that we could stop finding the first child of
the scrollable layer to guess its bounds, and then we could avoid workarounds
like https://codereview.chromium.org/14221010/.

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc#n...
cc/trees/layer_tree_impl.cc:570: gfx::ScaleSize(viewport_layer->bounds(), 1 /
page_scale_factor());
Just to make sure I'm reading this correctly, ViewportLayer() here is intending
to return the overflow controls host, which includes the non-overlay scrollbars
in its bounds?

It seems to me like you would want to have a bottom fixed pos element be flush
with the viewport *not* containing the non-overlay scrollbars, which is not what
I see this code doing.

Can you help me understand what's going on here?

[aelias_OOO_until_Jul13, 2013-07-30 02:52:00]

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc#n...
cc/trees/layer_tree_impl.cc:252: max_scroll.set_x(max_scroll.x() +
NonOverlayScrollbarThickness(VERTICAL));
On 2013/07/30 02:32:17, enne wrote:
> This code seems like it'd be cleaner if it set
> RenderLayerCompositor::m_scrollLayer's size to the right size rather than
making
> assumptions from other layers.
> 
> The advantage of that is for me is that we could stop finding the first child
of
> the scrollable layer to guess its bounds, and then we could avoid workarounds
> like https://codereview.chromium.org/14221010/.

OK, I'll try that.  It will be useful for root overflow:hidden support anyway. 
Note though that this will include the scrollbar size in ScrollableSize and you
objected to those semantics last time (though I think it's fine).

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc#n...
cc/trees/layer_tree_impl.cc:570: gfx::ScaleSize(viewport_layer->bounds(), 1 /
page_scale_factor());
On 2013/07/30 02:32:17, enne wrote:
> Just to make sure I'm reading this correctly, ViewportLayer() here is
intending
> to return the overflow controls host, which includes the non-overlay
scrollbars
> in its bounds?
> 
> It seems to me like you would want to have a bottom fixed pos element be flush
> with the viewport *not* containing the non-overlay scrollbars, which is not
what
> I see this code doing.
> 
> Can you help me understand what's going on here?

It doesn't matter because it's a delta.  This is comparing two viewports that
include the non-overlay scrollbar size.  Equivalently, I could instead compare
two viewports that exclude the non-overlay scrollbars (which would involve
explicitly subtracting the scrollbar size from true_viewport_size).  I did it
this way because it's less lines of code.

[enne (OOO), 2013-07-30 03:44:46]

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc#n...
cc/trees/layer_tree_impl.cc:252: max_scroll.set_x(max_scroll.x() +
NonOverlayScrollbarThickness(VERTICAL));
On 2013/07/30 02:52:00, aelias wrote:
> On 2013/07/30 02:32:17, enne wrote:
> > This code seems like it'd be cleaner if it set
> > RenderLayerCompositor::m_scrollLayer's size to the right size rather than
> making
> > assumptions from other layers.
> > 
> > The advantage of that is for me is that we could stop finding the first
child
> of
> > the scrollable layer to guess its bounds, and then we could avoid
workarounds
> > like https://codereview.chromium.org/14221010/.
> 
> OK, I'll try that.  It will be useful for root overflow:hidden support anyway.

> Note though that this will include the scrollbar size in ScrollableSize and
you
> objected to those semantics last time (though I think it's fine).

Oh, I see what you're getting at.  What would make sense to me would be if
ScrollableSize were the size of the scrolling contents and
ScrollableViewportSize were the size of the viewport where content can appear
(and is shrunk for non-overlay scrollbars, as they logically clip the view).

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc#n...
cc/trees/layer_tree_impl.cc:570: gfx::ScaleSize(viewport_layer->bounds(), 1 /
page_scale_factor());
On 2013/07/30 02:52:00, aelias wrote:
> On 2013/07/30 02:32:17, enne wrote:
> > Just to make sure I'm reading this correctly, ViewportLayer() here is
> intending
> > to return the overflow controls host, which includes the non-overlay
> scrollbars
> > in its bounds?
> > 
> > It seems to me like you would want to have a bottom fixed pos element be
flush
> > with the viewport *not* containing the non-overlay scrollbars, which is not
> what
> > I see this code doing.
> > 
> > Can you help me understand what's going on here?
> 
> It doesn't matter because it's a delta.  This is comparing two viewports that
> include the non-overlay scrollbar size.  Equivalently, I could instead compare
> two viewports that exclude the non-overlay scrollbars (which would involve
> explicitly subtracting the scrollbar size from true_viewport_size).  I did it
> this way because it's less lines of code.

I worked through an example, and I'll buy that explanation.  Thanks.

[aelias_OOO_until_Jul13, 2013-07-30 04:06:13]

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc#n...
cc/trees/layer_tree_impl.cc:252: max_scroll.set_x(max_scroll.x() +
NonOverlayScrollbarThickness(VERTICAL));
On 2013/07/30 03:44:47, enne wrote:
> Oh, I see what you're getting at.  What would make sense to me would be if
> ScrollableSize were the size of the scrolling contents and
> ScrollableViewportSize were the size of the viewport where content can appear
> (and is shrunk for non-overlay scrollbars, as they logically clip the view).

That actually is not correct.  In your page scale doc, non-overlay scrollbars
lie below the page scale layer.  They are in the same space as the contents but
not the inner viewport.  (Put another way, because the scrollbars scale up, they
take up a variable amount of the inner viewport that isn't known in advance.)

[enne (OOO), 2013-07-30 19:14:54]

On 2013/07/30 04:06:13, aelias wrote:
> https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc
> File cc/trees/layer_tree_impl.cc (right):
> 
>
https://codereview.chromium.org/21091004/diff/1/cc/trees/layer_tree_impl.cc#n...
> cc/trees/layer_tree_impl.cc:252: max_scroll.set_x(max_scroll.x() +
> NonOverlayScrollbarThickness(VERTICAL));
> On 2013/07/30 03:44:47, enne wrote:
> > Oh, I see what you're getting at.  What would make sense to me would be if
> > ScrollableSize were the size of the scrolling contents and
> > ScrollableViewportSize were the size of the viewport where content can
appear
> > (and is shrunk for non-overlay scrollbars, as they logically clip the view).
> 
> That actually is not correct.  In your page scale doc, non-overlay scrollbars
> lie below the page scale layer.  They are in the same space as the contents
but
> not the inner viewport.  (Put another way, because the scrollbars scale up,
they
> take up a variable amount of the inner viewport that isn't known in advance.)

That's a really good point.  I think I just don't like adding more implicit
interlayer relationships, such as scrollbars taking up space and shrinking some
other layer in the tree that doesn't explicitly know about those scrollbars.

Here's some thoughts:

With the page scale layers, there are two scroll layers.  The inner viewport
clip layer's size has to be large enough to include the non-overlay scrollbars
(otherwise they'd be clipped).  The outer viewport clip layer's size does not
include the non-overlay scrollbars (so that content doesn't draw underneath
them).  The max scroll offset of the outer viewport scroll layer is the outer
viewport scroll layer's bounds minus the outer viewport clip layer's bounds
(since they are in the same space).  The max scroll offset of the inner viewport
scroll layer is (inner viewport clip layer bounds / page scale) - outer viewport
clip layer bounds.

Without the page scale layers, if there are non-overlay scrollbars then the clip
layer will be sized to not contain them, and the max scroll offset of the root
scroll layer should be the the clip layer's bounds / page scale.

[aelias_OOO_until_Jul13, 2013-07-30 20:27:12]

Good point about the future two-scroll-layer world.

But, the other factor you're not considering is the dynamism of Android
viewports.  We can't use the inner clip layer for maxscrolloffset there, we need
to use the real device viewport.  In https://codereview.chromium.org/18202006/ I
tried to reduce platform divergence by making desktop use the true viewport as
well -- but this requires explicit knowledge of non-overlay scrollbar size. 
Given that, I think what I uploaded in Patchset 1 is still the best solution for
now, unless you want to bring back an if (Settings().solid_color_scrollbars).

[enne (OOO), 2013-07-30 20:40:21]

On 2013/07/30 20:27:12, aelias wrote:
> Good point about the future two-scroll-layer world.
> 
> But, the other factor you're not considering is the dynamism of Android
> viewports.  We can't use the inner clip layer for maxscrolloffset there, we
need
> to use the real device viewport.  In https://codereview.chromium.org/18202006/
I
> tried to reduce platform divergence by making desktop use the true viewport as
> well -- but this requires explicit knowledge of non-overlay scrollbar size. 
> Given that, I think what I uploaded in Patchset 1 is still the best solution
for
> now, unless you want to bring back an if (Settings().solid_color_scrollbars).

Dynamism meaning the top controls manager reducing the visible portion of the
inner clip layer while not reducing its bounds, right? Could you intersect the
bounds of the inner viewport layer with the VisibleViewportSize?

[aelias_OOO_until_Jul13, 2013-07-30 20:51:40]

On 2013/07/30 20:40:21, enne wrote:
> On 2013/07/30 20:27:12, aelias wrote:
> > Good point about the future two-scroll-layer world.
> > 
> > But, the other factor you're not considering is the dynamism of Android
> > viewports.  We can't use the inner clip layer for maxscrolloffset there, we
> need
> > to use the real device viewport.  In
https://codereview.chromium.org/18202006/
> I
> > tried to reduce platform divergence by making desktop use the true viewport
as
> > well -- but this requires explicit knowledge of non-overlay scrollbar size. 
> > Given that, I think what I uploaded in Patchset 1 is still the best solution
> for
> > now, unless you want to bring back an if
(Settings().solid_color_scrollbars).
> 
> Dynamism meaning the top controls manager reducing the visible portion of the
> inner clip layer while not reducing its bounds, right? Could you intersect the
> bounds of the inner viewport layer with the VisibleViewportSize?

Unfortunately, the top controls manager can also increase the size of the
visible viewport to be larger than the clip layer size.  (We had considered a
design where the clip layer was always the maximum size, but it raises problems
with hit testing on fixed-pos elements, among other things.)

[enne (OOO), 2013-07-31 00:59:09]

On 2013/07/30 20:51:40, aelias wrote:

> Unfortunately, the top controls manager can also increase the size of the
> visible viewport to be larger than the clip layer size.  (We had considered a
> design where the clip layer was always the maximum size, but it raises
problems
> with hit testing on fixed-pos elements, among other things.)

Oh, right.  Sorry for forgetting the details here.

I think your code as written works, but it just doesn't make much sense to me
because I don't think we can really make the platforms unified here.  If you had
a viewport that was larger than the size of the clip layer + non-overlay
scrollbars, then you'd have floating scrollbars just based on the way they're
set up.  Theoretically we could make them fixed pos attached to the bottom, but
we don't support stretching on the compositor for non-overlay scrollbars so they
wouldn't be the right size.  So, it really seems like we have either (1) fixed
size viewport + non-overlay scrollbars or (2) arbitrary viewport + only overlay
scrollbars.  There's not any other supported combination.

It just seems to me that the expression (LTHI viewport - non-overlay scrollbar
size) only makes sense in both worlds because non-overlay size is non-zero only
if (LTHI viewport == clip layer size + non-overlay scrollbar size).

[aelias_OOO_until_Jul13, 2013-07-31 01:33:06]

On 2013/07/31 00:59:09, enne wrote:
> On 2013/07/30 20:51:40, aelias wrote:
> 
> > Unfortunately, the top controls manager can also increase the size of the
> > visible viewport to be larger than the clip layer size.  (We had considered
a
> > design where the clip layer was always the maximum size, but it raises
> problems
> > with hit testing on fixed-pos elements, among other things.)
> 
> Oh, right.  Sorry for forgetting the details here.
> 
> I think your code as written works, but it just doesn't make much sense to me
> because I don't think we can really make the platforms unified here.  If you
had
> a viewport that was larger than the size of the clip layer + non-overlay
> scrollbars, then you'd have floating scrollbars just based on the way they're
> set up.  Theoretically we could make them fixed pos attached to the bottom,
but
> we don't support stretching on the compositor for non-overlay scrollbars so
they
> wouldn't be the right size.  So, it really seems like we have either (1) fixed
> size viewport + non-overlay scrollbars or (2) arbitrary viewport + only
overlay
> scrollbars.  There's not any other supported combination.
> 
> It just seems to me that the expression (LTHI viewport - non-overlay scrollbar
> size) only makes sense in both worlds because non-overlay size is non-zero
only
> if (LTHI viewport == clip layer size + non-overlay scrollbar size).

OK then, do you want me to just revert https://codereview.chromium.org/18202006/
(modulo no longer checking that the clip layer actually masksToBounds)?

[aelias_OOO_until_Jul13, 2013-07-31 03:42:12]

Semi-revert up for your consideration at
https://codereview.chromium.org/21323002