[BlinkGenPropertyTrees] virtual/android/rootscroller/nested-rootscroller-browser-controls-bounds-hidden.html is failing |
|||||
Issue descriptionThis 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?
,
Aug 31
Will look into it next week - looking into a scroll-snap failure with BGPT today
,
Aug 31
We discussed this in the meeting today and I'm going to add this to my queue.
,
Sep 6
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?
,
Sep 7
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.
,
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
,
Sep 14
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by pdr@chromium.org
, Aug 31