Scroll anchoring causes jumps (probably due to incorrect relative scroll position updates)
Categories
(Core :: Panning and Zooming, defect, P2)
Tracking
()
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(4 files)
While I was trying to create a test case for bug 1779404, I happened to create another variant of test cases causing scroll position jumps. This test case isn't fixed by the fix for bug 1779404, so I suppose there's another underlying issue on our relative scroll position update stuff.
As far as I can tell with the fix for bug 1779404 I don't see any scroll position jumps on Facebook, so I am assuming this case is unrelated to the facebook case. Setting P3:S3 for the reason, but if we received new reports about scroll jumps on Facebook, we will raise this severity.
Assignee | ||
Comment 1•1 year ago
|
||
Assignee | ||
Comment 2•1 year ago
|
||
This is tough.
After chatting with Botond and Timothy on Matrix, I thought using pure scroll position update for scroll anchor adjustment is promising. But in fact it's not, it's not at all.
For example in the cases such as bug 1852818, a scroll anchor adjustment happens in a setTimeout callback due to expanding an element's height above the anchor, and then in a scroll event handler a scroll anchor adjustment happens due to restoring the expanded element. Say something like; the first adjustment +500px pure relative update and the second one is -500px adjustment. They are going to be handled in a single NotifyLayersUpdated call. Then unfortunately we clamp the scroll offset on every update, so that the first update doesn't result +500px change, but the second one result -500px. We might want to defer the clamp later? It won't work if there's a Element.scrollBy call in between the scroll anchor adjustments, we may end up having more scroll related metrics in each ScrollPositionUpdate and use them in each updating functions. I don't think that's a good idea though.
So I'd rather introduce a new scroll position update type, it's an absolute update but it will be merged into the running async scroll animation. That's will be most stable in terms of scroll anchoring because scroll anchor adjustment wants literally to scroll the given destination, i.e. the anchor element position.
I've implemented it locally and it works fine as far as I can tell. And it looks also fine on a try run.
Also note that I implemented it behind a new pref so that we can disable it if it were problematic.
Assignee | ||
Comment 3•1 year ago
|
||
This test is a test case where a scroll anchor adjustment happens while an
async scroll animation is running. Without the dumped metrics it's hard to tell
what's wrong. In fact the test fails if we did use ScrollUpdateType::Absolute
for scroll anchoring
Updated•1 year ago
|
Assignee | ||
Comment 4•1 year ago
|
||
Depends on D190887
Comment 6•1 year ago
|
||
any progress on this? It's been going on for ages.
Comment 7•1 year ago
|
||
(In reply to Brinke Guthrie from comment #6)
any progress on this? It's been going on for ages.
There is a patch posted for this. I did a round of review on it today. It should be merged in the near future, and the fix will appear in Firefox 121 and possibly be backported to Firefox 120.
Note that this is just one of several recent fixes for these kinds of symptoms. Others are:
- bug 1833758 (fixed in Firefox 115)
- bug 1852818 (fixed in Firefox 119)
- bug 1779404 (fixed in Firefox 119)
So, depending on what version you were using when you encountered these symptoms, it's possible they are already fixed by one of the above changes.
Comment 8•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #7)
(In reply to Brinke Guthrie from comment #6)
any progress on this? It's been going on for ages.
There is a patch posted for this. I did a round of review on it today. It should be merged in the near future, and the fix will appear in Firefox 121 and possibly be backported to Firefox 120.
Note that this is just one of several recent fixes for these kinds of symptoms. Others are:
- bug 1833758 (fixed in Firefox 115)
- bug 1852818 (fixed in Firefox 119)
- bug 1779404 (fixed in Firefox 119)
So, depending on what version you were using when you encountered these symptoms, it's possible they are already fixed by one of the above changes.
Wait- what's the current FF build then? It's not in the next revision?
Comment 9•1 year ago
|
||
(In reply to Brinke Guthrie from comment #8)
Wait- what's the current FF build then? It's not in the next revision?
Currently, 119 is released, 120 is in beta, and 121 is under active development.
When this bugfix is merged, it goes on the development ("nightly") branch, and by default will be released in 121, but there is a possibility of backporting a fix to the beta branch (in which case the fix would be released in 120). This fix may be a good candidate for backport since we're early in the release cycle, but that's a call for the patch author and release managers to make.
What site are you experiencing these symptoms on, and with what Firefox version?
Comment 10•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #9)
(In reply to Brinke Guthrie from comment #8)
Wait- what's the current FF build then? It's not in the next revision?
Currently, 119 is released, 120 is in beta, and 121 is under active development.
When this bugfix is merged, it goes on the development ("nightly") branch, and by default will be released in 121, but there is a possibility of backporting a fix to the beta branch (in which case the fix would be released in 120). This fix may be a good candidate for backport since we're early in the release cycle, but that's a call for the patch author and release managers to make.
What site are you experiencing these symptoms on, and with what Firefox version?
Facebook has bounced like a ping pong ball on all FF versions for a couple of years now.
Comment 11•1 year ago
|
||
(In reply to Brinke Guthrie from comment #10)
Facebook has bounced like a ping pong ball on all FF versions for a couple of years now.
Ok, so the first thing I would suggest is trying the very recently released 119, which contains a fix for bug 1779404, which was specifically about Facebook.
Comment 12•1 year ago
|
||
Comment 14•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8229b3ad6bb
https://hg.mozilla.org/mozilla-central/rev/7a509d3a2778
Comment 16•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #11)
(In reply to Brinke Guthrie from comment #10)
Facebook has bounced like a ping pong ball on all FF versions for a couple of years now.
Ok, so the first thing I would suggest is trying the very recently released 119, which contains a fix for bug 1779404, which was specifically about Facebook.
Based on bug 1858022 comment 16, it looks like bug 1779404 did not fix the issue you were seeing.
It would be interesting to see if change made in this bug fixes it. When you get a chance, could you give the latest Nightly version (https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly) a try?
Updated•1 year ago
|
Comment 18•1 year ago
|
||
The patch landed in nightly and beta is affected.
:hiro, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox120
towontfix
.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 19•1 year ago
|
||
I am not going to uplift this change since this change didn't fix the facebook jumply scrolling issue completely and it's unclear to me whether it's mitigated or not.
Updated•1 year ago
|
Description
•