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

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: cpeterson, Assigned: kats)

Tracking

(Blocks 1 bug, {regression})

unspecified
mozilla65
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(geckoview62 disabled, firefox-esr60 disabled, firefox62 disabled, firefox63 disabled, firefox64 wontfix, firefox65 fixed)

Details

()

Attachments

(6 attachments, 1 obsolete attachment)

Reporter

Description

10 months ago
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.
Priority: -- → P2
Priority: P2 → P3
Priority: P3 → P2
Can repro, will investigate.
Assignee: nobody → kats
Posted file Reduced testcase (obsolete) —
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.
Argh. My patches fixed the reduced testcase but not the original site. :(
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.
Attachment #9014009 - Attachment is obsolete: true
Still not good enough! My updated patches fix the better reduced testcase but still not the original page.
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.
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.
QA Contact: mreavy
QA Contact: mreavy
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.
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
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 15

8 months ago
Pushed by kgupta@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.