[BGPT] virtual/threaded/fast/events/touch/gesture/gesture-scrollbar-mainframe.html fails |
||||
Issue descriptionvirtual/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.
,
Sep 18
,
Sep 18
Sahel, do you know if this is a real bug or an issue with the test? i.e. should this block BGPT?
,
Sep 18
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.
,
Sep 18
Thanks. Chao's got plenty of experience with scrollbar hit testing. Chao, ptal.
,
Sep 24
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().
,
Sep 24
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.
,
Sep 24
The property tree: http://www.mergely.com/AiDEKnZy/
,
Sep 24
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.
,
Sep 24
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)
,
Sep 24
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.
,
Sep 24
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.
,
Sep 25
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.
,
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
,
Oct 1
|
||||
►
Sign in to add a comment |
||||
Comment 1 by sahel@chromium.org
, Sep 17