1490393 - Trello scrollbar jumps to the wrong position when dragged with WebRender enabled

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Enable gfx.webrender.all
2. Open a Trello board, such as https://trello.com/b/TTAVI7Ny/ue4-roadmap
3. Click and drag the "Future Releases" column's scrollbar

EXPECTED RESULT:
The scrollbar position should smoothly follow the mouse pointer.

ACTUAL RESULT:
The scrollbar position will jump down below the mouse pointer position.

I see this bug on both Windows 10 and macOS. Bisecting with mozregression took me way back to bug 1423370 in Nightly 61, which was also the source of (fixed) Google Docs scrollbar bug 1450871. I use Trello a lot, so I'm surprised I didn't notice this bug earlier.

Comment 1

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

Can repro, will investigate.

Comment 2

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

Attachment: Reduced testcase

Comment 3

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

I think what's happening here is that we have two nsDisplayTransform items, one nested inside the other, but with the same ASR. The outer one has a nonempty transform and the inner one has an identity transform. When walking the display list, we "defer" the outer nsDisplayTransform (by stashing it in StackingContextHelper::mDeferredTransformItem) so that child items will incorporate the transform as needed. However, when we encounter the nested nsDisplayTransform, we defer that one too, and so the earlier deferred one is effectively lost. Instead in this situation we need to combine the transforms.

Doing this might entail storing just the ASR and transform from the nsDisplayTransform item on the StackingContextHelper instead of a pointer to the nsDisplayTransform itself, because we'll want an easy way to combine transforms. I'll try implementing that.

Comment 4

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

Argh. My patches fixed the reduced testcase but not the original site. :(

Comment 5

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

Attachment: Better reduced testcase

Here's a reduction that's slightly closer to the original page since my patches don't fix it. I think there can basically be an arbitrary number of StackingContextHelpers between the one with the deferred transform and the one that we care about, so I need to extend my solution accordingly.

Comment 6

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

Still not good enough! My updated patches fix the better reduced testcase but still not the original page.

Comment 7

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

Attachment: A different reduced testcase

This one still fails with my latest fixes. I want to keep attachment 9014118 [details] since this reduction is not a strict superset of that one, so it's possible my fix for this will break that and it would be good to turn both into regression tests.

Comment 8

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

Finally got to the bottom of it. The patch to fix attachment 9014158 [details] also fixes the original site. WIPs are at https://github.com/staktrace/gecko-dev/commits/trello, it'll take some cleaning up and documenting before they're ready for review.

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&bugfiler=&group_state=expanded&revision=62942980d57eae3190ba6517da1219824f3f3e6b

Comment 10

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

Attachment: Bug 1490393 - Improve documentation regarding the deferred transforms. r?mstange

This code will be expanded a bit in the next few patches, so hopefully
the comments here provide a reasonable explanation of what this code is
about.

Comment 11

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

Attachment: Bug 1490393 - Extract a helper method on StackingContextHelper to get the deferred transform matrix. r?mstange

Depends on D8108

Comment 12

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

Attachment: Bug 1490393 - Accumulate the deferred transform down the StackingContextHelper chain if the ASR matches. r?mstange

The implementation of deferred transforms did not handle the case where
we ended up deferring multiple transform items before encountering the
APZ-relevant display item. In this case we need to somehow accumulate
all the deferred transforms. This patch accomplishes that, and includes
a mochitest that exercises the relevant behaviour.

Depends on D8109

Comment 13

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

Attachment: Bug 1490393 - Flush a deferred transform before picking up another if the ASR changes. r?mstange

The implementation of deferred transforms did not handle the case where
we ended up deferring multiple transform items in a row with different
ASRs. In this case, when we encounter the nested transform item that we
want to defer, we need to flush the previously-deferred transform item
into a WebRenderLayerScrollData item. This patch accomplishes that, and
includes a mochitest that exercises the relevant behaviour.

Depends on D8110

Comment 14

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

Here's a try push with the patches rebased and Markus' comments addressed:

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=b16532a2d52bb1307ff96ad731fe2ae9f0d9a404

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3bdc9c20e2a
Improve documentation regarding the deferred transforms. r=mstange
https://hg.mozilla.org/integration/autoland/rev/992aeaea02b6
Extract a helper method on StackingContextHelper to get the deferred transform matrix. r=mstange
https://hg.mozilla.org/integration/autoland/rev/b1e8ab8ec051
Accumulate the deferred transform down the StackingContextHelper chain if the ASR matches. r=mstange
https://hg.mozilla.org/integration/autoland/rev/0b39b9d22436
Flush a deferred transform before picking up another if the ASR changes. r=mstange

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b3bdc9c20e2a
https://hg.mozilla.org/mozilla-central/rev/992aeaea02b6
https://hg.mozilla.org/mozilla-central/rev/b1e8ab8ec051
https://hg.mozilla.org/mozilla-central/rev/0b39b9d22436