Closed Bug 1402995 Opened 4 years ago Closed 4 years ago
Scroll thumb not synchronized with mouse
2.20 MB, text/html
251.80 KB, image/gif
2.24 KB, patch
|Details | Diff | Splinter Review|
59 bytes, text/x-review-board-request
1. Open attached testcase 2. Click "Pretty Print" button 3. Wait until the text is prettified 4. Mousedown the scroll node and move the mouse vertically. Expected: the scroll node follows the mouse pointer perfectly. Result: the scroll node can move more or less than the mouse. Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=41d11d56768c0f6125aaeda5633786764daf6aef&tochange=eb52bae91633e62a94968238f9f4d41d8ed4be06
I can repro on OS X, 56 beta as well. For me it's only the initial part of the drag that's wonky, after that it sort of fixes itself. It seems like the APZ state right at the start of the drag is out of sync with reality, and as soon as it gets an update from the main thread things are ok again. In the attached screencast though the badness persists longer, so maybe there's more to it. Botond, can you take a look?
4 years ago
Summary: Scroll node not synchronized with mouse → Scroll thumb not synchronized with mouse
I'm not able to reproduce this bug on Linux. Will try Windows next.
I'm able to reproduce on Windows. Will investigate.
Assignee: nobody → botond
Maybe related to Bug 1331390?
(In reply to avada from comment #5) > Maybe related to Bug 1331390? I don't think so, since this bug is specific to APZ scrollbar dragging, whereas bug 1331390 is about main-thread scrollbar behaviour.
(In reply to Kartikaya Gupta (email:email@example.com) from comment #2) > For me it's only the initial part of > the drag that's wonky, after that it sort of fixes itself. It seems like the > APZ state right at the start of the drag is out of sync with reality, and as > soon as it gets an update from the main thread things are ok again. In the > attached screencast though the badness persists longer, so maybe there's > more to it. I don't think there's anything different about what the OP's screencast depicts and what you described, except that in the screencast it takes a long time for the main thread update to arrive.
It looks like the issue is related to the "interruptible reflow" feature. Pressing the button on the attached test page causes a very long reflow. In such cases, instead of waiting for the reflow to complete before painting, we interrupt the reflow, paint the results of the partial reflow, and continue it on the next refresh driver tick. The problem is that for scroll frames, a partial reflow can leave the size of the scrolled rect and properties of the scrollbars (such as the thumb ratio) inconsistent (because we basically don't reflow the scrollbars until the whole reflow is complete). We then get a paint with these inconsistent values, which are propagated to APZ, and cause incorrect behaviour during async scrollbar dragging.
(Also, I can now reproduce the issue on Linux as well. It's just harder to repro because we complete the entire reflow faster. In a debug build it's plenty slow enough to trigger the bug though.)
Timothy, what do you think about this fix approach? The idea is that when doing a partial reflow on a scroll frame, we skip the code to do scrolling (as we did before), but we still reflow the scrollbars. I tried it and it does fix the problem, although it has the downside that during a long reflow, the scrollbar size will change more times to reflect intermediate states. In particular, on this test page, where the scrollable height changes from a large value (~200k pixels) to an even larger value (~4M pixels), we get a partial reflow with a much smaller height, and as a result of this change, the scrollbar briefly changes to be long before becoming short again. Do you think that's an acceptable side effect, or should we explore other solutions?
Comment on attachment 8915276 [details] [diff] [review] Possible fix approach Hmm, so the rest of ReflowFinished does a few things besides set the scrollbar attributes. It calls FrameNeedsReflow, and CurPosAttributeChanged. CurPosAttributeChanged calls ScrollToWithOrigin, so isn't this going to regress bug 1092626?
Updated the patch to address comment 11. Recording a few things we discussed on IRC: - We skip the ScrollToWithOrigin() call inside CurPosAttributeChanged(), but not the entire call to CurPosAttributeChanged(), because that's important for updating the scrollbar properly (without it, the patch doesn't fix the bug). - For re-flowing fixed-position children, we could probably go either way. This patch doesn't skip it for simplicity. - We want to return false from ReflowFinished() in the partial reflow case, otherwise we get layout flush which would cause a full reflow.
Comment on attachment 8916179 [details] Bug 1402995 - Reflow scrollbars after partial reflow of scroll frame. https://reviewboard.mozilla.org/r/187428/#review193380 Did you double check that we aren't regressing bug 1092626?
Attachment #8916179 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #15) > Did you double check that we aren't regressing bug 1092626? I can't reproduce bug 1092626 with my patch applied. However, that may not mean much, because I also can't reproduce bug 1092626 if I comment out the "doScroll = false" line. Perhaps something change on Twitter's side, or bug 1092626 is just hard to reproduce to begin with.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/47f85792a78c Reflow scrollbars after partial reflow of scroll frame. r=tnikkel
Given where we are in the cycle, I think this should probably ride the 58 train, but feel free to set the 57 status back to affected and request Beta approval if you feel strongly otherwise.
I'm happy to have this ride the 58 train.
QA Whiteboard: [good first verify]
I have successfully reproduced the bug in firefox Nightly 59.0a1 (2017-09-25) (32-bit) with windows 10 (32 bit) Verified as fixed with latest beta 58.0b13 (32-bit) Build ID: 20171226085105 Mozilla/5.0 (Windows NT 10.0; rv:58.0) Gecko/20100101 Firefox/58.0
QA Whiteboard: [good first verify] → [testday-20171222]
You need to log in before you can comment on or make changes to this bug.