Skip to content

fix: canonicalize the frozen-row boundary across all comparison sites - #1268

Merged
6pac merged 2 commits into
masterfrom
fix/frozen-row-boundary-canonicalization
Aug 9, 2026
Merged

fix: canonicalize the frozen-row boundary across all comparison sites#1268
6pac merged 2 commits into
masterfrom
fix/frozen-row-boundary-canonicalization

Conversation

@6pac-ai

@6pac-ai 6pac-ai commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Merge order: this PR assumes #1266 and #1267 are merged first. It has no textual conflicts with either, but it was authored and validated against a master that includes them — please land those two before this one.

The bug (Q3 in discussion #1247)

The frozen-row boundary was compared differently at six sites, contradicting each other and the render split (rows >= actualFrozenRow render in the bottom canvas):

Site Drift Observable
appendRowHtml frozen class row <= frozenRow (a count compare) bottom mode: the first rows classed .frozen, the actually-pinned rows never; top mode: off-by-one
cleanupRows / cleanUpCells <= actualFrozenRow first scrollable row never evicted/cleaned in top mode
_getContainerElement (getCanvasNode/getViewportNode) >= actualFrozenRow + 1 in top mode returns the TOP pane for a row whose DOM lives in the BOTTOM canvas — overlays/menus positioned via the public API mis-target exactly the boundary row
scrollRowIntoView actualFrozenRow - 1 boundaries bottom mode: refuses to scroll the LAST scrollable row (navigateToPos, one screen away, already compares correctly)

The fix

Two predicates matching the render split — isBottomBandRow (row >= actualFrozenRow) and isFrozenRowIdx (frozenBottom ? row >= actualFrozenRow : row < actualFrozenRow) — with every site routed through them. Net −5 lines.

Downstream check (executed)

slickgrid-universal maintains its own fork of this grid (verified by clone + grep — it does not consume this file at runtime), so this cannot break it. Its fork carries the same isBottomSide off-by-one and may want to port the fix.

Test

cypress/e2e/quirk-frozen-row-boundary.cy.tsself-hosting (harness via cy.intercept; no example-page dependency), one check per drifted site plus a frozen-row-stays-cached control. Pre-fix it fails on every drifted site with the predicted signatures (frozen class [0,1,2,3] in both modes; getCanvasNode returning the top canvas while the row's DOM is in the bottom; scrollRowIntoView(996) leaving viewport.top=0; boundary row never evicted); post-fix all pass. Frozen-family suites including the merged #1255/#1258 quirk specs ran 27/27.

⚠️ Temporary example page

examples/example-quirk-frozen-row-boundary.html (tinted .frozen rows + pane-lookup/scroll readouts) is intended to be deleted before merge — the cypress test is fully independent of it.

(Quirks-triage remaining-items wave: see discussion #1247; siblings #1266, #1267.)

The row-band boundary was compared differently at six sites, contradicting
each other and the render split (rows >= actualFrozenRow render in the
bottom canvas):

- appendRowHtml classed rows 'frozen' via row <= frozenRow (a COUNT
  compare): in bottom mode the first frozenRow+1 scrollable rows were
  classed and the actually-pinned rows never were; off-by-one in top mode
- cleanupRows and cleanUpCells exempted <= actualFrozenRow, sparing the
  first SCROLLABLE row from cache eviction and cell cleanup in top mode
- _getContainerElement (getCanvasNode/getViewportNode) classified with
  >= actualFrozenRow + 1 in top mode, returning the TOP pane for a row
  whose DOM lives in the BOTTOM canvas (overlay/menu positioning via the
  public API mis-targeted exactly the boundary row)
- scrollRowIntoView used actualFrozenRow - 1 boundaries, refusing to
  scroll the LAST scrollable row in bottom mode (navigateToPos, one screen
  away, already used the correct comparison)

All sites now route through two predicates matching the render split:
isBottomBandRow (row >= actualFrozenRow) and isFrozenRowIdx (frozenBottom
? row >= actualFrozenRow : row < actualFrozenRow). Net -5 lines.

Downstream note: slickgrid-universal maintains its own fork of this grid
(verified by clone+grep - it does not consume this file at runtime), so
this cannot break it; its fork carries the same isBottomSide off-by-one
and may want to port the fix.

Adds a permanent SELF-HOSTING regression test (cypress/e2e/quirk-frozen-
row-boundary.cy.ts) with one check per drifted site. Verified pre-fix
failures on every site (frozen class [0,1,2,3] in BOTH modes; pane lookup
disagreement; scroll refusal; no eviction) and all-pass with the fix;
frozen-family suites incl. the merged quirk specs ran 27/27.

NOTE: examples/example-quirk-frozen-row-boundary.html is a TEMPORARY
human-review repro page (tinted .frozen rows + pane-lookup/scroll
readouts) intended to be deleted before merge - the cypress test does not
depend on it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ghiscoding

ghiscoding commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

I replicated this PR as well but it seems to cause a constant error in Cypress test example-auto-scroll-when-dragging.cy.ts which would probably need to be reviewed before merging

I asked copilot to summarize why it might be failing because I have the same example and test, and it replied with:

Likely CI flake, not grid logic bug.

Short version you can post:
The failure is probably Cypress actionability during drag start: the target cell is briefly covered in headless CI, so mousedown/mousemove gets blocked. The drag helper should be hardened by forcing mousedown + first mousemove, and ending with mouseup on body (then grid) to avoid stale drag state when pointer leaves the container. This fixed the same frozen-grid test on my side without changing assertions.

Side note, I actually had to modify my similar Cypress test as well (example17.cy.ts in my case), so you might want to ask Claude to replicate my Cypress changes in this PR as well to make the tests pass and avoid flakiness.
See my PR for reference: ghiscoding/slickgrid-universal#2706

Port of ghiscoding/slickgrid-universal#2706: force the mousedown and first
mousemove in dragStart (force was previously absent here) and release the
drag with a body mouseup in dragEnd. The frozen-grid auto-scroll test
failed constantly after the boundary canonicalization because the drag
target's coordinates now land where the example page's descriptive text
overlaps the grid under scrollBehavior:false; the assertions are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@6pac-ai

6pac-ai commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the pointer @ghiscoding — reproduced here too, and it was indeed failing constantly on this PR's CI (example-auto-scroll-when-dragging.cy.ts, frozen-grid drag test, ensureElIsNotCovered).

Root cause checks out as test actionability rather than grid logic: the covered target is a live grid cell, and the covering element is the example page's own descriptive text (<div>better to multiple of MIN</div>) — with scrollBehavior: false, the drag's target coordinates now land where the page text overlaps the grid, because the canonicalized boundary changes where the auto-scroll legitimately lands. The dedicated frozen-boundary spec (which asserts canvas routing and classification directly) passes in the same runs, so rows aren't being misrouted.

Ported your #2706 hardening to cypress/support/drag.ts in fce4bcb — force on the mousedown and first mousemove in dragStart (force was previously missing entirely here) plus the body mouseup in dragEnd, assertions unchanged. The failing test now passes 5/5 consecutive local runs and the full suite is green. (The example17 median-sampling change wasn't needed on this side — our equivalent MAX-interval test is already skipped.)

@6pac
6pac merged commit bd2b6b6 into master Aug 9, 2026
3 checks passed
@6pac
6pac deleted the fix/frozen-row-boundary-canonicalization branch August 9, 2026 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants