1326290 - scroll bars not moving or not allowing scrolling on Google Sheets

Status: VERIFIED FIXED

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

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

Open a google sheets document that is large enough to need horizontal and vertical scrolling.

* You can click to the side of the scroll bar to move it.  This is good.
* If the scrollbar is at the top/left, you can't click on it and scroll.
* If it's in the middle, you can click and scroll but only towards the top/left.  Once you get there, you're stuck again.
* "Regular" scrolling works, in that the scroll bars follow the page.

Seems to be find on "normal" pages, but it's broken with sheets.

Comment 1

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

What FF version and OS? Assuming Nightly, do you still see it with apz.drag.enabled set to false (live pref, no restart required)?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, nightly, Windows, and the problem goes away with apz.drag.enabled set to false.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #0)
>...
> * If it's in the middle, you can click and scroll but only towards the
> top/left.  Once you get there, you're stuck again.

If it's in the middle you can scroll as long as you stay away from the ends.  Once you hit the end, you're stuck.  While scrolling though, the scroll bar does not move.

Comment 4

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

I can reproduce on OS X, but not consistently. The inconsistency might be related to the fact that my scrollbars fade out and I have to forcibly scroll to make the visible again in order to drag them.

Comment 5

[Botond Ballo [:botond]]

I can partially reproduce this issue: after loading the page, the first drag on the scrollbar has no effect. A second drag works fine. After that, dragging works fine, regardless of the position of the scrollbar before the drag.

Looking into it.

Comment 6

[Botond Ballo [:botond]]

Actually, it doesn't only happen on page load - it also happens if I've been away from the page for a while, and go back to it. This makes me suspect that it  has to do with displayport expiry.

Comment 7

[Botond Ballo [:botond]]

Attachment: Testcase

Here's a minimal testcase that reproduces the problem when scrolling on the subframe.

As I suspected, the problem seems to occur when trying to drag on a scroll frame that doesn't have a displayport. In this test case, that happens on page load, and whenever you've stopped scrolling the subframe for 15 seconds (which is the displayport expiry interval).

Comment 8

[Botond Ballo [:botond]]

So it looks like the following happens:

  - In response to the mouse-down that starts the scrollbar drag,
    the content process decides to layerize the subframe. This
    generates a layer transaction.

  - Also in response to the mouse-down, but elsewhere in the code,
    the content processes decides to start an APZ drag session,
    and sends a StartAsyncScrollbarDrag message.

  - The StartAsyncScrollbarDrag message arrives before the layer
    transaction (because IPC / threads). When it arrives, the
    target APZC doesn't exist yet (the layer transaction is about
    to create it, so we discard the message).

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Note that it can work after mouse-down, but stop working during the same drag.  It may very well be the same cause, but wanted to mention that scenario.  I can always click and drag if the scroll bar is "in the middle", but once the document hits the (either) end, it stops working.  During the same drag (e.g., no new clicks.)

Comment 10

[Botond Ballo [:botond]]

I've had a look, and the behaviour Milan is seeing is definitely worse than what I'm seeing. 

I'm not able to reproduce the worse behaviour. (I'm not sure why, although one possibility is that it only happens on Windows 10, which is what Milan is using, while I only have Windows 8.1 or Linux to test with.)

I'll start by fixing the issue identified in comment 8. We can see if that fixes Milan's issue as well. If not, I may need to get my hands on a Windows 10 device to reproduce and debug Milan's issue.

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
>   - The StartAsyncScrollbarDrag message arrives before the layer
>     transaction (because IPC / threads).

It's not quite "because IPC / threads". The layer transaction doesn't happen until the next refresh driver tick, while the StartAsyncScrollbarDrag message is sent right away.

We had a similar issue with the SetTargetAPZC notification in bug 1154130. The approach we took to fix it there is:

  - Determine whether the scroll frame was just layerized.

  - If so, instead of sending the message right away, register 
    a post-refresh observer that will fire at the end of the
    next refresh driver tick, and send the message in that.

We can employ the same technique here, although determining whether the scroll frame was just layerized will be more tricky since the code to layerize it is "farther away" from the code that sends the message than in the SetTargetAPZC case.

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1326290 - Correct sequencing of layer transaction and StartAsyncScrollbarDrag messages.

In cases where a mouse click that starts a scrollbar drag is also what
layerizes the scroll frame, the StartAsyncScrollbarDrag message needs to
arrive after the layer transaction. This patch ensures it does.

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

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> We can employ the same technique here, although determining whether the
> scroll frame was just layerized will be more tricky since the code to
> layerize it is "farther away" from the code that sends the message than in
> the SetTargetAPZC case.

It wasn't very tricky. I reused the existing InputAPZContext mechanism.

Comment 14

[Botond Ballo [:botond]]

(Kevin, I hope you don't mind me having taken this rather than giving you the opportunity to fix it. I wanted to minimize the amount of time this regression is live on the Nightly channel.)

Comment 15

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ff15a00718c

Comment 16

[Botond Ballo [:botond]]

Milan, could you try this build [1] (from the above Try push), and let me know if you still see your issue?

[1] https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-9ff15a00718c05c9f089d61cd7c547a4c7a7e529/try-win32/firefox-53.0a1.en-US.win32.zip

Comment 17

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

Comment on attachment 8822768 [details]
Bug 1326290 - Correct sequencing of layer transaction and StartAsyncScrollbarDrag messages.

https://reviewboard.mozilla.org/r/101568/#review102222

LGTM, thanks!

::: layout/xul/nsSliderFrame.cpp:961
(Diff revision 1)
> +    delete this;
> +
> +  }

Nit: move the blank line from before the close-brace to after the close-brace

Comment 18

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

(In reply to Botond Ballo [:botond] from comment #16)
> Milan, could you try this build [1] (from the above Try push), and let me
> know if you still see your issue?
> 
> [1]
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 9ff15a00718c05c9f089d61cd7c547a4c7a7e529/try-win32/firefox-53.0a1.en-US.
> win32.zip

FWIW I still see problems with this build.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Botond Ballo [:botond] from comment #16)
> Milan, could you try this build [1] (from the above Try push), and let me
> know if you still see your issue?
> 
> [1]
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 9ff15a00718c05c9f089d61cd7c547a4c7a7e529/try-win32/firefox-53.0a1.en-US.
> win32.zip

The problem I'm seeing goes away with this version.

Comment 21

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> FWIW I still see problems with this build.

Interesting. Since the patch fixes Milan's issue (per comment 19), can you give some details about the problems you're still seeing?

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8822768 [details]
Bug 1326290 - Correct sequencing of layer transaction and StartAsyncScrollbarDrag messages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101568/diff/1-2/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1162c1d1596
Correct sequencing of layer transaction and StartAsyncScrollbarDrag messages. r=kats

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a1162c1d1596

Comment 25

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

(In reply to Botond Ballo [:botond] from comment #21)
> Interesting. Since the patch fixes Milan's issue (per comment 19), can you
> give some details about the problems you're still seeing?

I retested in Jan 5's nightly, which includes your fix, and I still see it. The symptoms are pretty much exactly what Milan described in comment 0, with the additional note that even when I start at a nonzero scroll pos and drag the thumb, the thumb doesn't visibly move even though the page scrolls.

I'll see if I can debug it a bit.

Comment 26

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

I can still reproduce this Google Spreadsheets regression in Nightly 53 build 2017-01-05.

Comment 27

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

Since a real patch landed here, and since it seems to have fixed the issue for Milan, I'm going to close this bug and instead reopen bug 1328658 to track the issue I and cpeterson are still seeing.

Comment 28

[Botond Ballo [:botond]]

Async scrollbar dragging is currently Nightly-only, so marking as firefox-53:disabled.

Comment 29

[ovidiu boca[:Ovidiu]]

I verified this issue on FF Beta 54.0b5 and I found out that in google spreadsheet after you grab and move the scrollbar when you release it, it will jump back (1 pixel). To see this you need to zoom in the browser, I used 240%.

Comment 30

[Botond Ballo [:botond]]

(In reply to ovidiu boca[:Ovidiu] from comment #29)
> I verified this issue on FF Beta 54.0b5 and I found out that in google
> spreadsheet after you grab and move the scrollbar when you release it, it
> will jump back (1 pixel). To see this you need to zoom in the browser, I
> used 240%.

Google Spreadsheets "snaps" the scroll position to align with the beginning of a cell. In the case of scrollbar dragging, this causes the scrollbar to move from the position where you dragged it, to the nearest cell-aligned position. So, this is expected behaviour. It happens with Firefox Release as well.

Comment 31

[ovidiu boca[:Ovidiu]]

Thanks Botond for clearing this out, based on that I will mark this as a verified fix. I tested this issue on Mac OS X 10.10, Windows 10 x32 and Ubuntu 16.04 with FF Beta 54.0b5.