New issue
Advanced search Search tips

Issue 884846 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 1
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 836890



Sign in to add a comment

[BGPT] virtual/threaded/fast/events/touch/gesture/gesture-scrollbar-mainframe.html fails

Project Member Reported by sahel@chromium.org, Sep 17

Issue description

virtual/threaded/fast/events/touch/gesture/gesture-scrollbar-mainframe.html fails with blink gen property trees enabled.


The test fails since this promise never gets resolved:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/events/touch/gesture/gesture-scrollbar-mainframe.html?type=cs&q=gesture-scrollbar-mainframe.html&g=0&l=47

Based on the comments in the test, it looks like a hit testing issue.
 
Logging the window.scrollY shows that the scroll ends up scrolling upward.
Components: Blink>Scroll
Sahel, do you know if this is a real bug or an issue with the test? i.e. should this block BGPT?
Looks like a real bug, but I have not checked to reproduce it locally yet:

The test first scrolls down a scrollable page to make sure scrolling in both up and down directions is possible. Then moves the finger upward on the scrollbar and checks to see if the page has scrolled down or not. With blink gen enabled the test fails since hittesting somehow passes through the scrollbar and the page scrolls up.
Blocking: 836890
Owner: chaopeng@chromium.org
Status: Assigned (was: Available)
Thanks. Chao's got plenty of experience with scrollbar hit testing. Chao, ptal.
This issue is because we add no scrollable scroll node for scrollbar and set it as a child of a no scrollable scroll node in cc gen property tree. 

In LayerTreeHostImpl::IsInitialScrollHitTestReliable, we return false (fallback to main thread scroll) when it does not have a closest_scroll_node.

But for BGPT, we don't add scroll node for scrollbar so that it found the scroll node for scrollable (in this test is viewport) and return true.

The simple fix should be early return false for first_scrolling_layer_or_drawn_scrollbar->IsDrawnScrollbar().


For #6, first_scrolling_layer_or_drawn_scrollbar->IsDrawnScrollbar() should be layer_impl->IsDrawnScrollbar().

But this case still not working. Because the custom scrollbar is PictureLayer here with main thread scroll reason.

{
   "Bounds": [ 15, 600 ],
   "ContentsOpaque": false,
   "DrawsContent": true,
   "HitTestableWithoutDrawsContent": false,
   "Is3dSorted": false,
   "LayerId": 12,
   "LayerType": "cc::PictureLayerImpl",
   "OPACITY": 1.0,
   "Position": [ 785.0, 0.0 ],
   "Transform": [ 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0 ],
   "mainThreadScrollingReasons": "Custom scrollbar scrolling"
}

pdr@, should we query the state (mainThreadScrollingReasons) in LayerImpl. Or create scroll node for scrollbar and set it as child of a no scrollable node.

The property tree:

http://www.mergely.com/AiDEKnZy/
I'm probably misunderstanding but I'm a bit confused by the above. Why do we have a non-scrollable scroll node? Shouldn't the scrollbar be attached to a scrollable node?

Also, the scrollbar gets its own ScrollNode when using CC-GenPropertyTrees? That seems strange to me (the scrollbar itself isn't scrollable), it should be linked to the ScrollNode of the Element/scroller it's attached to.

I think the correct fix would be to ensure that the hit test in IsInitialScrollHitTestReliable returns false when we hit a scrollbar node.
Ok, the link above cleared up a bit of that - seems really strange that we used to create a ScrollNode for the scrollbar itself. IMHO, my recommendation above stands, we should make IsInitialScrollHitTestReliable return false if we're touch scrolling over a scrollbar (and maybe rename the function to better describe its purpose)
Also, unrelated by strange is the difference in user_input_scrollable. That looks like the visual viewport. Please file a bug to look into it.
Yes, scrollbar owns a scroll node in CGPT (See http://www.mergely.com/AiDEKnZy/ left Line 109). This is strange to us. That is why we don't add it when we found this difference when doing the effect node for overlay scrollbar.
layer_impl->IsDrawnScrollbar() seems incorrect.

We call CompositedLayerMapping::ToggleScrollbarLayerIfNeeded create PictureLayer for scrollbar. Then replace it with ScrollbarLayer in ScrollingCoordinator::ScrollableAreaScrollbarLayerDidChange. But we skip create ScrollbarLayer for custom scrollbar.

That is why layer_impl->IsDrawnScrollbar() returns false. We need a bit to identify scrollbar layer. If this is a bug, we should fix it first.
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 28

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

commit d2a75ab847608225d2044bce95398fb2905d798f
Author: chaopeng <chaopeng@chromium.org>
Date: Fri Sep 28 17:41:23 2018

[BGPT] fix virtual/threaded/fast/events/touch/gesture/gesture-scrollbar-mainframe.html

This test failing is because:

1. In cc gen property tree, we create non-scrollable scroll node for scrollbar
   and set it as child of a no scrollable scroll node.
2. In LayerTreeHostImpl::IsInitialScrollHitTestReliable, we return false
   (fallback to main thread scroll) when it does not have a closest_scroll_node
   (scroll node and it's ancestor nodes do not scrollable).
3. In blink gen property tree, we do not create the scroll node for scrollbar so
   closest_scroll_node is the scroll node of scrollable which owns the
   scrollbar. Then the scrolling process in impl thread and scroll on the
   scrollable.

In this patch we return scroll fallback to main thread with scrollbar scrolling
reason if the scroll hit testing layer is scrollbar and from touch before
LayerTreeHostImpl::IsInitialScrollHitTestReliable.

Bug:  884846 
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_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia1b2bb996879b4de75b5f84a7a94c73676bbcb23
Reviewed-on: https://chromium-review.googlesource.com/1243164
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Jianpeng Chao <chaopeng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595143}
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/layers/layer.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/layers/layer.h
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/layers/layer_impl.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/layers/layer_impl.h
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/layers/painted_overlay_scrollbar_layer.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/layers/painted_scrollbar_layer.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/layers/scrollbar_layer_impl_base.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/layers/solid_color_scrollbar_layer.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/d2a75ab847608225d2044bce95398fb2905d798f/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc

Status: Fixed (was: Assigned)

Sign in to add a comment