Wrong scroll position with scroll anchoring and "scroll-behavior:smooth"
Categories
(Core :: Layout: Scrolling and Overflow, defect, P3)
Tracking
()
People
(Reporter: jan, Assigned: emilio)
References
(Blocks 1 open bug)
Details
(Keywords: nightly-community, parity-chrome, regression, Whiteboard: [layout:backlog:quality])
Attachments
(3 files, 1 obsolete file)
Ctrl+Shift+Alt+R screencast: mozregression --launch 2020-07-03 -a 'https://triggeredpaintz.com/tmp/'
In Firefox, you need to click the arrow twice to scroll to the correct position.
This can be fixed by disabling layout.css.scroll-anchoring.enabled or by removing the "scroll-behavior: smooth;" rule.
mozregression --good 64 --bad 68 -a 'https://triggeredpaintz.com/tmp/'
5:50.79 INFO: Last good revision: ed68c008e5d8db0940a1fbc31bfc24efccba6c26 (2019-01-10)
5:50.79 INFO: First bad revision: c1894cbb4e7150a3a4a16e5d0702e038e7bd466f (2019-01-11)
5:50.79 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ed68c008e5d8db0940a1fbc31bfc24efccba6c26&tochange=c1894cbb4e7150a3a4a16e5d0702e038e7bd466f
|
||
Updated•5 years ago
|
|
Assignee | |
Comment 1•5 years ago
|
||
Probably not super prioritary, because bug 1305957 is on the regression range which is the initial scroll anchoring implementation. But I want to poke at this eventually.
|
|
Assignee | |
Comment 2•5 years ago
|
||
So we're supposed to disable scroll anchoring when an ongoing apz scroll is happening here: https://searchfox.org/mozilla-central/rev/cf561cece0ca9aeaf0202e68699836d957d0c670/layout/generic/ScrollAnchorContainer.cpp#422
However we're resetting that member like this while the scroll still is happening:
#0 mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, mozilla::ScrollOrigin) (this=<optimized out>, aPt=..., aRange=..., aOrigin=<optimized out>)
at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:2905
#1 0x00007fad0c53ed69 in mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) (this=0x7facf4019260, aRange=..., aOrigin=59)
at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:2279
#2 0x00007fad0c53f5d4 in mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, mozilla::ScrollOrigin, nsRect const*, nsIScrollbarMediator::ScrollSnapMode)
(this=0x7facf4019260, aScrollPosition=..., aMode=mozilla::ScrollMode::Instant, aOrigin=mozilla::ScrollOrigin::Apz, aRange=<optimized out>, aSnap=<optimized out>)
at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:2410
#3 0x00007fad0c53facb in mozilla::ScrollFrameHelper::ScrollToCSSPixelsApproximate(mozilla::gfx::PointTyped<mozilla::CSSPixel, float> const&, mozilla::ScrollOrigin)
(this=0x7fad11345bc8, aScrollPosition=..., aOrigin=59) at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:2357
#4 0x00007fad0a12c7db in mozilla::layers::ScrollFrameTo(nsIScrollableFrame*, mozilla::layers::RepaintRequest const&, bool&)
(aFrame=<optimized out>, aRequest=..., aSuccessOut=<optimized out>) at /home/emilio/src/moz/gecko/gfx/layers/apz/util/APZCCallbackHelper.cpp:141
#5 mozilla::layers::ScrollFrame(nsIContent*, mozilla::layers::RepaintRequest const&) (aContent=0x7facef703040, aRequest=...)
at /home/emilio/src/moz/gecko/gfx/layers/apz/util/APZCCallbackHelper.cpp:187
#6 0x00007fad0a12c440 in mozilla::layers::APZCCallbackHelper::UpdateRootFrame(mozilla::layers::RepaintRequest const&) (aRequest=...)
at /home/emilio/src/moz/gecko/gfx/layers/apz/util/APZCCallbackHelper.cpp:355
#7 0x00007fad0be38f98 in mozilla::dom::BrowserChild::UpdateFrame(mozilla::layers::RepaintRequest const&) (this=<optimized out>, aRequest=...)
at /home/emilio/src/moz/gecko/dom/ipc/BrowserChild.cpp:189
#8 0x00007fad0a1863ec in mozilla::layers::APZChild::RecvRequestContentRepaint(mozilla::layers::RepaintRequest const&) (this=<optimized out>, aRequest=...)
at /home/emilio/src/moz/gecko/gfx/layers/ipc/APZChild.cpp:40
#9 0x00007fad09814976 in mozilla::layers::PAPZChild::OnMessageReceived(IPC::Message const&) (this=0x7facf4103680, msg__=...) at PAPZChild.cpp:182
But that is only supposed to happen when we don't have an scroll in progress. But the last scroll origin gets reset here:
#0 0x00007fad0c5a196b in non-virtual thunk to nsHTMLScrollFrame::ResetScrollInfoIfGeneration(unsigned int) () at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.h:470
#1 0x00007fad0a12c54c in mozilla::layers::ScrollFrame(nsIContent*, mozilla::layers::RepaintRequest const&) (aContent=0x7facef703040, aRequest=...)
at /home/emilio/src/moz/gecko/gfx/layers/apz/util/APZCCallbackHelper.cpp:167
#2 0x00007fad0a12c440 in mozilla::layers::APZCCallbackHelper::UpdateRootFrame(mozilla::layers::RepaintRequest const&) (aRequest=...)
at /home/emilio/src/moz/gecko/gfx/layers/apz/util/APZCCallbackHelper.cpp:355
#3 0x00007fad0be38f98 in mozilla::dom::BrowserChild::UpdateFrame(mozilla::layers::RepaintRequest const&) (this=<optimized out>, aRequest=...)
at /home/emilio/src/moz/gecko/dom/ipc/BrowserChild.cpp:189
#4 0x00007fad0a1863ec in mozilla::layers::APZChild::RecvRequestContentRepaint(mozilla::layers::RepaintRequest const&) (this=<optimized out>, aRequest=...)
at /home/emilio/src/moz/gecko/gfx/layers/ipc/APZChild.cpp:40
#5 0x00007fad09814976 in mozilla::layers::PAPZChild::OnMessageReceived(IPC::Message const&) (this=0x7facf4103680, msg__=...) at PAPZChild.cpp:182
#6 0x00007fad09857a07 in mozilla::layers::PCompositorManagerChild::OnMessageReceived(IPC::Message const&) (this=<optimized out>, msg__=...) at PCompositorManagerChild.cpp:376
That message is sent from here:
#0 mozilla::layers::PAPZParent::SendRequestContentRepaint(mozilla::layers::RepaintRequest const&) (this=0x7fa493729840, request=...) at PAPZParent.cpp:77
#1 0x00007fa4a825c4c1 in mozilla::layers::AsyncPanZoomController::RequestContentRepaint(mozilla::layers::FrameMetrics const&, mozilla::gfx::PointTyped<mozilla::ParentLayerPixel, float> const&, mozilla::layers::RepaintRequest::ScrollOffsetUpdateType) (this=0x7fa47f096000, aFrameMetrics=..., aVelocity=..., aUpdateType=<optimized out>)
at /home/emilio/src/moz/gecko/gfx/layers/apz/src/AsyncPanZoomController.cpp:4003
#2 0x00007fa4a8258939 in mozilla::layers::AsyncPanZoomController::RequestContentRepaint(mozilla::layers::RepaintRequest::ScrollOffsetUpdateType)
(this=0x7fa47f096000, aUpdateType=<optimized out>) at /home/emilio/src/moz/gecko/gfx/layers/apz/src/AsyncPanZoomController.cpp:3948
#3 0x00007fa4a825c96b in mozilla::layers::AsyncPanZoomController::UpdateAnimation(mozilla::RecursiveMutexAutoLock const&, mozilla::TimeStamp const&, nsTArray<RefPtr<mozilla::Runnable> >*)
(this=0x7fa47f096000, aProofOfLock=..., aSampleTime=..., aOutDeferredTasks=<optimized out>) at /home/emilio/src/moz/gecko/gfx/layers/apz/src/AsyncPanZoomController.cpp:4058
#4 0x00007fa4a82415f5 in mozilla::layers::AsyncPanZoomController::AdvanceAnimations(mozilla::TimeStamp const&) (this=0x7fa47f096000, aSampleTime=...)
at /home/emilio/src/moz/gecko/gfx/layers/apz/src/AsyncPanZoomController.cpp:4114
#5 0x00007fa4a8240759 in mozilla::layers::APZCTreeManager::AdvanceAnimationsInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&, mozilla::TimeStamp const&)
(this=0x7fa48c60f800, aProofOfMapLock=..., aSampleTime=...) at /home/emilio/src/moz/gecko/gfx/layers/apz/src/APZCTreeManager.cpp:973
#6 0x00007fa4a8241016 in mozilla::layers::APZCTreeManager::AdvanceAnimations(mozilla::TimeStamp const&) (this=0x7fa48c60f800, aSampleTime=...)
at /home/emilio/src/moz/gecko/gfx/layers/apz/src/APZCTreeManager.cpp:899
#7 0x00007fa4a82bca35 in mozilla::layers::AsyncCompositionManager::TransformShadowTree(mozilla::TimeStamp, mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>, mozilla::layers::CompositorBridgeParentBase::TransformsToSkip) (this=0x7fa49259f460, aCurrentFrame=..., aVsyncRate=..., aSkip=mozilla::layers::CompositorBridgeParentBase::TransformsToSkip::NoneOfThem)
at /home/emilio/src/moz/gecko/gfx/layers/composite/AsyncCompositionManager.cpp:1257
So, here, and here continueAnimation is true, and WantsRepaints is true as well. So it seems bad to hit this code while the animation is still running.
Botond, I'm not all that familiar with our apz smooth scroll implementation to know what the best way to fix this is.
I'd naively go for stashing info in the RepaintRequest about whether we're still animating (and if so, don't reset the smooth scroll origin). But again I'm not familiar enough with this code to know for sure.
|
||
Comment 4•5 years ago
|
||
Hmm, I'm a bit lost trying to follow this.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
So we're supposed to disable scroll anchoring when an ongoing apz scroll is happening here: https://searchfox.org/mozilla-central/rev/cf561cece0ca9aeaf0202e68699836d957d0c670/layout/generic/ScrollAnchorContainer.cpp#422
This link is to a line which checks mScrollFrame->mApzSmoothScrollDestination.isSome().
However we're resetting that member like this while the scroll still is happening:
So I assume by "that member", you mean ScrollFrameHelper::mApzSmoothScrollDestination. However...
#0 mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, mozilla::ScrollOrigin) (this=<optimized out>, aPt=..., aRange=..., aOrigin=<optimized out>) [...]
I'm not seeing mApzSmoothScrollDestination being reset in ScrollToImpl(), so I think I'm missing something.
|
|
Assignee | |
Comment 5•5 years ago
|
||
Maybe ScrollToWithOrigin was inlined or something, but anyhow this is the line that resets it: https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/layout/generic/nsGfxScrollFrame.cpp#2404
|
|
Assignee | |
Comment 6•5 years ago
|
||
Err, I was just looking further down in the call stack, sorry :). It's ScrollToWithOrigin what resets it.
|
|
||
Comment 7•5 years ago
•
|
||
So yeah, mApzSmoothScrollDestination does not do a good job of tracking whether there is currently a smooth scroll in progress in APZ. I believe it was introduced with a more narrow use case in mind (something like "the destination of a pending APZ smooth scroll request that APZ hasn't acknowledged yet", see this use site), but since then, two more uses of it have been added (the scroll-anchoring one, and this one in ReflowFinished()) which seem to want the answer to the slightly different question of "is APZ in the process of a smooth scroll animation right now".
I think it makes sense to introduce another variable to track this, and communicate it via RepaintRequest like you suggest.
One thing to note: we do want APZ to be sending regular repaint requests during APZ smooth scrolls, and for those repaints to be getting here. This is the mechanism by which the main thread scroll offset (as reported by e.g. window.scrollX/Y) gets updated, and we want that to be up to date as much as possible. (The scrollInProgress condition that's guarding that block is intended to be more like "is there a main-thread driven scroll animation in progress, or some other scrolling instruction originated by the main thread that hasn't been acknowledged by APZ yet"; it's only under these conditions that we want to hold off on updating the main thread scroll offset to the value sent in the repaint request by APZ.)
So, I believe the solution should not be to avoid getting into ScrollToWithOrigin() at all, but rather to ensure that, unlike mApzSmoothScrollDestination, our new variable only gets clobbered by ScrollToWithOrigin() when APZ indicates that the smooth scroll is completed.
Let me know if that helps! (I was a bit vague about the "communicate it via RepaintRequest" part, I can map that out in more detail if you'd like.)
|
|
Assignee | |
Comment 8•5 years ago
|
||
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 9•5 years ago
|
||
And store that information in the scroll frame for the purposes of
avoiding scroll anchoring and scroll restoration.
Depends on D85156
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 10•5 years ago
|
||
There's no correctness issue here, but the assertion is just wrong. For
the scrolling element we may get the root scrollable frame even though
the primary frame is null.
|
|
||
Comment 11•5 years ago
|
||
Comment on attachment 9166573 [details]
Bug 1650502 - Don't assert that there's a primary frame as long as there's a scroll frame for the root scroll frame. r=mats,#layout-reviewers
Revision D85160 was moved to bug 1655741. Setting attachment 9166573 [details] to obsolete.
|
||
Comment 12•5 years ago
|
||
|
||
Comment 13•5 years ago
|
||
Backed out for failing android at test_frame_reconstruction.html
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311703647&repo=autoland&lineNumber=6079
Backout: https://hg.mozilla.org/integration/autoland/rev/274e86c0eb8fcaf5dc3890b98b84026a44c40bfd
|
|
||
Comment 14•5 years ago
|
||
Looks like this was not restricted to android, occurred on linux too https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311705401&repo=autoland&lineNumber=7505
|
|
||
Comment 15•5 years ago
|
||
A failure of test_frame_reconstruction.html is likely related to the change we made in ReflowFinished().
As that was a tangentially related change that I suggested during review, feel free to spin it off to another bug if you don't want investigating that failure to block this landing.
|
|
Assignee | |
Updated•5 years ago
|
|
|
||
Comment 16•5 years ago
|
||
|
||
Comment 17•5 years ago
|
||
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311788425&repo=autoland&lineNumber=2309
Backout link: https://hg.mozilla.org/integration/autoland/rev/0639c63b67a4df1c3468badde271c65ec773a5ef
|
|
Assignee | |
Comment 18•5 years ago
|
||
ugh, seems like I somehow failed to update the patch with the one-liner that fixes that test :(
|
|
||
Comment 19•5 years ago
|
||
|
|
||
Comment 20•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/4558230653fc
https://hg.mozilla.org/mozilla-central/rev/ae6e361aaef1
|
||
Comment 21•5 years ago
|
||
The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.
For more information, please visit auto_nag documentation.
|
|
Assignee | |
Updated•5 years ago
|
|
||
Updated•5 years ago
|
Description
•