1410527 - Fix layout/reftests/sticky-position/right-1.html (and much improve position:sticky support)

Status: RESOLVED FIXED

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

Description

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

I was investigating why right-1.html wasn't working and I came to a new level of understanding of the sticky positioning Gecko code. I wrote a patch which removes much of the ugly stuff in nsDisplayStickyPosition::CreateWebRenderCommands and fixes more tests than it regresses, so that's good. It also fixes the remaining problem in bug 1409508 (i.e. what's left after the WIP on that bug is applied).

A couple of the problems I found:
- The scrollPort obtained at [1] is relative to the scrollframe, and then we later do math on it with the itemBounds, which is relative to the ReferenceFrame(). So that's a coordinate space mismatch which is a problem
- Code like [2] was written with the assumption e.g. inner.YMost() >= 0 which might not necessarily be the case. In fact we can only assume inner.YMost() < outer.YMost() but they can be on either side of 0

[1] http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/layout/painting/nsDisplayList.cpp#7031
[2] http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/layout/painting/nsDisplayList.cpp#7044

Comment 1

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

Attachment: Bug 1410527 - Update how we tell WR about position:sticky elements.

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8920696 [details]
Bug 1410527 - Update how we tell WR about position:sticky elements.

https://reviewboard.mozilla.org/r/191694/#review196918


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/painting/nsDisplayList.cpp:7008
(Diff revision 1)
>  
> +static nscoord DistanceToRange(nscoord min, nscoord max) {
> +  MOZ_ASSERT(min <= max);
> +  if (max < 0) {
> +    return max;
> +  } else if (min > 0) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 3

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

I have an updated WIP that fixes the rest of the position-sticky tests.

Comment 4

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

Comment on attachment 8920696 [details]
Bug 1410527 - Update how we tell WR about position:sticky elements.

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

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8920696 [details]
Bug 1410527 - Update how we tell WR about position:sticky elements.

https://reviewboard.mozilla.org/r/191694/#review197314


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/painting/nsDisplayList.cpp:7272
(Diff revision 2)
>  
> +static nscoord DistanceToRange(nscoord min, nscoord max) {
> +  MOZ_ASSERT(min <= max);
> +  if (max < 0) {
> +    return max;
> +  } else if (min > 0) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 6

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

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

Comment 7

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

Comment on attachment 8920696 [details]
Bug 1410527 - Update how we tell WR about position:sticky elements.

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

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8920696 [details]
Bug 1410527 - Update how we tell WR about position:sticky elements.

https://reviewboard.mozilla.org/r/191694/#review198274

This is a bit hard to follow, but I think I understand most of it now, and everything that I've understood seems correct. And the passing tests speak for themselves.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8920696 [details]
Bug 1410527 - Update how we tell WR about position:sticky elements.

https://reviewboard.mozilla.org/r/191694/#review198278

::: layout/painting/nsDisplayList.cpp:7331
(Diff revision 3)
>      // SetStickyPositionData in Layers.h.
>  
>      if (outer.YMost() != inner.YMost()) {
>        // Question: How far will itemBounds.y be from the top of the scrollport
> -      // when we have scrolled down from the current scroll position of "0" to a
> -      // scroll position of "inner.YMost()" (which is >= 0 since we are
> +      // when we have scrolled from the current scroll position of "0" to
> +      // reach the range [outer.YMost(), inner.YMost()] where the item gets

reverse range

Comment 10

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

Comment on attachment 8920696 [details]
Bug 1410527 - Update how we tell WR about position:sticky elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191694/diff/3-4/

Comment 11

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

(In reply to Markus Stange [:mstange] from comment #9)
> reverse range

Fixed. I also reworded the second Question as we discussed on IRC and decided to leave it in for now. When the API change lands I'll rewrite this comment (hopefully it won't need as much commenting then).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/184c3c90cf23
Update how we tell WR about position:sticky elements. r=mstange

Comment 13

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/184c3c90cf23