Implement position: sticky - #887

Read through the whole patch — clean design, and several details most implementations get wrong are right here:

• scrolls() treating overflow: hidden as a scroll container but overflow: clip as not — clip is not scrollable per spec, so sticky under a clip ancestor correctly ignores it.
• Resolving sticky inset percentages against the scrollport, and extending the containing-block clamp to the scrollable-overflow area when the containing block is the scroll container — both mirror Chromium's StickyPositionConstraint / sticky_constraining_rect, which is exactly what makes the sticky-<th>-in-a-tall-table case behave like Chrome.
• End-inset-applied-then-start-inset (start wins when overconstrained), including the writing-mode/direction winner selection — matches CSS Position 3 §3.5 and the WPT overconstrained expectations.
• Keeping absolute_position (scrollIntoView targeting) on the in-flow position while gBCR/offsetTop report the drawn position is the right split.

Two real bugs to check:

1. Sticky inside a position: fixed subtree slides with page scroll. The viewport scrollport is viewport_scroll-based and the container walk never stops at a fixed (or transformed/filter) ancestor — but contents of a fixed subtree don't move with scroll. Repro: position: fixed panel containing a position: sticky; top: 0 header, then scroll the page. The header's flow position inside the panel is constant while viewport_scroll.y grows, so axis_offset shifts the header down by the scroll delta inside the panel. Blink/WebKit stop constraint computation there (sticky inside fixed behaves like relative). Suggest: when the container walk hits a position: fixed (or transformed) ancestor, stop and return shift 0 — or treat that ancestor as a terminal zero-scroll container.

2. Nested scrollers: the sticky view rectangle should be the intersection of the scrollports of all scroll containers between the sticky box and its containing block (CSS Position 3 §3.5), not just the nearest scroller's. Identical with one scroller; diverges when the containing block is outside the inner scroller (page + inner scrollable + sticky whose CB spans past it). The walk in sticky_offset_for already passes those ancestors — intersecting the scrollport rect while climbing when scrolls(anc.overflow) would close it in a few lines.

Minor notes:

• has_box() is size > 0 || size > 0, so a zero-size sticky element (empty width:0;height:0 box) goes down the boxless table-part path, finds no cells, and silently never sticks. Obscure, but a table-part flag from layout instead of a size heuristic would be exact.
• update_sticky_offsets() walks the whole tree twice on every scroll write and every paint. Most documents have zero sticky boxes — a sticky count (or seen-sticky bool) from resolve() would let it early-out and keep the per-scroll cost O(1) for the common case. Worth it if this is heading into production scrolling paths.
• The insets-to-AUTO neutralization exists in both to_taffy_style and the CoreStyle::inset wrapper — if those are two live build paths for the same style, fine (defensive), but worth confirming both are actually consulted; if one is dead, regressing it later would silently reintroduce the permanent-shift bug.

@yinnho Thanks again for the feedback.

I think I fixed what your saying, Well at least I did some testing it seem very fine, at least good enought for a merge.

If it is I will be confident enought work on Shadow DOM and text-overflow this week.

Re-reviewed b851c33 against Chrome 152 (headless):

• Fixed-scroller fix confirmed. Fixed panel at top:100 with an inner position:sticky;top:0 header — Chrome keeps the header at top=100 while the page scrolls to 600 (the pre-fix math reported 700, i.e. the header drifted with the page). The container_below_cb scrollport intersection matches that behavior.
• Nested-scroller view rectangle fix confirmed by reading; the origin-accumulating walk now equals "intersection of the scrollports of scrollers between the box and its containing block", and outer scrollports no longer shrink a box whose nearest scroller sits above its containing block.
• One regression I can reproduce: the container walk now also stops at transformed ancestors (|| anc.transform().is_some()). A transform is not a scroll container — the wrapper itself moves with page scroll, so sticky inside it should still resolve against the real scrollport. Chrome probe:

<body style="margin:0">
<div style="height:50px"></div>
<div style="transform:translateZ(0)">
  <div style="position:sticky;top:0;height:40px">sticky</div>
  <div style="height:1200px"></div>
</div>
</body>

window.scrollTo(0,600) → header getBoundingClientRect().top === 0 (pinned; docY = 600, not 50). With the walk terminating at the wrapper, the scrollport becomes the wrapper's static padding box, the sticky offset stays 0, and the header scrolls away like a normal box. Dropping the transform arm (keeping position:fixed, which the fixed-panel case above shows is genuinely needed) should restore parity — transformed ancestors then fold into the coordinate mapping like any other in-flow ancestor.

The rest (sticky_nodes caching, is_boxless_table_part, the NaN sentinel) looks right to me.

You are right, easy fix.
Rustfmt should pass too.

Some performance concerns with this (this does a full tree walk every frame to find sticky nodes!)

• We should split "constraints" from "offset". Everything in sticky_offset_for except the scroll positions is layout-invariant: scroller id, containing-block rect, in-flow rect, resolved insets, overconstraint winner. Compute that once in resolve() into a per-node struct; per-scroll then reduces to a handful of clamps in axis_offset. Currently every refresh re-walks the ancestor chain (twice, plus container_below_cb), borrows styles, and re-resolves percentages per sticky node.
• We should maintain a list of position:sticky nodes (probably during style traversal). Only iterate that list (ensuring to correctly handle nested sticky nodes, and DOM mutations they may remove nodes, etc).
• refresh_sticky_offsets allocates a Vec every call; hand_down_shift allocates per boxless child. Minor, but avoidable (reuse a scratch vec; iterate children directly).
• update_sticky_offsets sits after timer.print_times in resolve(), so it's invisible to log-phase-times. Move it above.

Correctness:

• text inside a sticky header will have its caret/selection drawn at the in-flow position.

Verified cb7ca14 against the Chrome probe from my last comment — the walk now stops at the nearest scroll container or fixed ancestor only, which matches: sticky inside a translateZ(0) wrapper pins to the real scrollport (scrollTo(0,600) → getBoundingClientRect().top === 0). And the sticky_nodes list answers the every-frame full-tree walk.

On the caret/selection item: we hit the same read/write split when we made transform reflect in geometry (getBoundingClientRect, hit testing). The pattern that survived there was a single read funnel — one per-node geometry accessor that composes the paint-time offset/transform, with every consumer (text queries, caret anchor, selection rects, paint) going through it, so nothing can drift out of sync again. If LayoutData::sticky_offset gets the same treatment transform() already has in blitz, the caret inherits the shift for free instead of becoming a special case.

Read the new "Single geometry funnel" commit — this is the right consolidation. Verified all five refactored sites (hit_inner, absolute_position, unrounded_absolute_position, the offset_* accumulation loop, paint box_position) are algebra-identical to the old inline location + sticky form, so it is behavior-neutral as intended.

Two details worth calling out as Chrome-correct: keeping the rounded/unrounded split (paint/hit on final_layout, CSSOM on unrounded_layout), and the doc comment pinning scrollIntoView as the only consumer that wants the in-flow location. offsetTop including the sticky shift also matches Chrome — a stuck header's offsetTop tracks scroll there.

Still holding my approval on the pair from the first review (the sticky walk not stopping at a position:fixed ancestor; nested scrollports needing the §3.5 intersection of all scrollports between the nearest scroller and the CB) plus the per-refresh full-tree walk cost — none of those are touched in this range yet.

Follow-up from our earlier review (we said we'd hold until the fixed-subtree and nested-scroller fixes landed): while waiting we shipped our own position:sticky in the HTML engine inside aginxbrowser (Rust, taffy for layout), so here's a data point from the other direction — what shipping it confirmed, and where it bit us.

Scope first, to keep the numbers honest: ours is narrower than this PR in places (per-parent z-band, no boxless table rows yet). Three commits, ~1.6k insertions including tests, 1,348-test suite green, clippy count unchanged.

v1, root scroller (+756): sticky lays out in-flow (taffy Relative, insets clamped to AUTO at the taffy boundary — same call this PR makes), and pins at read time via a shift map keyed by (layout epoch, layout rev, scroll, viewport). A scroll doesn't touch the tree at all — it just changes the map key, so scrolling is free and invalidation falls out of layout's existing epoch.

v2, element-level scrollers (+630): one unified depth-ascending walk computes per-node telescoping shifts (base + own + accumulated), so nested scrollers compose — this is bug #2 from our review, and we can confirm doing it as a single walk up front was much simpler than patching nearest-scroller-only afterwards. Fixed/transformed ancestors gate to zero shift (bug #1's fix shape).

One trap this PR will hit too: our first pass ordered the flat paint list per-parent, so a stuck header with z-index lost both paint and hit-test to a later static sibling — PositionMode::Sticky has to sit in the positioned set of the z-band partition (real-page repro: uv-docs' .md-header z:4 vs .md-container). It's the paint-side analogue of nicoburns' caret/selection point: every geometry consumer — caret, selection, gBCR, elementFromPoint — must go through the same shift funnel as paint, or the stuck element visibly tears.

And +1 on nicoburns' ①/②: our shift map is exactly the "compute constraints once per layout, clamp per scroll" shape. Keying by epoch meant we never had to maintain a sticky node list — the cache entry just dies when layout changes.

@yinnho
On the three held items: they landed in earlier commits of this PR, not in
the funnel range. position:fixed ancestors ending the walk and the §3.5
intersection of the scrollports between the nearest scroller and the
containing block are in b851c33 (with cb7ca14 dropping the transform stop
per your Chrome probe); the per-refresh cost is in 141195b — constraints
computed once per layout (StickyConstraints), a depth-sorted registry
maintained where styles are flushed to layout, and scroll/paint reduced to a
few clamps per box with no tree walk, style access or allocation. 5c82865
only consolidates the geometry reads on top of that.

Thanks for the aginxbrowser data point. The z-band trap is already covered
here: Blitz puts Position::Sticky in the same positioned band as Relative
for the paint-order partition (damage.rs) and treats it as a stacking
context like Fixed (node.rs), so a stuck z-index header keeps paint and
hit test over later static siblings. Your epoch-keyed shift map and this PR's
"constraints once per layout, clamp per scroll" are the same split; the
depth-sorted registry plays the role your epoch key does for invalidation.

Correction to my close-out comment — I inverted the transformed-ancestor case in the cb7ca14 bullet.

The patch drops the transform stop (fixed || anc.transform().is_some() becomes fixed), so the walk runs through a transformed ancestor: it moves with the page like any in-flow box and folds into the coordinate mapping, and the sticky keeps tracking the outer scroller — sticky inside a transform stays pinned relative to the viewport, which is what our Chrome probe showed. "Follows the ancestor, not the viewport" is the behavior the stop produced, and it's what the fixed-ancestor stop (kept in b851c33) correctly yields — fixed contents don't move with the page. That fixed/transformed asymmetry is the b851c33 → cb7ca14 split; I had it backwards.

Is this mergeable now, @nicoburns ? Keeping three PRs alive at once is starting to hurt :p

Is this mergeable now?

Not yet!

Remaining issues from AI review:

1. Bug: a sticky node removed and re-inserted stops being sticky. update_sticky_offsets retains away entries whose node is out of document, but doesn't clear IS_STICKY on the node. On re-append, note_sticky(id, true) sees registered == is_sticky and early-returns, so it's never re-registered. Reactive frameworks (Dioxus keyed lists) move nodes like this routinely. Fix: clear the flag in the retain closure (or at remove_node_from_tree), so the flag really mirrors registration.

2. Bug: walks parent, not layout_parent. constraints_for (and cells_extent/hand_down_shift) accumulate final_layout().location up the DOM parent chain, but locations are relative to the layout parent — anonymous blocks wrapping inline runs, and inline-embedded boxes whose layout_parent is the inline root (construct.rs:1024, :1220), not their <span>. Paint, absolute_position and hit testing all walk layout_parent; sticky must too or an inline-level sticky (span/inline-block inside text) gets a wrong position and a containing block taken from a span with no real box. Same fix for the containing-block lookup: use the layout parent's box.

3. Perf leftover: paint_scene still calls update_sticky_offsets, not refresh_*. That's now O(#sticky × depth) with style borrows and Vec allocs (sticky_ancestors, extra_scrollers) per frame — no longer a tree walk, but it's redundant since paint always follows resolve(), which already ran it. Should be refresh_sticky_offsets (and the doc comment on the PR/module says paint refreshes, so this is just a leftover).

And a couple of nits:

• NodeFlags::IS_STICKY = 0b00010000 skips 0b1000 for no reason;
• A node that goes display:none stays registered with stale constraints (harmless (not painted), but retain could also drop nodes with no display_style()))

Happy to post these as PR comments if you want.

Keeping three PRs alive at once is starting to hurt :p

Imagine how I feel trying to review 3 PRs at once (plus probably something like 20-100 PRs across Blitz and all of the other related repositories I maintain)! We're not a slop project, so everything needs to actually be reviewed (not just by AI - although I'm using AI review as a first line of defence to maintain my sanity) in order to maintain quality, and that takes a lot of time! Especially for LLM generated PRs

Your PRs are actually for high-priority features which is getting them a lot more attention than they would otherwise. But they're still non-trivial implementations, and I'm guessing by the speed at which you are responding to the feedback, mostly LLM generated (?) (which means that I don't have your own judgement or explanation of why you've implemented things the way you have to lean on, and have to carefully verify everything myself from scratch).

All three are fixed

Imagine how I feel trying to review 3 PRs at once

Well I feel you a lot, you are rights, comments in the code are not from me, I never documents code myself..
.html I use to test the feature are AI generated, I test everything manually to make sure every test pass, things show up like they are supposed too (glad I did) and verify that nothing break.

I'm fast, but not supe-human fast, beside some works I did on these matter before , AI chose to guide me on that speciffic path too, does it's resoning is better than me alone? most likely.. Would I forgot something without AI? yes. So as long as the code don't look stupid, and pass my test, it's fine on my side.

Take your time, I still have a lot of time too

A couple more:

• The new display_style().is_some() filter (sticky.rs:170-183) doesn't do what the comment says: display_style() returns Some(Display::None) for display: none, so it only excludes nodes without primary styles. Harmless (they aren't painted), but either check != Display::None or drop the comment's claim. sticky.rs:170-183
• dropped is a per-resolve Vec alloc. could clear the flag inside retain if it took &mut self.nodes via a split borrow. sticky.rs:173-189

Probably those'll be the last ones before human review.

Should be fine I hope.

@nicoburns I see a new conflict... I will probably fix it this week-end, but I'm wondering if these PR have lost any kind on momentum they had...

I'm currently sick, so don't have much capacity this week. My priority when I do is to land position: absolute/fixed hoisting support + vertical-align in Parley. I'm planning to return to your position: sticky and text-overflow PRs next after that.

Absolute/fixed hoisting landed which unblocks this (the vertical align stuff still needs to land, but that shouldn't conflict)

@nicoburns All conflits on 3 Prs are resolved.

I've pushed a small extra change which makes some more tests pass