feat(grid)!: replace frozen panes with pinning and sticky docking - #1302
ghiscoding wants to merge 10 commits into
Conversation
|
@6pac I think you should close your previous PR #1238 since this is the new approach that includes Pinning and Sticky. Please note that I would ask if you can ask Claude to audit and verify the entire PR to detect any possible problem, there's a progress file written by AI and read by AI to keep it focused, you should tell Claude to read that file Side note, with the code now you can at least start testing it out (including the new Also important, the +/- 1000px that we carried from the original SlickGrid gone in this PR, I'm pretty sure that was to support legacy IE browser back in the day but there's no reason to keep such old code and approach that caused alignment issues when implementing this PR and so I told the AI to remove it all which is a lot easier to the DOM now |
|
OK, Claude Fable is done with the evaluation. There's a lot of it! |
Evaluation of 6pac/SlickGrid PR #1302 — "feat(grid)!: replace frozen panes with pinning and sticky docking"
1. VerdictNot mergeable as it stands. The architecture is sound and the big-ticket claims (one viewport, one row per data row with left/centre/right regions, one horizontal scroll owner, Top issues (details in §4):
Nothing found requires abandoning the design. Most items are local fixes; the largest are the row-reference model (§4.1 group B) and the hit-testing/wheel routing for docked content (group D). 2. What was verified and how
Not done: no unit tests exist in this repository (the 3. Cypress results (local run, Windows)
The one failure is
Otherwise the suite is green locally, which matches CI. Note that green CI does not cover findings 1–7 above: the spreadsheet spec asserts the wrong pinned count, no spec drags a range over pinned cells, no spec uses non-contiguous row pins, 4. FindingsSeverity: Blocker = wrong behaviour for ordinary configurations or silent regression for existing users; High = wrong behaviour for documented pinning/sticky configurations; Medium = correctness edge cases, performance, API hygiene; Low/Nit = polish. Status: Confirmed (re-verified in source or browser), Observed (seen in the browser), Reasoned (reviewer, from code only). 4.1 Blockers and HighA. Column pinning referencesA1. Blocker — Numeric index references are also matched against A2. High — Numeric shorthands count hidden columns; B. Row references (pinned and sticky rows)B1. High — B2. High — The id→index cache is never invalidated on a count-preserving DataView sort/filter. Confirmed in B3. High — Custom DataView B4. High — Bottom-pinned rows keep their natural slot in the canvas. Reasoned ( B5. High — Non-contiguous top pins break hit-testing and active-cell tracking. Reasoned. Unpinned rows render at B6. High — Sticky-row thresholds ignore the permanent top band and subtract the bottom band twice. Confirmed in B7. High — B8. Medium — C. Regressions for grids that do not use pinning at allC1. Blocker — Every C2. Blocker — Vertical wheel now scrolls one row per notch on every grid. Confirmed by diff. C3. Blocker — Ctrl/Meta+drag multi-selection regressed. Confirmed by diff. Base C4. High — Undocumented rename C5. High — C6. High — Plain grids pay O(columns) per rendered cell in C7. Medium — Keyboard/focus contract changes not listed as breaking. Reasoned. Focus sinks moved outside the container with C8. Blocker — D. Interaction with docked contentD1. High — D2. High — Wheel over a pinned/sticky row scrolls the page. Reasoned. D3. High — Column reorder throws when a sticky column is docked (LTR proxy path). Reasoned. D4. High — Forwarded chrome D5. Medium — Docked rows outside the vertical rendered range never receive new centre cells on horizontal scroll, and in-range docked rows are never cell-cleaned. Reasoned ( D6. Medium — D7. Medium — Pinning cannot be switched off at runtime; the proxy scroller and chrome regions are created lazily but never removed. Confirmed by reading D8. Medium — Lazy docking activation empties header/header-row/footer without firing the D9. Medium — Cross-band colspan fragments freeze the host's E. Legacy surface and claimsE1. High — Legacy frozen options remain declared with live JSDoc; the Grid Menu still branches on them. Confirmed. E2. High — PR description and progress file claim things that do not exist in this repository. Confirmed by grep/diff.
E3. High — Test integrity. Confirmed by diff.
4.2 MediumM1. Performance on the per-scroll path. Reasoned by two reviewers (consistent with each other):
M2. M3. Public API drift not listed as breaking. Reasoned/confirmed by call-site diff:
M4. slickgrid-universal leakage into public types. Confirmed in the model diff. M5. Dead file that ships as an empty bundle. Confirmed. M6. Docked-row overlay artifact with zero-width scrollbars. Observed only in headless Chrome with scrollbars hidden (which is what overlay-scrollbar platforms such as macOS report): the last digit of each docked sticky row's rightmost cell is painted a second time, offset down-right, in the strip between the overlay clip and the grid border ( M7. Small controller/geometry issues. M8. Examples and docs. 4.3 Low / Nits
5. Verified sound
6. Recommended actions before merge
7. Reproducing the confirmed findingsAll steps use the repository's own scripts on a clean checkout of the PR branch (
|
Print Screens
|
|
Hang on a minute, there's quite a bit of stuff in there that's specific to my computer and its environment. I'm just gonna remove that and repost. |
|
OK the evaluation has been updated |
|
wow that is a lot.... providing this to Codex, and we'll see what it's able to fix. Just curious, do you also have access to Fable 5.1? Seems like an improvement, probably more expensive though Side note I also fixed colspan just now which can now spread on both side of the column pinning and also updated data Grouping which also spreads its grouping title (see above). |
|
Yep, this review was done with Fable 5.1. It did take up about 35% of my weekly quota though! Which is fine, I usually don't use more than about 30% of it anyway. |
| const queueMicrotaskPolyfill = (callback: () => void) => typeof queueMicrotask === 'function' ? queueMicrotask(callback) : setTimeout(callback, 0); | ||
| const destroyAllElementProps = (_target: object) => undefined; | ||
| const destroyAllElementProps = (target: object): void => { | ||
| const elementProperties = [ |
There was a problem hiding this comment.
not really sure why it added all of these, this seems very overkill. Shouldn't it be able to destroy and remove whatever it needs without us having to name all functions? I assume it came from Claude report
|
@6pac ok the AI is done with the audit report, the remaining things it said was basically verifying the UI myself... can you do a final audit to make sure it fixed everything. Also, can you ask it to see if it there's any areas to decrease LOC (I usually ask the AI if it's the most minimalist it can do without regressing). I'm especially concerned about the comment I left just above, I don't understand the point of listing all function names to loop and and destroy (this seems ridiculous and not minimalist to do this way). If there's anything else, I'd prefer you let it fix the rest... having a different AI model to double-check is actually a very good exercise, this will be used for the next few years, so better be good :) ahh wait, last commit caused a few test failures, let me fix them 
|
|
@6pac ok I'm done and fixed Cypress failures, so would you mind addressing what I wrote above |
|
In the middle of a very busy workday, but I'll point Claude at it and see how it goes. The destroyAllElementProps issue looks like a reversion rather than something Claude suggested (it found an issue with that function, but that's not the suggestion it made), from what I can see. I'll query it. |
|
ah yeah it might have been my old code actually, but I think we can remove that or lighten it at least. Thanks. There's no rush on it, need to make sure that we cover all angles and that the UI/UX works for you :) |
|
Here's the eval. Let me know if you're happy to go ahead and I can get Fable to do the final commits. |
Re-evaluation of 6pac/SlickGrid PR #1302 after the audit-fix commits
1. SummaryThe fix commits are substantive, not cosmetic. Of the eight blocker/high items that were demonstrated live in the first report, seven are fixed and verified on the new build (column over-pinning, autoHeight band, wheel behaviour, Ctrl+drag selection, editor placement, row-reference matching and cache invalidation, docked-cell hit-testing). The frozen option surface, the dead re-export file, the three tautological quirk specs and the missing documentation are dealt with. Two items were "fixed" in a way that needs another pass (bottom-pinned rows, Still blocking, in my view:
Worth doing before merge but not blocking: the remaining universal-fork leakage, the unrequested keyboard/focus feature carried in from slickgrid-universal, and the LOC/comment reductions in §5. 2. What was re-run
3. Status of the first report's findingsLegend: Fixed (verified) = re-demonstrated on the new build; Fixed (code) = the diff addresses it, not executed; Partial; Open. Column references
Row references
Regressions for non-pinned grids
Docked-content interaction
Legacy surface, claims, tests
Medium items
4. Review of the fix commits themselves4.1
|
| # | What | Est. saving | Effect on perf / readability |
|---|---|---|---|
| 1 | Unrequested keyboard/focus feature ported from universal: focusHeaderRowFilter (33), focusHeaderMenuOrColumn (15), focusGridMenu (12), focusHeaderColumn, focusGridCell, focusElementWithoutBubbling, stopFullBubbling, getVisibleElements, handleContainerKeyDown (20) and the F6/Tab/Shift+Tab routing inside handleGridKeyDown (~20). The selectors they target (.slick-header-menu-button[tabIndex="0"], .slick-grid-menu-button[tabIndex="0"]) have no producer in this repo. |
−130 | None on perf; removes an undocumented behaviour change (C7). Ship it as its own PR with plugin support if wanted. |
| 2 | Universal leakage in §4.3 | −120 src, −20 models | None; removes dead branches and any types. |
| 3 | Narrative comments. Many new comments are debugging history ("placed right-pinned titles at that stale edge (for example 1537px for a 1637px proxy)", "The docking POC's one horizontal scrollbar…", 10-line justifications before one-line writes). Trim to intent-level comments. |
−200 to −300 | Improves readability; the file already has 2,343 comment lines. Keep the ones that explain a non-obvious invariant (proxy translation, overlay clip, row shift). |
| 4 | destroyAllElementProps → reflective method (§4.1) |
−45 | Safer. |
| 5 | applyDockingToColumnChrome (161 lines): the four branches (sticky-transform / centre / left / right) each set position/left/right/order/transform with slightly different values; a placeChrome(element, { position, left, right, order, transform, offset }) helper and building dockingChromeByColumn from getHeaderColumn(id) instead of querySelectorAll(...).find per column |
−50 | Also removes the O(n²) header lookup on every resize step. |
| 6 | setOptions pinning/stickyRows array replacement as a loop or Utils helper |
−30 | Neutral. |
| 7 | Revert the slick-state-default rename |
−10 src, −25 scss | Removes a breaking change; nothing in this repo keys on either class. |
| 8 | applyRowTopOffset (74): the rowspan metadata scan can be computed once per row at render time and stored on the cache entry instead of on every syncDockedRowContainers pass |
−20 | Faster vertical scrolling on row-docked grids. |
| 9 | Small unused/duplicate public methods: getColumnByIdx (0 callers), getColumnHeaderByIndex (alias of getColumnByIndex), removeCellCssStylesBatch (0 external callers), getTopPanels returning the same panel twice |
−30 | Smaller public surface. |
| 10 | updateRenderedColspanFragmentGeometry host lookup fallback (querySelectorAll + find) — the host is always in cellNodesByColumnIdx |
−6 | Neutral. |
Total: roughly 650–750 lines (about a third of the net growth) without touching the docking architecture, and items 5 and 8 are also performance improvements. What should not be cut: the region-routing code in appendRowHtml/appendCellHtml/createColumnHeaders, the DockingController, the overlay/proxy sync — that is the feature.
6. Cypress
| Specs | 68 (67 + new example11-autoheight.cy.ts) |
| Passing | 710 |
| Failing | 1 — example-plugin-headermenu.cy.ts › "…Feedback->ContactUs sub-menus…": level-2 sub-menu still opens dropleft on Windows (Electron); unchanged from the first report, where the base version of the spec passed on the same machine. Nothing in the fix commits touches header layout or getGridPosition().width, so this was expected |
| Pending | 1 (example-auto-scroll-when-dragging "MAX interval", skipped in base too) |
The new and reworked specs (example11-autoheight, the three quirk-pinning-* harnesses, the spreadsheet hidden-column case, example-grouping-esm pinning cases, example-colspan pinned-colspan cases) all pass and now assert the intended behaviour rather than echoing options.
7. Recommended next steps
- Fix bottom-pin reachability (B4) — extend the scroll range or shrink the scroll viewport by the band height; add a spec that scrolls to the end with
bottom: [N-1]and asserts row N−2 is fully visible, with and withoutenableAddRow. - Replace the
destroyAllElementPropslist with the reflective method (§4.1). - Replace the
elementFromPointfallback with a geometric inverse (B5/D1) so off-screen drag coordinates resolve correctly under non-contiguous pins. - Decide the colspan-over-pinning look: the probe shows the host does paint over scrolled centre cells (§4.2). Unless that is the intended AG-Grid-style behaviour for every cross-boundary colspan, clip the host to its band and let the fragment carry the text.
- Remove the universal leakage (§4.3) and, unless the focus feature is wanted now, the focus/keyboard routing (§5 item 1).
- Address the open Ds (D3, D4, D5, D8, D9) or list them explicitly as known limitations in
docs/pinning-sticky.md. - Expand
docs/pinning-sticky.mdinto a real migration section and remove the stale narrative from the progress file (or delete the file from the PR). - Reproduce the Windows header-menu alignment failure (§6) — it is deterministic here and passes on the base.
|
@6pac so I would prefer if you ask Claude to finish the rest, I'm out of token until Saturday anyway. Thanks |
|
Can do, might be worth looking at the '5. Can the PR lose lines without hurting performance or readability?' section especially 1,7,8 - they are all optional. Would be good for you to check. eg. 7, 8 maybe should be left for legacy purposes. |
2 participants
supersede #1238
fixes #410
fixes #443
fixes #739
fixes #1219
Summary
Introduce a single-viewport docking architecture for permanent pinned columns/rows and
scroll-activated sticky columns/rows.
This is an intentional v6 breaking change. The previous multi-pane frozen implementation has
been removed from the runtime and replaced with one virtualized body viewport, one vertical
scroll owner, one horizontal scroll owner, and stable per-row left/center/right cell regions.
Why
The legacy frozen-pane implementation required multiple synchronized panes and scroll
containers. This increased complexity around scrolling, resizing, virtualization, editing,
grouping, and framework integrations.
The new architecture provides a simpler and more predictable model:
Unlike the previous freeze-until-column/row behavior, users can now pin individual columns or
rows independently. For example, columns 0 and 2 can be pinned while column 1 remains in the
center region.
Changes
GridOption.pinningsupport for:columns.left/columns.right;rows.top/rows.bottom.Column.pinnedandCurrentColumn.pinningstate support.Column.stickyandGridOption.stickyRowsfor scroll-activated docking.DockingControllerfor permanent and sticky column/row resolution.conveyor/clampoverflow strategies.
fragments.
and framework integrations.
example-sticky-financial-report.htmlColumn.pinnablesupport for controlling Header Menu pinning commands.pinningshape.is intentionally not serialized.
.slick-horizontal-scrollerand.slick-vertical-scrollerselectors.scroll branches, redundant viewport/canvas aliases, and old pane CSS classes.
-1000pxheader coordinate workaround andHEADER_WIDTH_SLACK.pinning-stickyskill as implementation and documentation guidance.Breaking changes
The old frozen-pane configuration and APIs are removed.
The canonical configuration is now:
Legacy flat pinning options and temporary aliases are no longer supported.
Sticky state is not serialized because it changes with scrolling.
The old multi-pane DOM structure and pane selectors are no longer available.
Column reordering remains within each docking band; moving a column between pinned and center
bands is an explicit pinning operation.
Legacy names and theme variables are retained only as migration documentation references.
References
Ag-Grid Column Pinning was used as key concept reference for the idea of a single horizontal scroller and single vertical scroller, also for its declaration of left/center/right cell docking regions
Validation
The following checks pass:
git diff --check.pinning/sticky, resizing, reordering, RTL, variable row heights, editing, selection,
grouping, spans, and framework parity.
The accessibility audit found no pinning/sticky-specific semantic-tree or keyboard-navigation
regressions. Automated axe/WCAG integration and manual screen-reader validation are not included
in this PR.
Implementation status
The single-viewport rewrite and legacy runtime cleanup are complete. This is no longer a POC
that runs alongside the old frozen-pane implementation.
The approximate library-only production diff is:
+3,989 / -1,550;+2,439net LOC relative to the base commit.These figures exclude demos, tests, generated output, and framework-wrapper changes.
Follow-up work
The following items are intentionally separate from the v6 implementation:
None of these requires restoring the legacy pane architecture or changing the current pinning/sticky
runtime design.
AI / LLM assistance
documentation updates, test maintenance, and validation support.
Checklist
documentation, tests, and cleanup.