Expand PaintLayer clip to account for hidden URL bar with document.rootScroller

Expand PaintLayer clip to account for hidden URL bar with document.rootScroller

In previous patches, the document.rootScroller would disable clipping on all
parent layers in the compositor. This was fine in terms of visibility; however,
hit testing is done on PaintLayers which come before compositor layers in the
lifecycle. This means hit testing would still be blocked by parent layers, even
if the area is visible.

This is particularily problematic in terms of the top controls expansion. In
order for a root scroller deeper in the frame hierarchy to get hit tested in
the area exposed by the hidden URL bar, we need to use the root frame's visible
content area when recursing down the PaintLayer tree for hit testing.

This patch makes a few changes:

-For hit testing entry, PaintLayers that are ancestors of the global
RootScroller will use the root FrameView's size for clipping.  In order for
this to work, local LayoutBox hit testing needs to use the top LayoutView's
overflowClipRect. There's an exception for SVG viewports since those appear to
need clipping based on the SVG root rather than the viewport.  I've also tried
to consolidate this logic inside TopDocumentRootScrollerController and reuse it
across call-sites.

-Since we're expanding the clipping on PaintLayers, we no longer need to do the
URL bar adjustment for root scrollers in CompositedLayerMapping.

-PaintLayerClipper uses treats the local root scroller as the clipping root.

-Had to fix BlockBoundTest in WebFrameTest as it didn't have a WebView size so
it's FrameView had (0, 0) size which was now causing a failed hit test.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[bokan, 2017-01-11 16:12:41]

Description was changed from

==========
Expand PaintLayer clip to account for hidden URL bar with document.rootScroller

BUG=505516
==========

to

==========
Expand PaintLayer clip to account for hidden URL bar with document.rootScroller

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2017-01-11 18:20:28]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 18:20:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 19:03:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:03:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bokan, 2017-01-11 19:24:19]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 19:24:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 19:41:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:41:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bokan, 2017-01-16 12:40:18]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 12:40:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 13:13:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 13:13:05]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[bokan, 2017-01-16 17:42:24]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 17:42:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 18:40:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 18:40:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bokan, 2017-01-17 18:04:44]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 18:05:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 18:07:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 18:07:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[bokan, 2017-01-17 18:23:58]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 18:24:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-01-17 18:44:28]

Description was changed from

==========
Expand PaintLayer clip to account for hidden URL bar with document.rootScroller

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Expand PaintLayer clip to account for hidden URL bar with document.rootScroller

In previous patches, the document.rootScroller would disable clipping on all
parent layers in the compositor. This was fine in terms of visibility; however,
hit testing is done on PaintLayers which come before compositor layers in the
lifecycle. This means hit testing would still be blocked by parent layers, even
if the area is visible.

This is particularily problematic in terms of the top controls expansion. In
order for a root scroller deeper in the frame hierarchy to get hit tested in
the area exposed by the hidden URL bar, we need to use the root frame's visible
content area when recursing down the PaintLayer tree for hit testing.

This patch makes a few changes:

-For hit testing, PaintLayers that are ancestors of the global RootScroller
will use the root FrameView for clipping.  In order for this to work, a
LayoutBox that's the root scroller also needs to use the top LayoutView's
overflowClipRect. There's an exception for SVG viewports since those appear to
need clipping based on the SVG root rather than the viewport. I've also tried
to consolidate this logic inside TopDocumentRootScrollerController and reuse
it across call-sites.

-Since we're expanding the clipping on PaintLayers, we no longer need to do the
URL bar adjustment for root scrollers in CompositedLayerMapping.

-PaintLayerClipper uses treats the local root scroller as the clipping root.

-Had to fix BlockBoundTest in WebFrameTest as it didn't have a WebView size so
it's FrameView had (0, 0) size which was now causing a failed hit test.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2017-01-17 18:56:46]

Description was changed from

==========
Expand PaintLayer clip to account for hidden URL bar with document.rootScroller

In previous patches, the document.rootScroller would disable clipping on all
parent layers in the compositor. This was fine in terms of visibility; however,
hit testing is done on PaintLayers which come before compositor layers in the
lifecycle. This means hit testing would still be blocked by parent layers, even
if the area is visible.

This is particularily problematic in terms of the top controls expansion. In
order for a root scroller deeper in the frame hierarchy to get hit tested in
the area exposed by the hidden URL bar, we need to use the root frame's visible
content area when recursing down the PaintLayer tree for hit testing.

This patch makes a few changes:

-For hit testing, PaintLayers that are ancestors of the global RootScroller
will use the root FrameView for clipping.  In order for this to work, a
LayoutBox that's the root scroller also needs to use the top LayoutView's
overflowClipRect. There's an exception for SVG viewports since those appear to
need clipping based on the SVG root rather than the viewport. I've also tried
to consolidate this logic inside TopDocumentRootScrollerController and reuse
it across call-sites.

-Since we're expanding the clipping on PaintLayers, we no longer need to do the
URL bar adjustment for root scrollers in CompositedLayerMapping.

-PaintLayerClipper uses treats the local root scroller as the clipping root.

-Had to fix BlockBoundTest in WebFrameTest as it didn't have a WebView size so
it's FrameView had (0, 0) size which was now causing a failed hit test.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Expand PaintLayer clip to account for hidden URL bar with document.rootScroller

In previous patches, the document.rootScroller would disable clipping on all
parent layers in the compositor. This was fine in terms of visibility; however,
hit testing is done on PaintLayers which come before compositor layers in the
lifecycle. This means hit testing would still be blocked by parent layers, even
if the area is visible.

This is particularily problematic in terms of the top controls expansion. In
order for a root scroller deeper in the frame hierarchy to get hit tested in
the area exposed by the hidden URL bar, we need to use the root frame's visible
content area when recursing down the PaintLayer tree for hit testing.

This patch makes a few changes:

-For hit testing entry, PaintLayers that are ancestors of the global
RootScroller will use the root FrameView's size for clipping.  In order for
this to work, local LayoutBox hit testing needs to use the top LayoutView's
overflowClipRect. There's an exception for SVG viewports since those appear to
need clipping based on the SVG root rather than the viewport.  I've also tried
to consolidate this logic inside TopDocumentRootScrollerController and reuse it
across call-sites.

-Since we're expanding the clipping on PaintLayers, we no longer need to do the
URL bar adjustment for root scrollers in CompositedLayerMapping.

-PaintLayerClipper uses treats the local root scroller as the clipping root.

-Had to fix BlockBoundTest in WebFrameTest as it didn't have a WebView size so
it's FrameView had (0, 0) size which was now causing a failed hit test.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2017-01-17 18:57:29]

bokan@chromium.org changed reviewers:
+ chrishtr@chromium.org

[bokan, 2017-01-17 19:03:02]

Hey Chris,

In a couple of the previous patches
(https://codereview.chromium.org/2289833002/,
https://codereview.chromium.org/2499853002/) I made the root scroller disable
clipping on compositing layers that are ancestors of the root scroller. This
works fine for visibility and paint, but breaks hit testing since that's done on
the PaintLayers themselves.

This patch fixes the issues I've found but I'm new to PaintLayerClipper and
friend so I don't know the concepts well. Please add whoever is the expert there
if it's not you.

Thanks,
David

[commit-bot: I haz the power, 2017-01-17 20:00:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 20:00:44]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-01-18 23:23:09]

How about just starting all hit tests at the root scroller? Anything above it
can't be hit,
right? Would that avoid the complexity in this CL?

[bokan, 2017-01-18 23:46:58]

On 2017/01/18 23:23:09, chrishtr wrote:
> How about just starting all hit tests at the root scroller? Anything above it
> can't be hit,
> right? Would that avoid the complexity in this CL?

Not quite. One of the nice properties of the proposal is that you can have
scrollers that are siblings of the rootScroller so that solves the issue of
unwanted scroll chaining (you have a model popup that scrolls but don't want it
to chain up to the main page). See http://bokand.github.io/totese.html and click
one of the show popup buttons.

[chrishtr, 2017-01-19 01:43:23]

On 2017/01/18 at 23:46:58, bokan wrote:
> On 2017/01/18 23:23:09, chrishtr wrote:
> > How about just starting all hit tests at the root scroller? Anything above
it
> > can't be hit,
> > right? Would that avoid the complexity in this CL?
> 
> Not quite. One of the nice properties of the proposal is that you can have
scrollers that are siblings of the rootScroller so that solves the issue of
unwanted scroll chaining (you have a model popup that scrolls but don't want it
to chain up to the main page). See http://bokand.github.io/totese.html and click
one of the show popup buttons.

I don't quite understand what that would have to do with my suggestion though. I
must be 
missing something. When the popup is open, it becomes the root scroller right?
In which
case it must cover the entire screen, and therefore nothing else can receive a
hit test.
This must be incorrect logic, I wrote above, but I don't see why yet...

[bokan, 2017-01-19 03:18:03]

On 2017/01/19 01:43:23, chrishtr wrote:
> On 2017/01/18 at 23:46:58, bokan wrote:
> > On 2017/01/18 23:23:09, chrishtr wrote:
> > > How about just starting all hit tests at the root scroller? Anything above
> it
> > > can't be hit,
> > > right? Would that avoid the complexity in this CL?
> > 
> > Not quite. One of the nice properties of the proposal is that you can have
> scrollers that are siblings of the rootScroller so that solves the issue of
> unwanted scroll chaining (you have a model popup that scrolls but don't want
it
> to chain up to the main page). See http://bokand.github.io/totese.html and
click
> one of the show popup buttons.
> 
> I don't quite understand what that would have to do with my suggestion though.
I
> must be 
> missing something. When the popup is open, it becomes the root scroller right?
> In which
> case it must cover the entire screen, and therefore nothing else can receive a
> hit test.
> This must be incorrect logic, I wrote above, but I don't see why yet...

The popup isn't the root scroller. It doesn't fill the screen and we don't want
it to hide the top controls. The main content remains the rootScroller. The DOM
looks like this:

<div id="popup" style="position: fixed; ...">...</div>
<div id="rootScroller" style="width:100%; height:100%"></div>

They're siblings, the rootScroller div remains the root scroller throughout. The
advantage here is that previously the popup div had to be a child of the <body>
element. This means that authors have to use a load of non-interoperable hacks
to prevent scrolls on the popup from chaining up to the <body>. In this
arrangement, the author doesn't have to do anything, it works out naturally.

This is a nice property and something I'd like to keep (though admittedly, the
scroll chaining issue is being worked on with separate APIs so maybe becoming
less valuable).

[chrishtr, 2017-01-21 02:06:47]

I still feel it's strange to have content outside of the rootScroller. The
example
of a fixed-position overlay outside of the scroller also sounds strange, because
it
means there are some magic-like effects outside of the rootScroller that
everyone
has to remember all the time.

If we only had to be concerned with things in the rootScroller and below it in
the DOM
etc, it would fit in much better with the way the paint, hit testing and
compositing
system make assumptions. e.g. SPv1 composited layers are perhaps a good analogy
to
rootScroller - they are independent things that don't depend on any layout/DOM
state
from things above them. Such effects are applied by composited layers above them
in the
graphics layer tree.

For the situation of a modal dialog or fading between views, maybe it's
sufficient
to remove the rootScroller property during such conditions. e.g. the app could
turn off
rootScroller, show the popup, and also put a hit testing region over the entire
screen
so that the rootScroller can't be scrolled (which is probably desireable UX).
Similarly for fading?

Or have a bunch of rootScrollers and rotate the active one, to maximize
performance/
encapsulation? Anyway that is getting into bikeshedding the spec; the concern I
have here
is about implementation complexity and performance.

https://codereview.chromium.org/2623213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2623213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1806: bool
applySVGViewportClip =
overflowClipRect is a virtual method. You can put this in
LayoutSVGRoot.

https://codereview.chromium.org/2623213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1808: if
(!applySVGViewportClip && node() && node() == globalRootScroller) {
Is it possible for the rootScroller to not have a PaintLayer? Doesn't
it have to be positioned or have overflow scroll?

https://codereview.chromium.org/2623213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1812: if
(!topView->viewRect().isEmpty()) {
How about just adjusting overflowClipRect() for LayoutBox to know
how to do this instead? And possibly remove the code from LayoutView,
since with root-layer scrolling on the view acts more like a box.

https://codereview.chromium.org/2623213002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(left):

https://codereview.chromium.org/2623213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1315:
layoutBox->document().layoutView()->overflowClipRect(LayoutPoint());
Why can this now be removed?

https://codereview.chromium.org/2623213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2623213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:1727: if
(host->globalRootScrollerController().isRootScrollerAncestor(doc)) {
This adjustment in hit-testing code doesn't also appear in in LayoutBox.
Why not?

[bokan, 2017-01-24 20:38:54]

On 2017/01/21 02:06:47, chrishtr wrote:
> I still feel it's strange to have content outside of the rootScroller. The
> example
> of a fixed-position overlay outside of the scroller also sounds strange,
because
> it
> means there are some magic-like effects outside of the rootScroller that
> everyone
> has to remember all the time.
> 
> If we only had to be concerned with things in the rootScroller and below it in
> the DOM
> etc, it would fit in much better with the way the paint, hit testing and
> compositing
> system make assumptions. e.g. SPv1 composited layers are perhaps a good
analogy
> to
> rootScroller - they are independent things that don't depend on any layout/DOM
> state
> from things above them. Such effects are applied by composited layers above
them
> in the
> graphics layer tree.
> 
> For the situation of a modal dialog or fading between views, maybe it's
> sufficient
> to remove the rootScroller property during such conditions. e.g. the app could
> turn off
> rootScroller, show the popup, and also put a hit testing region over the
entire
> screen
> so that the rootScroller can't be scrolled (which is probably desireable UX).
> Similarly for fading?
> 
> Or have a bunch of rootScrollers and rotate the active one, to maximize
> performance/
> encapsulation? Anyway that is getting into bikeshedding the spec; the concern
I
> have here
> is about implementation complexity and performance.

Thanks for the review, points taken. I'll see how far I can get with making the
root scroller the paint root, or at least preventing "magical" effects outside
its containing layer.