New issue
Advanced search Search tips

Issue 701025 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

[scroll anchoring] large jumps with scrollbar dragging

Project Member Reported by skobes@chromium.org, Mar 13 2017

Issue description

Reproduced on Linux and Windows with trunk build.

What steps will reproduce the problem?
1. Open https://output.jsbin.com/kabubun/quiet
2. Using the mouse wheel, scroll down slightly, so that the orange box is offscreen, but the green box is partly visible.
3. Drag the scrollbar down, until the blue box is on screen.

What is the expected result?
Blue box should remain on-screen.

What happens instead of that?
While the scrollbar is dragged, or soon after it is released, the scroll position jumps back to the green box, or to the white space below it.
 

Comment 1 by skobes@chromium.org, Mar 13 2017

Cc: ajuma@chromium.org
The bug only repros with scroll anchoring enabled, but the jump is not coming from ScrollAnchor.  It also goes away with --disable-threaded-compositing, so I suspect something to do with cc scroll offset sync'ing.

Comment 2 by skobes@chromium.org, Mar 15 2017

It looks like this is coming from the animation adjustments that we trigger for ScrollType == AnchoringScroll.

ScrollOffsetAnimationsImpl::ScrollAnimationApplyAdjustment thinks we are still in the middle of an impl-side smooth-scroll that actually finished long ago.  It "adjusts" that animation, causing a jump to the target of the most recent wheel scroll.

Comment 3 by skobes@chromium.org, Mar 15 2017

Cc: loyso@chromium.org

Comment 4 by skobes@chromium.org, Mar 16 2017

Patch up: http://crrev.com/2757453003

Fun fact: this is also a memory leak.  If you log the size of AnimationPlayer::animations_, it grows every time you scroll.
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 16 2017

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

commit e26333eb717aa94b9f05ccf34ec91bfe31927f2a
Author: skobes <skobes@chromium.org>
Date: Thu Mar 16 15:36:41 2017

Purge impl-only animations awaiting deletion in AnimationPlayer::UpdateState.

Previously, animations in WAITING_FOR_DELETION state would only be purged via
AnimationPlayer::PushPropertiesTo, which synchronizes impl- and main-thread
players.  But the player in ScrollOffsetAnimationsImpl has no main-thread copy,
so its animations never got deleted after they finished.

BUG= 701025 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2757453003
Cr-Commit-Position: refs/heads/master@{#457443}

[modify] https://crrev.com/e26333eb717aa94b9f05ccf34ec91bfe31927f2a/cc/animation/animation_player.cc
[modify] https://crrev.com/e26333eb717aa94b9f05ccf34ec91bfe31927f2a/cc/animation/animation_player.h
[modify] https://crrev.com/e26333eb717aa94b9f05ccf34ec91bfe31927f2a/cc/animation/element_animations_unittest.cc

Comment 6 by skobes@chromium.org, Mar 17 2017

Labels: -OS-Android -OS-Mac
Labels: TE-Verified-59.0.3043.0 TE-Verified-M59
Tested the issue on windows 7, Linux Ubuntu 14.04 using chrome version# 59.0.3043.0 with the steps mentioned in comment #0.Observed that the blue box remains on screen after leaving the mouse too. Hence adding TE-Verified labels.
Please find the attached screen cast for the same.
Thanks!!
701025.mp4
318 KB View Download

Comment 8 by skobes@chromium.org, Mar 17 2017

Labels: -Type-Bug ReleaseBlock-Stable Merge-Request-58 M-58 Type-Bug-Regression
Requesting merge into M58 based on severity.  (This regressed in M56.)
Project Member

Comment 9 by sheriffbot@chromium.org, Mar 17 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

Comment 10 by bugdroid1@chromium.org, Mar 17 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72d4a9b5ceda4f5cca9c8c91101d92ed051c6b08

commit 72d4a9b5ceda4f5cca9c8c91101d92ed051c6b08
Author: Steve Kobes <skobes@chromium.org>
Date: Fri Mar 17 18:18:31 2017

Purge impl-only animations awaiting deletion in AnimationPlayer::UpdateState.

Previously, animations in WAITING_FOR_DELETION state would only be purged via
AnimationPlayer::PushPropertiesTo, which synchronizes impl- and main-thread
players.  But the player in ScrollOffsetAnimationsImpl has no main-thread copy,
so its animations never got deleted after they finished.

BUG= 701025 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2757453003
Cr-Commit-Position: refs/heads/master@{#457443}
(cherry picked from commit e26333eb717aa94b9f05ccf34ec91bfe31927f2a)

Review-Url: https://codereview.chromium.org/2756963002 .
Cr-Commit-Position: refs/branch-heads/3029@{#271}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/72d4a9b5ceda4f5cca9c8c91101d92ed051c6b08/cc/animation/animation_player.cc
[modify] https://crrev.com/72d4a9b5ceda4f5cca9c8c91101d92ed051c6b08/cc/animation/animation_player.h
[modify] https://crrev.com/72d4a9b5ceda4f5cca9c8c91101d92ed051c6b08/cc/animation/element_animations_unittest.cc

Status: Fixed (was: Started)
Labels: TE-Verified-M58 TE-Verified-58.0.3029.33
Verified the issue on windows 10 and Ubuntu 14.04 using chrome beta version #58.0.3029.33 as per comment #0.

Observed that blue box remained on-screen when the scrollbar was released. Hence, the fix is working as expected.

Attaching screen cast for reference.

Hence, adding the verified labels.

Thanks...!!

701025.mp4
633 KB View Download

Sign in to add a comment