New issue
Advanced search Search tips

Issue 879610 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 836890



Sign in to add a comment

[BlinkGenPropertyTrees] virtual/android/rootscroller/nested-rootscroller-browser-controls-bounds-hidden.html is failing

Project Member Reported by pdr@chromium.org, Aug 31

Issue description

This test is failing with --additional-driver-flag=--enable-blink-gen-property-trees:
virtual/android/rootscroller/nested-rootscroller-browser-controls-bounds-hidden.html

The failure looks real but it's hard to tell what's going wrong. Maybe the body's height is not affected by the browser controls?

David, could you look into this?
 
fast/dom/scroll-reveal-left-overflow.html and fast/dom/scroll-reveal-top-overflow.html may be related. This may just be a paint bug.
Status: Assigned (was: Untriaged)
Will look into it next week - looking into a scroll-snap failure with BGPT today
Components: -Blink>Scroll Blink>Paint
Owner: pdr@chromium.org
We discussed this in the meeting today and I'm going to add this to my queue.
Cc: bokan@chromium.org
My current thinking is that BGPT has some difference in scrolling that exposes an existing bug. If nested-rootscroller-browser-controls-bounds-hidden.html is modified so that #scroller has "border: 1px solid yellow;", the BGPT and !BGPT results match. This test has an iframe's child as the root scroller, and browser controls of height 100px with a shown ratio is 0. The visual viewport's scroll node has a height of 600-controls_height = 500 but is that correct if the shown ratio is 0?

@Bokan, should the visual viewport's property nodes be affected by by browser controls if the shown ratio is 0?
If you add a border to the scroller then it won't be a valid rootScroller so hiding the top controls will have different semantics. I'm not sure we should take that as indicative of the issue. 

Hiding the URL bar should add a viewport bounds delta on the inner and outer viewports. This will happen on the compositor side only and it will affect the scrolling bounds.

In this test, we start with the URL bar showing so the renderer should be 500px in height. Since we're hiding the URL bar (but don't yet cause a renderer resize), we should expect the bounds delta to add 100 to the viewport scroll node's container and contents rects (since the outer viewport is also adjusted). So I would actually expect both rects to be 600px tall.

Actually, I said  above it'll only happen on the compositor side but that's not completely true. We tell Blink about the controls shown ratio when we commit in ApplyViewportDeltas. This value is used in VisualViewport and PLSA to add a "top controls adjustment". This is basically a fudge factor to adjust the maximum scroll offset. I wonder if that's not well integrated with scrolls being stored in paint properties...

I'd have to take a closer look at the test tomorrow to get a better sense of what might be going wrong if that's not it.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 11

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

commit c3ddb307e451789dec98f8f6188cfc1f94031a1f
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Sep 11 21:52:45 2018

[BlinkGenPropertyTrees] Expand all clips between inner and outer viewports

The root scroller is also known as the outer viewport and is not always the
root frame's LayoutView. If the root scroller is a child of the root frame,
the clips between the child and the root frame should not apply to the root
scroller. Previously, this was implemented by disabling clipping on the
ancestor GraphicsLayers of the root scroller. This patch changes the behavior to
adjust all ancestor clips in LayerTreeHostImpl::UpdateViewportContainerSizes.
This should be safe because of the restrictions on what can be a root scroller.

Bug:  879610 
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: I1974ae6b2a2f130cc2ce4a4d90e3dea1c9d64340
Reviewed-on: https://chromium-review.googlesource.com/1217509
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590495}
[modify] https://crrev.com/c3ddb307e451789dec98f8f6188cfc1f94031a1f/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/c3ddb307e451789dec98f8f6188cfc1f94031a1f/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[modify] https://crrev.com/c3ddb307e451789dec98f8f6188cfc1f94031a1f/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.cc

Status: Fixed (was: Assigned)

Sign in to add a comment