New issue
Advanced search Search tips

Issue 756740 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Files app: Scroll bar can not be dragged by mouse.

Project Member Reported by fukino@chromium.org, Aug 18 2017

Issue description

Chrome Version: ToT
OS: Chrome OS

What steps will reproduce the problem?
(1) Open Files app
(2) Navigate to a folder which has many files
(3) Try to drag scroll bar using mouse

What is the expected result?
Scroll bar can be dragged

What happens instead?
Area selection is triggered.


I guess this is a regression.
yamaguchi@:
When we migrated to overlay scrollbar, was it working?
 
Owner: yamaguchi@chromium.org
I think this is a recent regression. I will take a look.
- repro on 3189
- no repro on 3182
Owner: chaopeng@chromium.org
Status: Assigned (was: Available)
This also affects other places such as the directory tree view in the Files app. and the console in the developer tools of the browser.

Now I have identified the issue happens after this change.
https://chromium-review.googlesource.com/c/613640
Issue 733546 is the relevant issue.

chaopeng@:
Based on my bisecting result, I am thinking the change on the overlay scrollbar is affecting some apps using scrollable elements. However I am not sure if it's a bug. Will you take a look?
Status: Started (was: Assigned)
Cc: bokan@chromium.org
This issue shows without kExcludeOverlayScrollbarSizeForHitTesting, Hit test may skip the scrollable and go to the children of scrollable. 

So https://chromium-review.googlesource.com/c/613640 is wrong. We should find another approach.

Comment 7 by bokan@chromium.org, Aug 18 2017

Hmm...isn't the issue that we're missing the scrollbar thumb in the hit test? i.e. if the hit test occured outside the scrollbar thumb but still inside the track rect we *should* hit the content and allow selection.

Comment 8 by bokan@chromium.org, Aug 18 2017

Labels: ReleaseBlock-Beta
Lets work on figuring out the correct solution, but +RBB to make sure we don't ship this.

Comment 9 by bokan@chromium.org, Aug 18 2017

Components: -Platform>Apps>FileManager Blink>Scroll
Labels: Hotlist-Input-Dev
The log shows

LayoutBlockFlow (relative positioned) 0x2ba1d3627070 {LIST} at (0,40) size 759x212 id="file-list" class="list"

and

LayoutFlexibleBox (relative positioned) 0x2ba1d3636b48 {LI} at (0,0) size 759x40 id="listitem-1" class="table-row directory"

In the LayoutTree, file-list is listitem-1's parent. But in PaintLayerTree, file-list and listitem-1 in same level. That is why file-list get skiped by fragment correct without kExcludeOverlayScrollbarSizeForHitTesting. 
Project Member

Comment 11 by sheriffbot@chromium.org, Aug 21 2017

This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone.

All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 12 by bokan@chromium.org, Aug 21 2017

Labels: M-62
FYI, this bug also breaks scrollbars dragging on Tweetdeck. So yea, it's a public bug.
If you have a Twitter account, login at https://tweetdeck.twitter.com/ and see.

I'm on Windows with the overlay scrollbar flag enabled.
Cc: pdr@chromium.org
Here is the minimize test case from file app. And we can see the log, PL for <list> is not parent of PL for <li>. 

pdr@ do you have idea about this?
fileapp.html
721 bytes View Download

Comment 15 by bokan@chromium.org, Aug 22 2017

Chao, does this case work on Mac's overlay scrollbars?

Philip, do you know why the scroll container (the <list>) is a sibling PaintLayer to the <li> PaintLayers, rather than their parent as I'd expect?

Attached a slightly more minimized case to Chao's above (to make showLayerTree less verbose). Here's the layer tree output:

*layer 0x82f35614010 at (0,0) size 1559x1068 (composited, bounds=at (0,0) size 1559x1068, drawsContent=1)
  LayoutView 0x82f35604010 at (0,0) size 1559x1068
 positive z-order list(1)
   layer 0x82f356140e8 at (0,0) size 1559x1068
    LayoutBlockFlow 0x82f35624010 {HTML} at (0,0) size 1559x1068
      LayoutFlexibleBox 0x82f35630010 {BODY} at (0,0) size 1559x1068
        LayoutFlexibleBox 0x82f35630188 {DIV} at (0,0) size 1559x1068 id="table"
   normal flow list(1)
     layer 0x82f356141c0 at (0,0) size 800x1068 scrollY 742.00 scrollHeight 3000 (composited, bounds=at (0,0) size 800x1068, drawsContent=1)
      LayoutBlockFlow 0x82f35624130 {LIST} at (0,0) size 800x1068 id="list"
   positive z-order list(1)
     layer 0x82f35614298 at (0,-742) size 800x3000 backgroundClip at (0,0) size 800x1068 clip at (0,0) size 800x1068 (composited, bounds=at (-1,0) size 801x3000, drawsContent=1)
      LayoutListItem (relative positioned) 0x82f3563c010 {LI} at (0,0) size 800x3000
        LayoutListMarker (anonymous) 0x82f35624250 at (-1,0) size 7x17: bullet
        LayoutText 0x82f35644010 {#text} at (22,0) size 42x17
          text run at (22,0) width 42: "Item 1"

Presumably the issue is that we're walking the positive-z list before the scroll container's layer which is where the overflow controls are so we return before checking for scrollbars.
fileapp.html
435 bytes View Download
It also not draggable for Mac

Comment 17 by pdr@chromium.org, Aug 22 2017

The dom order is fairly unrelated to the paint order when positioning is involved. For example:
<!DOCTYPE html>
<style>
  .myrel {
    position: relative;
    background: lightblue;
    width: 100px;
    height: 100px;
  }
  .myrelchild {
    position: relative;
    z-index: -100;
    background: lightpink;
    width: 200px;
    height: 100px;
  }
</style>
<div class="myrel">
  <div class="myrelchild"></div>
</div>

Will produce a layer tree:
 positive z-order list(1)
   layer 450080A8 at (0,0) size 1041x116 layerType: background only
   negative z-order list(1)
     layer 450081D8 at (8,8) size 200x100
      LayoutBlockFlow (relative positioned) 450180D8 zI: -100 {DIV} at (0,0) size 200x100 [bgcolor=#FFB6C1] class="myrelchild"
   ...
   positive z-order list(1)
     layer 45008308 at (8,8) size 100x100
      LayoutBlockFlow (relative positioned) 45018330 {DIV} at (0,0) size 100x100 [bgcolor=#ADD8E6] class="myrel"

The testcase has a lot going on so it's hard to see that this is just stacking context changing paint order. Does that help?

Comment 18 by pdr@chromium.org, Aug 22 2017

Here's an example showing how paint order can be independent of dom order:
https://docs.google.com/presentation/d/1ak7YVrJITGXxqQ7tyRbwOuXB1dsLJlfpgC4wP7lykeo/edit#slide=id.g211348a01e_0_754

Comment 19 by bokan@chromium.org, Aug 23 2017

Yeah, I know paint order can vary in surprising ways. I guess I'm just surprised in this case since I'd expect an overflow clipping parent with scrollbars to be higer up in the paint order than its overflow content. i.e. since we're painting scrollbars over the content.

So it seems to me that the scrollbars actually _are_ painted below the child content in these cases but we've side-stepped the issue since scrollbars used to always totally occlude the content below - we just apply the clip and then we don't see any ordering issues. This works fine until you want scrollbars to actually show some of the content behind them (visibily, Aura overlay scrollbars look correct today because the scrollbars are drawn by the compositor).

This is a bit of a head-scratcher, see https://output.jsbin.com/gukuworaze with overlays turned off. The greenbox is a child of the scroller, but appears on top of the red box which is outside the scroller but draws over the scrollbars. How should an overlay scrollbar behave here? Should the green box be painted over the scrollbars? Should the scrollbars paint overtop of everything (visibly, they do today but not for hit testing)?

I don't have a good solution here yet. pdr, any ideas? Do you know how overlay scrollbars are painted in Blink? They appear over everything today when drawn from Blink (see attached image) so we clearly have a special case for painting them that isn't reflected in hit testing. Perhaps we could insert a dummy paint layer that parents a scroller and its children solely for the purpose of hittesting its scrollbars?

In the mean time, I think we should revert this patch to fix the regression (there's some related perf regressions too).
Screenshot from 2017-08-23 12:34:57.png
6.3 KB View Download

Comment 20 by pdr@chromium.org, Aug 23 2017

I do not think this is specified but I've looked at firefox and edge and our behavior and I think we should paint scrollbars just after we paint the border (https://www.w3.org/TR/CSS2/zindex.html). Our behavior is not exactly this ( https://jsbin.com/vikiyam) and I think that it's a bug. Painting just after the border means that scrollbars would paint before negative z-index children.

I think it's a bug if hit test order and paint order are different.

Comment 21 by pdr@chromium.org, Aug 23 2017

I filed https://crbug.com/758351 for changing our paint and hit test order slightly to paint after the border. It seems like an easy change and will improve compat but so many scrollbar changes seem easy at the start..

Comment 22 by pdr@chromium.org, Aug 23 2017

Tien-Ren pointed out that things are more complicated and overlay scrollbars which are intentionally painted later to avoid being clipped (more explanation in crbug.com/758351).

To the original question in comment #19, is it possible to just special-case hit testing overlay the same way we special-case painting? This seems to be done without an extra paintlayer today.

Comment 23 by bokan@chromium.org, Sep 13 2017

Labels: -ReleaseBlock-Beta
Status: Fixed (was: Started)
Marking this as fixed since the patch in question was reverted in r496917. Lets track re-fixing the original scrollbar hittesting issue in issue 733546 and issue 758351.
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-62; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-62 label, otherwise remove Merge-TBD label. Thanks.

Comment 25 by bokan@chromium.org, Sep 13 2017

Labels: -Merge-TBD
No merge required, the patch was reverted before 62 branched.

Sign in to add a comment