Gecko sticky positioning code creates sticky margins that it shouldn't

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Other Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

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

Attachments

(1 attachment)

The gecko sticky positioning code is "mind-bendingly clever" as Markus put it, because it tries to use the difference between two rects to represent sticky ranges. It also initializes the rects using nscoord's min and max values [1] in what I can only assume is a clever attempt to avoid overflow.

Unfortunately, it turns out that the code has a slight off-by-one problem, because the YMost() of this rect (if unchanged) is (nscoord_MIN/2 + nscoord_MAX). However, the test uses nscoord_MAX/2 which is almost, but not quite, the same thing.

This results in bottom/right margins getting set on elements that shouldn't have them, and in the conversion to the webrender API these margins gets turned into Very Large Numbers which then cause problems. In particular, with the WIP series from bug 1411627 applied, scrolling down on a github review page like [3] results in an assertion failure which crashes a debug build.

[1] http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/layout/generic/StickyScrollContainer.cpp#160
[2] http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/layout/generic/StickyScrollContainer.cpp#288
[3] https://github.com/servo/webrender/pull/1969/commits/1c50cc04367171cc1e260d467b6a62cce5ca8325
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp] [gfx-noted]
Comment on attachment 8924289 [details]
Bug 1413666 - Fix off-by-one error in sticky positioning code.

https://reviewboard.mozilla.org/r/195506/#review200722

You may want to make the bug title more specific - after all, there may be other ways in which the Gecko sticky positioning code is too clever for its own good :p
Attachment #8924289 - Flags: review?(botond) → review+
Fine..  :p
Summary: Gecko sticky positioning code is too clever for its own good → Gecko sticky positioning code creates sticky margins that it shouldn't
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0540b49a6244
Fix off-by-one error in sticky positioning code. r=botond
Guilty as charged :)
Some of that turned out a bit hairy, particularly on the "scroll ranges" side (vs. positioning during reflow).

I believe the MAX/MIN values only serve as infinities, for the cases where the frame is not sticky in a given direction or the containing block doesn't factor in. Since the rectangles don't leave StickyScrollContainer, it probably wouldn't be too hard to refactor to use more explicit flags for those cases.
https://hg.mozilla.org/mozilla-central/rev/0540b49a6244
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Corey Ford [:coyotebush] from comment #5)
> Guilty as charged :)
> Some of that turned out a bit hairy, particularly on the "scroll ranges"
> side (vs. positioning during reflow).

Yeah, that's the part I mostly ended up dealing with, for APZ and now WebRender :)

> I believe the MAX/MIN values only serve as infinities, for the cases where
> the frame is not sticky in a given direction or the containing block doesn't
> factor in. Since the rectangles don't leave StickyScrollContainer, it
> probably wouldn't be too hard to refactor to use more explicit flags for
> those cases.

Well, there are inner/outer rectangles on the layer as well, but you're right that it probably wouldn't be too hard to refactor that and use more explicit flags. It's fairly self-contained which is nice.
You need to log in before you can comment on or make changes to this bug.