1411627 - WR API for position:sticky needs to support painting at nonzero scroll offsets

Status: RESOLVED FIXED

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

Description

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

Right now the WR implementation for position:sticky assumes that all the scrollframes are at zero offsets when the display list is generated. This means, for instance, if something is sticky with a top margin, then async scrolling can only ever move that item downwards. Therefore WR does things like [1] where it bakes in a max(0, ...) to prevent negative adjustments.

However, in reality we send display lists to gecko with scrollframes at nonzero scroll offsets. So if we async scroll to a smaller scroll offset, we tell WR to scroll to what it considers a negative scroll offset (because it's negative relative to the display list that we painted at). This means that items that are sticky with a top margin actually can move upwards with async scrolling, and the WR code doesn't allow that.

I believe the way to fix this is not hard-code the 0 value into the code at [1] but instead have the caller provide it via the API just like it provides max_offset.

[1] http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/gfx/webrender/src/clip_scroll_node.rs#449

Comment 1

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

WIPs that I believe to be functionally complete (but need documentation/cleanup):

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

Comment 2

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

I cleaned up the patches and added another reftest. Here's a final try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12867e3d82bc3db7418108ad848f22d647ee3a6c

I submitted the WR patch upstream as servo/webrender#1977. Once that is merged I'll put the rest of the patches up for review.

Comment 3

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

Attachment: Bug 1411627 - Stop sending sticky clips to WR for scrollframes with no ASR.

If the scrollframe has no ASR and is not async scrollable, WR will never
need to adjust the position of the sticky item. So we don't actually
need to send this data to WR. In theory this would be harmless but the
later patches in this bug actually expose another coordinate space
mismatch between the gecko and WR sticky-position code, so this patch is
needed to avoid the latent problem from manifesting to the user.

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

Comment 4

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

Attachment: Bug 1411627 - Send the applied offset for sticky frames to WR.

There are cases where we do a main-thread paint at a scroll position
where sticky offsets have been applied in order to keep sticky items
visually unmoving. From that paint, it's possible to do an async-scroll
in the direction that reduces the sticky offset. In order for WR to
handle this case properly we need to tell WR how much of a sticky offset
was already applied on the main thread.

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

Comment 5

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

Attachment: Bug 1411627 - Add reftests.

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

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8925959 [details]
Bug 1411627 - Stop sending sticky clips to WR for scrollframes with no ASR.

https://reviewboard.mozilla.org/r/197178/#review202400

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8925960 [details]
Bug 1411627 - Send the applied offset for sticky frames to WR.

https://reviewboard.mozilla.org/r/197180/#review202402

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8925961 [details]
Bug 1411627 - Add reftests.

https://reviewboard.mozilla.org/r/197182/#review202404

Comment 9

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

Updated try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=162ca08881d0200570a325e673990432c8ced927 is green also.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a165d1dcc904
Stop sending sticky clips to WR for scrollframes with no ASR. r=mstange
https://hg.mozilla.org/integration/autoland/rev/857a0e87e490
Send the applied offset for sticky frames to WR. r=mstange
https://hg.mozilla.org/integration/autoland/rev/6ca404d46fc2
Add reftests. r=mstange

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a165d1dcc904
https://hg.mozilla.org/mozilla-central/rev/857a0e87e490
https://hg.mozilla.org/mozilla-central/rev/6ca404d46fc2