Closed Bug 1410527 Opened 4 years ago Closed 4 years ago

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

Categories

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

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(1 file)

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 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]
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp] [gfx-noted]
I have an updated WIP that fixes the rest of the position-sticky tests.
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]
Summary: Fix layout/reftests/sticky-position/right-1.html → Fix layout/reftests/sticky-position/right-1.html (and much improve position:sticky support)
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.
Attachment #8920696 - Flags: review?(mstange) → review+
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
(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).
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/184c3c90cf23
Update how we tell WR about position:sticky elements. r=mstange
https://hg.mozilla.org/mozilla-central/rev/184c3c90cf23
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1424686
You need to log in before you can comment on or make changes to this bug.