1818721 - Jumpy scrollbar on Google Sheets

Status: VERIFIED FIXED

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

Description

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: jumpy_scroll_00.gif

Found in

• 112.0a1 (2023–02-23)

Affected versions

• 112.0a1 (2023–02-23)
• 110.0b5

Tested platforms

• Affected platforms: Windows 10x64
• Unaffected platforms: macOS 12, Ubuntu 20

Steps to reproduce

1. Open Firefox and https://docs.google.com/spreadsheets/d/1fq-rFyj7GjSssseposuUMXKFRs6DEUmtkeyamahwTxs/edit#gid=0.
2. Drag the scroll bar or use the auto scroll on the scroll bar to scroll up and down and observe the behavior.

Expected result

• The scroll bar moves smoothly.

Actual result

• Jumpy scrollbar when scrolling.

Regression range

• Last good revision: 9d7979f180dcf8ef859014d805b32d03f5c6252a
  First bad revision: ef1bbae9b06b9c53da70f03ceda62d2055326a6d
  Pushlog:
  https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9d7979f180dcf8ef859014d805b32d03f5c6252a&tochange=ef1bbae9b06b9c53da70f03ceda62d2055326a6d

Additional notes

• Attached a screen recording.
• Sometimes scrolling to the maximum will hide the scroll bar completely and make it jump back.
• The issue may be intermittent sometimes.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:rzvncj, since you are the author of the regressor, bug 1554795, could you take a look?

For more information, please visit auto_nag documentation.

Comment 2

[Razvan Cojocaru]

Changing RI to Botond.

Comment 3

[Donal Meehan [:dmeehan]]

Setting 111 to fixed as the regressor, Bug 1554795, was backed out of beta

Comment 4

[Botond Ballo [:botond]]

Alexandru, could you please re-test this with the latest Nightly? It may have been fixed by bug 1818702 + bug 1819848, but I'm not sure.

Comment 5

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: scroll_00.gif

(In reply to Botond Ballo [:botond] from comment #4)

Alexandru, could you please re-test this with the latest Nightly? It may have been fixed by bug 1818702 + bug 1819848, but I'm not sure.

Sure thing. Unfortunately, I can still see the issue with the latest Firefox Nightly (20230305214646) on Windows 10x64. I attached a screen recording showing the case with the jumpy scrollbar and scrollbar shrinking when dragged to the end of the document. It's still intermittent but reproducible. If more information is needed please let me know. Thank you!

Comment 6

[Botond Ballo [:botond]]

Thanks, Alexandru. I can reproduce at least the stutter part on Linux (most noticeable when dragging downwards with the thumb starting at the top), and confirm that it starts in nightlies after the landing of bug 1554795. I don't have a theory offhand about what goes wrong but bug 1554795 is definitely a plausible regressor since it affects thumb positioning and sizing.

Will mark this as P3 for now. We can consider raising the priority if we get additional user reports about it.

Comment 7

[Botond Ballo [:botond]]

Removing "[win]" from bug title as this doesn't seem to be Windows-specific.

Comment 8

[Dianna Smith [:diannaS]]

Setting 112 to fixed as well since the regressor, Bug 1554795, was backed out of 112.0rc2
113 is still affected

Comment 9

[Botond Ballo [:botond]]

(In reply to Dianna Smith [:diannaS] from comment #8)

113 is still affected

As a further update, thanks to bug 1826499, 113 and onward are only affected on the nightly channel (the change was put behind a pref that's disabled on beta and release).

Comment 10

[Razvan Cojocaru]

Attachment: logging.patch

I'm attaching the logging patch I've been using to debug this today, as requested by Botond in our private chat on Matrix. It's rough but I hope it's helpful.

Comment 11

[Razvan Cojocaru]

Just to update the activity here, Botond has had the great insight that this might be a consequence of apz.paint_skipping.enabled=true, which was at least confirmed by my testing. Setting apz.paint_skipping.enabled=false "fixes" the issue for me, which points to the problem probably being caused by skipped full state updates from the main thread to the compositor thread.

Comment 12

[Botond Ballo [:botond]]

Marking the status as disabled for 113 because the affected code has been put behind a nightly-only pref in bug 1826499.

Comment 13

[Botond Ballo [:botond]]

A few preliminary notes based on reading over the paint skipping code:

• There are two varieties of paint skipping:
  • This branch is for skipping paints triggered by an APZ repaint request in cases where the repaint request did not change the displayport (after aligning it to tile boundaries). In such cases, APZ already has the new scroll offset (it's the one telling the main thread to scroll), so no message needs to be sent to APZ.
  • This branch is for skipping paints triggered by scrolling that originates on the main-thread, but does not change the tile-aligned displayport. In such cases, we need to send APZ a message telling it about the scrolling, which takes the form of an "empty transaction" (a paint transaction that doesn't have actual display list data, hence it's "empty").
• The second variety has been broken since 2021. This is tracked in bug 1803713. "Broken" here means we bail at this check and schedule a full paint instead. This is unfortunate for performance reasons, but it's not related to these jumpy scrollbar bugs.
• Since the only kind of paint-skipping we end up doing is the first kind, it follows that these jumpy scrollbar bugs are somehow caused by this kind of paint skipping. It's not clear to me from inspection why that might be, some debugging will be needed.

Comment 14

[Botond Ballo [:botond]]

For debugging this, I switched to the testcase from bug 1826947 rather than Google Sheets, because Google Sheets is almost unusably slow in a debug build; by comparison, the testcase in bug 1826947 is reduced and much lighter-weight.

An interesting thing I noticed is that in the bug 1826947 testcase, setting apz.paint_skipping.enabled=false does not make the bug go away. The STR there do involve pinch-zooming in, so it seems the bug can be triggered as a result of paint skipping OR pinch-zooming.

Comment 15

[Botond Ballo [:botond]]

It looks like the main thread is repainting and updating the thumb position visually, but failing to update the value of mThumbStart sent to the compositor, such that the compositor is basing its calculations on a stale value of mThumbStart. The non-recalc codepath is not affected because it does not use mThumbStart as an input to its calculation.

Comment 16

[Botond Ballo [:botond]]

Setting layout.display-list.retain=false makes the bug go away, suggesting that there is a bug in the retained display list codepath that fails to update mThumbStart during partial display list updates. The relevance of paint skipping and pinch-zooming is likely that they affect whether we get partial updates (mThumbStart remains stale) vs. full display list rebuilds (mThumbStart is updated correctly).

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #16)

suggesting that there is a bug in the retained display list codepath that fails to update mThumbStart during partial display list updates

So mThumbStart is stored in the mScrollbarData field of nsDisplayOwnLayer during display list building.

During a partial update, I see us get into MergeChildLists() with aNewItem being a newly built nsDisplayOwnLayer with the correct (up to date) mScrollbarData.mThumbStart, aOldItem being the old nsDisplayOwnLayer with the stale mScrollbarData.mThumbStart, and aOutItem = aOldItem.

The bounds of the thumb's OwnLayer item are updated in this UpdateBounds() call, but the mScrollbarData remains stale.

Timothy, what would be the appropriate place to copy over the mScrollbarData from the new item to the old one, or would some other solution (e.g. getting ShouldUseNewItem() to return true for OwnLayer items) be appropriate here?

Comment 18

[Timothy Nikkel (:tnikkel)]

So we create the own layer item here

https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/layout/xul/nsSliderFrame.cpp#516

using the slider frame as the frame of the item, but it's the thumb frame that has moved. Are we marking the thumb frame as modified but not the slider frame?

Looks like making the display item us the frame of the slider was done in https://hg.mozilla.org/mozilla-central/rev/8891a5d7c79c

If we want to keep that setup (ie we don't want to risk changing it or the issue in bug 1148515 is still an issue) then I think there are two options, always mark the slider parent frame as modified if the thumb is marked modified, or extend ShouldUseNewItem to handle the thumb case too.

Comment 19

[Botond Ballo [:botond]]

Thanks for the suggestions!

(In reply to Timothy Nikkel (:tnikkel) from comment #18)

Are we marking the thumb frame as modified but not the slider frame?

Yes, that appears to be the case.

The place where the thumb frame is marked as modified is this SetRect() call, whose implementation calls MarkNeedsDisplayItemRebuild(). There's nothing comparable that marks the nsSliderFrame itself for rebuild in that situation.

Comment 20

[Timothy Nikkel (:tnikkel)]

Okay in that case any of the proposed solutions from comment 18 seem okay to me. Although it might be a bit harder to make sure we catch all ways the thumb frame can be marked modified to make sure to mark the slider frame too.

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1818721 - Invalidate the slider frame when the thumb is moved. r=tnikkel

The thumb's position is sent to the compositor in ScrollbarData::mThumbStart
stored on the OwnLayer item built by the slider frame, so we need to
invalidate the slider frame when the thumb's position changes.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:botond, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 23

[Botond Ballo [:botond]]

The patch hasn't landed yet because it doesn't include a test yet. Work on a test is ongoing.

Comment 24

[Botond Ballo [:botond]]

Attachment: Bug 1818721 - Remove unused test helper getBoundingClientRectRelativeToVisualViewport(). r=hiro

This has been unused since bug 1637776.

Comment 25

[Botond Ballo [:botond]]

Attachment: Bug 1818721 - Remove an unused call to getVisualViewportOffsetRelativeToLayoutViewport(). r=hiro

Depends on D177990

Comment 26

[Botond Ballo [:botond]]

Attachment: Bug 1818721 - Add a getRelativeViewportOffset() helper. r=hiro

This is a more ergonomic wrapper around getVisualViewportOffsetRelativeToLayoutViewport().

Depends on D177991

Comment 27

[Botond Ballo [:botond]]

Attachment: Bug 1818721 - Add a targetIsWindow() helper. r=hiro

Depends on D177992

Comment 28

[Botond Ballo [:botond]]

Attachment: Bug 1818721 - Make promiseVerticalScrollbar[Touch]Drag() work on pinch-zoomed pages. r=hiro

Depends on D177993

Comment 29

[Botond Ballo [:botond]]

Attachment: Bug 1818721 - Document what kind of coordinates hitTest() expects. r=hiro

Depends on D177994

Comment 30

[Botond Ballo [:botond]]

Attachment: Bug 1818721 - Add a mochitest. r=hiro

Comment 31

[Botond Ballo [:botond]]

Try push with test patches: https://treeherder.mozilla.org/jobs?repo=try&revision=c41ad4d0d9f03d45e26202e66231c238d053110c

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdf60665dfa4
Remove unused test helper getBoundingClientRectRelativeToVisualViewport(). r=hiro
https://hg.mozilla.org/integration/autoland/rev/6e90095f917a
Remove an unused call to getVisualViewportOffsetRelativeToLayoutViewport(). r=hiro
https://hg.mozilla.org/integration/autoland/rev/3283ab499d5e
Add a getRelativeViewportOffset() helper. r=hiro
https://hg.mozilla.org/integration/autoland/rev/41f45dd8df15
Add a targetIsWindow() helper. r=hiro
https://hg.mozilla.org/integration/autoland/rev/211eb5edab37
Make promiseVerticalScrollbar[Touch]Drag() work on pinch-zoomed pages. r=hiro
https://hg.mozilla.org/integration/autoland/rev/b292bc72b171
Document what kind of coordinates hitTest() expects. r=hiro
https://hg.mozilla.org/integration/autoland/rev/7898f2f6d012
Add a mochitest. r=hiro
https://hg.mozilla.org/integration/autoland/rev/8f44aadcd59b
Invalidate the slider frame when the thumb is moved. r=tnikkel

Comment 33

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/fdf60665dfa4
https://hg.mozilla.org/mozilla-central/rev/6e90095f917a
https://hg.mozilla.org/mozilla-central/rev/3283ab499d5e
https://hg.mozilla.org/mozilla-central/rev/41f45dd8df15
https://hg.mozilla.org/mozilla-central/rev/211eb5edab37
https://hg.mozilla.org/mozilla-central/rev/b292bc72b171
https://hg.mozilla.org/mozilla-central/rev/7898f2f6d012
https://hg.mozilla.org/mozilla-central/rev/8f44aadcd59b

Comment 34

[Botond Ballo [:botond]]

Alexandru, could I ask you to test this one more time in the latest nightly (with the pref apz.scrollthumb.recalc at its default value of true), and confirm that you no longer see the issue?

Comment 36

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: scroll_1818721_0.gif

(In reply to Botond Ballo [:botond] from comment #34)

Alexandru, could I ask you to test this one more time in the latest nightly (with the pref apz.scrollthumb.recalc at its default value of true), and confirm that you no longer see the issue?

Hello! Looked at this today on the Windows 10x64 station where I could initially reproduce the bug by using Firefox 115.0a1 (20230516212859) and the spreadsheet from comment 0 and I can confirm that I can no longer see the issue when dragging the vertical/ horizontal scroll bar or when using autoscrolling on them. Thank you!

Comment 37

[Botond Ballo [:botond]]

Great, thanks for checking!

Comment 39

[Alexandru Trif, Desktop QA [:atrif]]

Verified fixed on Windows 10x64, Ubuntu 20, and macOS 12 by using the spreadsheet from comment 0 and the test case from bug 1826947 on macOS 12. The scrollbar moves smoothly when scrolling.