ui::Layer::SetScrollOffset fails to scroll and DCHECKs when called soon after a view is resized |
|||||
Issue descriptionChrome Version: 62.0.3198.2 OS: All, but most likely to happen on Mac. What steps will reproduce the problem? Enable kToolkitViewsScrollWithLayers in scroll_view.cc Set up a ScrollView with a View inside as contents. On Mac the ScrollView will use layers for the contents. Now, increase the size of the contents View and call ScrollRectToVisible() on it to scroll to the end. What is the expected result? The ScrollView scrolls to show the end of the contents View. What happens instead? ui::Layer::SetScrollOffset() calls compositor->ScrollLayerTo() in order to perform the scroll. The problem is the Compositor still has old info about the layer, before it has been resized to a larger size. Therefore it cannot scroll to the desired position because it falls outside the bounds of its (stale) information about the layer. At least that's what I think, as I don't know the Compositor very well. ui::Layer::SetScrollOffset() also has a second path that calls cc_layer_->SetScrollOffset(offset) and that seems to work fine (I guess the scroll is then committed to the Compositor with the rest of the changes and therefore can operate on updated layer size). I think this can also be reproduced with any other layered scrollable view.
,
Aug 30 2017
The issue probably isn't Mac-specific, it's just that on Mac ScrollView uses layers by default. However it is my understanding that the problem may occur in other settings. I could attempt fixing it but I'm not sure exactly how.
,
Aug 30 2017
->tapted who implemented this. I think there are issues in trying to do absolute scrolls on the impl side (because of out-of-date info, as hinted here), and even more so in trying to do sometimes a scroll on the impl side and sometimes on the main side (what does Layer::CurrentScrollOffset() even mean in that case?). At the very least you'd need to track whether there are structural changes (hierarchy, bounds) that haven't been committed yet and fall back to main thread scrolling when there are any.
,
Aug 30 2017
Yep - there are some gaps in how this is implemented. Without a solid use-case, I've been hesitating to add any additional logic in case we make the refactoring, e.g., in Issue 620721 more complicated. (Issue 394772 is also somewhat related I guess). One maybe-use-case I've been looking at is when you resize the Edit Bookmark dialog that shows on Mac with chrome://flags/#secondary-ui-md Enabled while you have a bookmark folder selected, and enough folders that the selected folder may need to be scrolled in. This can hit [23006:775:0830/173256.228664:FATAL:layer.cc(951)] Check failed: offset.y() == CurrentScrollOffset().y() (72 vs. 66) 0 libbase.dylib 0x00000001058ea38e base::debug::StackTrace::StackTrace(unsigned long) + 174 1 libbase.dylib 0x00000001058ea44d base::debug::StackTrace::StackTrace(unsigned long) + 29 2 libbase.dylib 0x00000001058e86fc base::debug::StackTrace::StackTrace() + 28 3 libbase.dylib 0x00000001059878af logging::LogMessage::~LogMessage() + 479 4 libbase.dylib 0x0000000105985215 logging::LogMessage::~LogMessage() + 21 5 libcompositor.dylib 0x00000001115b468c ui::Layer::SetScrollOffset(gfx::ScrollOffset const&) + 652 6 libviews.dylib 0x00000001105b51de views::ScrollView::ScrollToOffset(gfx::ScrollOffset const&) + 94 7 libviews.dylib 0x00000001105b612e views::ScrollView::ScrollContentsRegionToBeVisible(gfx::Rect const&) + 2718 8 libviews.dylib 0x00000001105b7000 views::ScrollView::Viewport::ScrollRectToVisible(gfx::Rect const&) + 576 9 libviews.dylib 0x00000001106752c3 views::View::ScrollRectToVisible(gfx::Rect const&) + 131 10 libviews.dylib 0x0000000110617938 views::TreeView::LayoutEditor() + 648 But - The Edit Bookmark dialog isn't resizable on other platforms -- just Mac -- we could just disabled resizing - Nothing explodes if we just remove those DCHECKs I added There also might be a simple fix to just convert this to a scroll delta - that's just not how views::ScrollView currently thinks about things.
,
Aug 1
,
Nov 26
*** UI Mass Triage *** |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by piman@chromium.org
, Aug 28 2017