1425565 - scrolling on github review of a PR certain elements don't scroll with the rest of the page, position:sticky not working

Status: RESOLVED FIXED

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

Description

[David Burns :automatedtester]

STR:

1) Go to a Github PR that you can review
2) Click on the Files changed tab
3) click on review changes. This will display a new div
4) scroll quickly up and down the page certain elements have a lag in scrolling on the page

Comment 1

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

I can repro; click on the URL, click on the "Review Changes" button to get the little dialog showing, and then scroll up and down. The sticky header is misplaced when you scroll into areas where a sticky adjustment would have taken place. Note that this doesn't happen when the dialog is not showing.

Based on the symptoms I suspect it's the same problem as bug 1424686, and will need the updated clipping API to fix.

Comment 3

[Markus Stange [:mstange]]

Attachment: reduced testcase

On Github there's a position:fixed element inside of the position:sticky element which causes breakage.

Comment 5

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

I'll look at this at some point

Comment 6

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

The presence of the fixed-pos item inside the sticky-pos item seems to change the ASR for the sticky item from being the root content scrollframe to just asr(), and that seems to result in the observed behaviour. I'm not sure why the ASR for the sticky item changes, but it seems wrong.

Comment 7

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

https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/layout/generic/nsFrame.cpp#3438-3441

The comment there seems to explain why this is the case.

Comment 8

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

Kind of a hacky fix, but seems to work on the test case:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=702be18fcc1c8253299515155cad5fbb7644984f

Comment 9

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

Attachment: Bug 1425565 - Use the container ASR for sticky items when building WebRender clips.

Even if the sticky item has a fixed descendant, we want to use the
sticky container item's "real" ASR when computing the WR clips because
otherwise the WR item doesn't get attached to the correct scrolling
layer.

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

Comment 10

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

Attachment: Bug 1425565 - Add a reftest.

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

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8974953 [details]
Bug 1425565 - Use the container ASR for sticky items when building WebRender clips.

https://reviewboard.mozilla.org/r/243344/#review251166

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8974954 [details]
Bug 1425565 - Add a reftest.

https://reviewboard.mozilla.org/r/243346/#review251168

Comment 13

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

Comment on attachment 8974953 [details]
Bug 1425565 - Use the container ASR for sticky items when building WebRender clips.

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

Comment 14

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

Comment on attachment 8974954 [details]
Bug 1425565 - Add a reftest.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a2b4256407a
Use the container ASR for sticky items when building WebRender clips. r=mstange
https://hg.mozilla.org/integration/autoland/rev/e9f8e4d5aef7
Add a reftest. r=mstange

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2a2b4256407a
https://hg.mozilla.org/mozilla-central/rev/e9f8e4d5aef7