1402995 - Scroll thumb not synchronized with mouse

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Oriol Brufau [:Oriol]]

Attachment: testcase.htm

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

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: screencast.gif

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 3

[Botond Ballo [:botond]]

I'm not able to reproduce this bug on Linux. Will try Windows next.

Comment 4

[Botond Ballo [:botond]]

I'm able to reproduce on Windows. Will investigate.

Comment 5

[avada]

Maybe related to Bug 1331390?

Comment 6

[Botond Ballo [:botond]]

(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.

Comment 7

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 8

[Botond Ballo [:botond]]

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.

Comment 9

[Botond Ballo [:botond]]

(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.)

Comment 10

[Botond Ballo [:botond]]

Attachment: Possible fix approach

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 11

[Timothy Nikkel (:tnikkel)]

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?

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1402995 - Reflow scrollbars after partial reflow of scroll frame.

Review commit: https://reviewboard.mozilla.org/r/187428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187428/

Comment 13

[Botond Ballo [:botond]]

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 14

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80f7b712b984c3685210355b09297f4bf0ecafa6

Comment 15

[Timothy Nikkel (:tnikkel)]

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?

Comment 16

[Botond Ballo [:botond]]

(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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47f85792a78c
Reflow scrollbars after partial reflow of scroll frame. r=tnikkel

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/47f85792a78c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 20

[Botond Ballo [:botond]]

I'm happy to have this ride the 58 train.

Comment 21

[Md. Rahimul Islam]

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