StickySideConstraint needs documentation

[staktrace]

It appears that I've been laboring under a misunderstanding about how StickySideConstraint is supposed to work. Some documentation here would probably have saved me a couple of weeks of wasted time (and will save me from wasting more time). In particular, the way max_offset is used makes no sense to me. Here's my current understanding based on the code in calculate_sticky_offset:

This line:

sticky_offset.y = viewport_rect.min_y() + info.margin - sticky_rect.min_y();

seems to be computing the position the item would be in when "stuck", relative to sticky_rect's current y-offset. And this line:

sticky_offset.y = sticky_offset.y.max(0.0).min(info.max_offset);

then tries to clamp the item so that it doesn't go outside the container it's supposed to be inside. However it's not clear to me why the min bound of the clamp range is 0 because the item might have a normal position somewhere further down inside the sticky container.

Looking at the "bottom" case we have:

                sticky_offset.y = (viewport_rect.max_y() - info.margin) -
                    (sticky_offset.y + sticky_rect.min_y() + sticky_rect.size.height);

which is a roundabout way of writing sticky_offset.y = (viewport_rect.max_y() - info.margin - sticky_rect.size.height) - sticky_rect.min_y() which is again the position the item would be in when "stuck", relative to sticky_rect's current y-offset. But then once we've done that, the clamping should be exactly the same as in the previous case, but for some reason it's inverted:

sticky_offset.y = sticky_offset.y.min(0.0).max(info.max_offset);

and this makes no sense to me. I must be misunderstanding something but I can't find an interpretation of max_offset that makes this code make any sense.

@mrobinson can you please document this API so that users can build a model of what's supposed to happen and what needs to be passed in? Thanks!

[mrobinson]

@staktrace Thanks for the feedback and sorry for the pain! My plan for today is to:

1. Rework the API so the arguments are clearer. In particular I think it's better to think of the two max_offsets as the "minimum negative offset" and the "maximum positive offset". These offsets define the edges of the sticky constraint rectangle relative to the sticky rect. The "bottom" and "top" terminology just makes it very confusing.
2. Document the new API very well.
3. Add comments to this section of the code.

[staktrace]

@mrobinson Thanks! I was also thinking about this some more yesterday and I realized that we need to expose the 0 value as part of the API. The reason is that when gecko generates the display list it can do so at a scroll offset that's not zero. In those cases, the "top" sticky item for example might need to allow a negative offset in order for the item to positioned properly during async scrolling. A more concrete example:

• item is position:sticky with top:10px. if the scroll container is at a zero scroll position, the sticky item has y=20 inside the container. (i.e. the stickiness will take effect after 10px of scrolling). let's say the max_offset is 100px. so what gecko passes to WR is { margin: 10, max_offset: 100 }.
• now let's say the gecko scroll offset is 20px. and the item has already has a sticky offset of 10px applied to it. what gecko really wants to tell WR is { margin: 10, min_offset: -10, max_offset: 90 } because if we async scroll back to the top of the scroll container, WR has to apply a sticky offset of -10 and if we scroll all the way down WR has to apply an offset of +90. However the API doesn't allow this, but assumes the minimum bound is always 0.

I think this is mostly an artifact of how we did the integration, because WR internally assumes all the inputs are with zero scroll offsets but in gecko we can't actually do that easily, so we draw things with scroll offsets applied. However async scrolling can still happen and make the scroll position "negative". If you recall there was some other place where we were clamping the scroll offset in WR that needed to be taken out to solve this same class of problems. I think the same thing needs to be done to the sticky API.

[staktrace]

So in addition to any naming/documentation I think the API needs to change like so: staktrace@866096d

[mrobinson]

@staktrace I guess we can close this now?

[staktrace]

Yup!