ScrollView ability to clip child layers conflicts with sticky header rows |
|||||||||||
Issue descriptionWhat steps will reproduce the problem? (1) Have several VPN entries in system menu (to be able to scroll the VPN view) (2) Connect VPN (Disconnect button appears) (3) Scroll up What is the expected result? Disconnect button stays below the top header row What happens instead? Since the Disconnect button has a layer (to make 1px border scaling nice) it is stacked above the header row even though the scroll contents is clipped by the header row. This also affects other layer-based views, e.g. ToggleButton's ripple and such when used in ScrollViews in the system menu. This problem is easily fixable by using ScrollView::EnableViewPortLayer(). This however brings its own set of problems. Sticky header rows (implemented here - https://codereview.chromium.org/2453133002/) use ScrollView's contents view OnBoundsChanged() callback to position the sticky header whenever contents are scrolled. EnableViewPortLayer implements scrolling by setting scroll offset on the layer so the contents view is not repositioned and OnBoundsChanged is never called. Worse the layer scroll happens asynchronously so even if I try to push the scroll notification to the ScrollContents there are some graphics frames that contain new scroll offset but stale sticky header row bounds and the scrolling looks janky as a result (the contents are scrolled correctly but the header is bouncing near the top instead of staying put). I'd like to use this bug as a place to discuss some way out that preserves clipping of children in ScrollView but does it in a way that also preserves the sticky rows. I have tried (and it seems to work well) is to decouple additional layer in ScrollView (controlled via a feature that is enabled by default on Mac views or by explicit call to EnableViewPortLayer()) from the scrolling implementation, meaning keeping the scrolling implemented via contents view repositioning even though an additional layer is injected to maintain clipping. This is probably not the right way given the performance wins from the layer scrolling (https://codereview.chromium.org/2188133002). Note that if layer scrolling is enabled by default ("ToolkitViewsScrollWithLayers" feature) then the current MD implementation of the sticky headers in system menu will break. There are probably other ways to satisfy all the pieces of this, would be good to discuss.
,
Nov 11 2016
I have some things in the pipeline. First some context: ToolkitViewsScrollWithLayers came about since we want elastic scrolling for scrollable areas on Mac (Issue 615948). And, after that, smooth scrolling on other platforms, which is also best done with layers, for animations. Would also promoting the header row to a layer work towards fixing the scroll synchronization problems? I wasn't planning to simply because it's rare to need elasticity there and I wasn't hitting any glitches or performance problems (but technically it should be subject to elasticity as well, e.g., when scrolling horizontally). With this, scroll offsets should get applied together in SingleThreadProxy::DoBeginMainFrame(..). E.g. One data point: The scroll bars for views::ScrollView with ToolkitViewsScrollWithLayers enabled always seem to be in sync with the scroll offset in the viewport. On Mac, the (overlay) ScrollBars are in a layer (for opacity), but are just positioned with View::SetPosition() in BaseScrollBar::Update(). There's also some more plumbing to do for funneling input events to the right place to get elasticity. Once these are going through the cc::InputHandler (i.e. cc::LayerTreeHostImpl) we could be in a better position to synchronize with compositor frames. The next step is https://codereview.chromium.org/2197503002/ "Route Scroll events through a cc::InputHandler" *However* The sequence that Scroll Events come in from the OS is special to each platform. And complex. I was focusing on getting Mac working first. After events are routed, the following step is something like https://codereview.chromium.org/2194833002 "Overscroll and Elasticity for views::ScrollView" -- it does the necessary things with scroll trees and transforms to get elastic scrolling in a single layer (rather than only the root viewport as things are currently on Mac - Issue 585766, Issue 546520). Maybe this step is also necessary, or helps, with the synchronization problem. I think I saw some kind of stutter / sync problems with scrollbars at one point while developing that CL, but they went away while sorting out all the bugs. There's on overview CL in http://crrev.com/2189583004 for patching in, but I haven't rebased it in a while.. In any case, it probably doesn't process scroll events from CrOS correctly just yet. (also a disclaimer: I've learnt a lot while writing these CLs, but I'm still a bit of a n00b when it comes to the crazy stuff we do for scrolling in the compositor. I.e. don't treat the above as any kind of expert testimony :).
,
Nov 11 2016
+flackr@ I think it would be useful to understand how this sort of thing is done on the web, and see how much of it we can replicate in views/layers (or what sort of hooks we would need to introduce in views/layers).
,
Nov 11 2016
I don't understand when the header ever gets pushed up (I recreated the error case here but never see headers get pushed up)? If it does not, as seems to be the case on canary, then it doesn't need to be part of the scrolling content, right? If it does need to be pushed up though then we need to implement something similar to position sticky which does the positioning as part of the accelerated composited scrolling.
,
Nov 11 2016
varkha showed me the issue, we should definitely try to use the same sticky concept from the web as it seems a perfect fit for the desired behavior. I think ChromeOS uses the same compositing code as web (i.e. cc::Layers) on the native UI right? If so, you should be able to call cc::Layer::SetStickyPositionConstraint on the headers and set up the same thing that I've used for sticky position composited scrolling on the web: https://cs.chromium.org/chromium/src/cc/layers/layer_sticky_position_constraint.h?sq=package:chromium&dr=CSs&rcl=1478866657&l=18 Let me know if I can be of any help in calculating those constraints, there are examples layer_tree_host_common_unittest.cc.
,
Nov 11 2016
That sounds promising. The ui uses ui::Layers, which have backing cc::Layers. It should be possible to add ui::Layer API that calls into cc::Layer::SetStickyPositionConstraint.
,
Nov 11 2016
this patch fixes the problem for me: http://hastebin.com/raw/buqobaboji
,
Nov 11 2016
#7, yes, this would work but enabling scroll layer by calling EnableViewPortLayer() or enabling "ToolkitViewsScrollWithLayers" feature would still break the sticky header rows. So since we are not enabling that on Chrome OS just yet I will do just that short term. Longer term I will see how to integrate cc::Layer::SetStickyPositionConstraint (and yes, #2, my understanding is that I will need to promote the sticky header rows to their own layers for that)
,
Nov 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb7063bbe11432014548bddea36ce68fe2b36dd5 commit eb7063bbe11432014548bddea36ce68fe2b36dd5 Author: varkha <varkha@chromium.org> Date: Sat Nov 12 00:33:18 2016 [ash-md] Fixes clipping of controls in system tray BUG= 664244 TEST=manual - add more VPN rows, connect one, scroll up to make sure the "Disconnect" button is stacked below the main header row. TEST=manual - Scroll Wi-Fi detailed page up so that the toggle is half-way below the header row. Press mouse, verify that the ripple is clipped byt the header row. Review-Url: https://codereview.chromium.org/2500493004 Cr-Commit-Position: refs/heads/master@{#431708} [modify] https://crrev.com/eb7063bbe11432014548bddea36ce68fe2b36dd5/ash/common/system/tray/tray_details_view.cc
,
Nov 23 2016
Possibly related, here's something that was pointed out to me today: 1. Open the network detailed view with a long list of WiFi devices. 2. Touch down on a device name so that it starts to get the ripple. 3. Fling upwards. You will sometimes see the device name (briefly) appear over top of the WiFi sub-header row.
,
Nov 23 2016
,
Dec 6 2016
,
Dec 6 2016
#10, yes this is the same bug. Stacking is broken when the individual rows have children with layers (e.g. ripple) and it is temporary because the layers only exist while the ripple is active.
,
Dec 6 2016
Marking the rest of this for 57.
,
Dec 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cd9913053dc56f12271b0c73558635ffbc26c580 commit cd9913053dc56f12271b0c73558635ffbc26c580 Author: varkha <varkha@chromium.org> Date: Thu Dec 15 01:00:58 2016 [ash-md] Stacks child layers properly for sticky header rows The CL implements sticky section header rows in VPN page. Care is taken to stack the rows with child controls that have layers below the sticky section header rows. BUG= 664244 TEST=Run with --material-design-ink-drop-animation-speed=slow Touch a Wi-Fi connection row and while ripple is growing drag-scroll the list of connections up. Verify that the ripple is stacked below the Wi-Fi header row. Review-Url: https://codereview.chromium.org/2557333003 Cr-Commit-Position: refs/heads/master@{#438689} [modify] https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580/ash/common/system/chromeos/network/network_list_md.cc [modify] https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580/ash/common/system/chromeos/network/vpn_list_view.cc [modify] https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580/ash/common/system/tray/tray_details_view.cc [modify] https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580/ash/common/system/tray/tray_details_view_unittest.cc [modify] https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580/ash/common/system/tray/tray_popup_utils.cc [modify] https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580/ash/common/system/tray/tray_popup_utils.h
,
Dec 22 2016
Merge request for https://codereview.chromium.org/2557333003 only. This also fixes an issue where the ink drop on wifi detailed rows incorrectly shows up on top of the sticky header row.
,
Dec 22 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Dec 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7000b78c4fb63b02f48700904b8f6c44f828322c commit 7000b78c4fb63b02f48700904b8f6c44f828322c Author: Ben Ruthig <bruthig@chromium.org> Date: Thu Dec 22 22:26:00 2016 [ash-md] Stacks child layers properly for sticky header rows The CL implements sticky section header rows in VPN page. Care is taken to stack the rows with child controls that have layers below the sticky section header rows. BUG= 664244 TEST=Run with --material-design-ink-drop-animation-speed=slow Touch a Wi-Fi connection row and while ripple is growing drag-scroll the list of connections up. Verify that the ripple is stacked below the Wi-Fi header row. Review-Url: https://codereview.chromium.org/2557333003 Cr-Commit-Position: refs/heads/master@{#438689} (cherry picked from commit cd9913053dc56f12271b0c73558635ffbc26c580) Review-Url: https://codereview.chromium.org/2602483002 . Cr-Commit-Position: refs/branch-heads/2924@{#602} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/7000b78c4fb63b02f48700904b8f6c44f828322c/ash/common/system/chromeos/network/network_list_md.cc [modify] https://crrev.com/7000b78c4fb63b02f48700904b8f6c44f828322c/ash/common/system/chromeos/network/vpn_list_view.cc [modify] https://crrev.com/7000b78c4fb63b02f48700904b8f6c44f828322c/ash/common/system/tray/tray_details_view.cc [modify] https://crrev.com/7000b78c4fb63b02f48700904b8f6c44f828322c/ash/common/system/tray/tray_details_view_unittest.cc [modify] https://crrev.com/7000b78c4fb63b02f48700904b8f6c44f828322c/ash/common/system/tray/tray_popup_utils.cc [modify] https://crrev.com/7000b78c4fb63b02f48700904b8f6c44f828322c/ash/common/system/tray/tray_popup_utils.h
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/321f8fbf1e90199a9d8bbbe14f5a99db92e85246 commit 321f8fbf1e90199a9d8bbbe14f5a99db92e85246 Author: varkha <varkha@chromium.org> Date: Tue Jan 03 23:21:48 2017 [ash-md] Adds support for Z-order iteration in views::View This CL is a demonstration of an alternative to https://codereview.chromium.org/2557333003/ . This alternative shows how we could add a custom iterator over children to views::View. Benefits: - Avoids need to override all of those: View::GetTooltipHandlerForPoint View::PaintChildren View::ReorderChildLayers ViewTargeterDelegate::TargetForRect just for the sake of implementing Z-order. We may still need to override them to add custom painting and such but not to mess with the Z-order. - Keeps the iterations in views::View base class implementation BUG= 664244 Review-Url: https://codereview.chromium.org/2561253002 Cr-Commit-Position: refs/heads/master@{#441247} [modify] https://crrev.com/321f8fbf1e90199a9d8bbbe14f5a99db92e85246/ash/common/system/tray/tray_details_view.cc [modify] https://crrev.com/321f8fbf1e90199a9d8bbbe14f5a99db92e85246/ui/views/view.cc [modify] https://crrev.com/321f8fbf1e90199a9d8bbbe14f5a99db92e85246/ui/views/view.h [modify] https://crrev.com/321f8fbf1e90199a9d8bbbe14f5a99db92e85246/ui/views/view_targeter_delegate.cc [modify] https://crrev.com/321f8fbf1e90199a9d8bbbe14f5a99db92e85246/ui/views/view_unittest.cc
,
Jan 3 2017
,
Jan 10 2017
,
May 9 2017
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by varkha@chromium.org
, Nov 10 2016