feat(grid)!: replace frozen panes with pinning and sticky docking - #1302
ghiscoding wants to merge 47 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 lib is officially gone in this PR, I'm pretty sure that it was in place 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 read 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 (
|
|
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. |
…next band A colspan starting in a pinned band was rendered as one host cell stretched to the full span width, given `overflow: visible` and `z-index: 21`. The host sits in the sticky pinned band, so it stayed put while the centre band scrolled and covered whatever passed beneath it: with a four-column span the Owner, Effort Driven and Region cells of that row were invisible at any non-zero scroll position, while the same cells were readable in every neighbouring row. Each piece of the span is now clipped to its own band. The host renders the part of the content that belongs to its band, and each continuation carries a presentational copy of the host's content shifted left by what the earlier bands already showed, so the text reads continuously across the boundary instead of restarting or being elided. The copy is `aria-hidden` and the host keeps the role, the value and the event wiring, so selection, navigation and formatters are unchanged. example-colspan.cy.ts asserted the old behaviour directly (the host's right edge had to lie beyond its band). It now asserts the host is clipped to the band, the continuation starts at the host's edge, its copy is aligned with the host's text, and a scrolling cell stays the topmost element under the pointer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…wport edge Dragging a centre column's resize handle past the right edge grew the column to about a viewport width and then stopped: the column froze, the grid never scrolled, and the handle sat outside the visible area with no way to continue. A centre column's cached coordinates are relative to the centre band, but the test that decides whether to scroll compared them against the whole scroll owner's clientWidth. The pinned bands' width therefore acted as dead room in which the column could grow past the edge without the grid following it. Since nothing scrolled, the auto-scroll interval's target never moved either, so each tick re-applied the same width and the drag stalled. The comparison now uses the visible width of the centre band. Measured on the pinning example, the scroll owner advances 0, 62, 254, 434, 590, 746 over two seconds while the column grows 80 to 993, and the column's trailing edge stays at the viewport edge throughout. Adds the resize auto-scroll case to example-pinning-columns-reorder.cy.ts; it was the one case from the deleted frozen reorder spec that could not be ported while this was broken. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…breaking The spreadsheet and colspan specs forced their clicks past the actionability check, and the spreadsheet spec's helper blamed a duplicate cell node left by docked virtualized rendering. There is no duplicate: sampling the DOM once per animation frame across the scroll never finds more than one node for the cell, and the node is the topmost element at its own centre. The runner scrolls a subject into view before clicking it. On a virtualized grid that scroll re-renders the row, detaching the element the test just resolved, and the live node that replaces it is then reported as "covering" the detached one, which is why both elements in the error looked identical. Passing scrollBehavior: false to the click, on a cell that is already in view, removes the cause instead of ignoring the symptom. The colspan fragment clicks no longer need forcing either, now that a span is clipped to its band rather than rendered on top of the next one. force: true across the suite goes from 150 to 141. The one use left in example-sticky-financial-report is genuine: a right-docked sticky column really does cover the natural cell beneath it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- internalScrollColumnIntoView() measured the scroll owner's border box and then subtracted the vertical scrollbar again. In proxy mode the docking scrollbar is already sized to the inner width (measured 583 against a 598 container with a 15px gutter), so the usable width came out 15px short and the grid scrolled when it did not need to. It now reads clientWidth, which excludes the scrollbar in both the proxy and the native scroll-owner modes. - viewportHasHScroll used `canvasWidth >= viewportW - scrollbarWidth` while the docking scrollbar decides its own visibility with `contentWidth > clientWidth`. Content that exactly filled the viewport therefore had room reserved for a scrollbar the proxy never showed. Both now make the same test. - getRightDockedChromeLeft() subtracted two getBoundingClientRect() values, which are screen pixels, from terms that are layout pixels; a CSS scale on any ancestor skewed the right-pinned chrome. The measured distance is converted back to layout pixels, which is identity for an unscaled grid. - validateColspanPinningSequence() only inspected rendered rows, so a colspan that a non-sequential pinning would split went unnoticed until it scrolled into view. It now scans every row that can carry metadata, stopping at the first match. Only a non-sequential request reaches that scan, and a data provider that exposes no length still falls back to the rendered rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ment Every horizontal scroll wrote --slick-docking-scroll-left onto both cell regions of every cached docked row, every active sticky cell, every full-width group cell and every pinned chrome element. All of those writes carried the same value, and custom properties inherit, so one write on the grid container reaches all of them. On a grid with 30 docked rows and a few pinned columns that is roughly 70 style writes per scroll event replaced by one. The property is refreshed by the proxy scroll pass and whenever the docking scrollbar is resized, so it is current before the first paint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The docking chrome pass called getComputedStyle() twice per column to read the padding and borders of the header-row and footer cells. Those come from the cell's classes, not from its column, so cells that look alike share one measurement; the pass now memoises it by class signature. No stylesheet rule selects a chrome cell by position, so the signature is a safe key. This is a reduction in style queries rather than a measured speedup: on a 211-column grid the pass times between 16 and 33ms across runs, which is too noisy to attribute a difference to. The pass is left running on every resize mousemove deliberately, because the pinned chrome has to track the column width while the drag is in progress. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…reach the docking root updateRenderedColspanFragmentGeometry() searched a row's DOM for the span host whenever the cell map had no entry for it. The map is only incomplete while a row's render queue is still pending, so the row's queue is drained first and the host is read from the map, which is what the fallback was standing in for. The auto header height rule sized .slick-header-columns-left and -right. Inside a docking chrome root both are display: contents and have no box, so the height went nowhere; it now also targets .slick-header-columns-root, which is the real element there. Plain grids are unaffected, since the left wrapper is a real box for them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Porting the pre-header column-picker case from the deleted frozen spec found a crash in the example itself: renderHeaderGroups() and syncPinnedGroupHeaders() both read getComputedStyle(grid.getHeaderColumn(0)), and that header is briefly absent while the columns are rebuilt. Hiding any column from the pre-header picker therefore threw. Both now fall back to the default background. The restored case also covers what the frozen spec asserted and the pinning one did not: the picker names each column by its group, and hiding the first pinned column leaves the remaining pinned columns consistent. Because columns.left is an inclusive boundary over the visible columns, hiding the first one moves Start into the pinned band, which the case now pins down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@6pac after replicating and pushing commits to my repo and give it a try, I can see that it's following the mockup provided above. The scroll does work as mentioned in the mockup but it added a cell border showing where the pinning crosses, but that is not what we would want, if I click a cell that crosses boundary it should be 1 clickable cell (the z-index 21 was hiding that fact before the recent changes). So it fixes a bug but also introduces a new unexpected regression 
|
|
@ghiscoding RTL is done — opened as #1306, stacked on This was the one item the audit rounds deliberately left open. A right-to-left grid created the docking scrollbar but The fix measures the docking geometry along the inline axis, which runs from the leading edge in both reading directions, and converts to physical pixels only where a style needs one. One thing worth your eye, because it affects LTR too: The bands stay named for the reading order rather than the screen, so one configuration describes both directions:
A stylesheet targeting New One trap that cost me some time and may catch users: a sticky column that never activates is usually the width budget rather than a layout bug. Pinned and sticky columns share |
|
@ghiscoding OK almost done, will deal with that last bug and request LOC reduction |
|
Also not sure if you've heard or not but both Claude and ChatGPT released new models today (Claude Opus 5.5 and ChatGPT 6 Sol/Luna), both of them decreased rates as well, so good news |
A colspan that crosses a docking boundary renders as a host plus a continuation, and both are cells, so both drew an edge where the two meet. The active-cell outline showed it in every theme: the rule that drops a shared edge matched only continuations, and the host is not one, so the host drew its trailing edge down the middle of the span. The theme's own column separator showed it wherever a theme draws one; every shipped colspan example uses alpine, which draws none. The piece whose right edge is shared with another piece of the same span no longer paints that separator, and the active outline drops the shared edge on any piece that is not the last. Both are direction-aware: the shared edge is the following piece's in a left-to-right grid and the preceding piece's in a right-to-left one. Measured on example-colspan with the stock separator restored, the host's border-right goes from 1px dotted silver to 1px dotted transparent and its active outline's trailing edge from 1px to 0, while the pieces stay where they were (host 102..202, continuation 202..402). The colour is dropped rather than the width so the geometry does not move. A click on either half already reported one cell; the continuation is cloned from the host and carries its column classes. getCellFromPoint still resolves to the column under the pointer, which is how it behaves for any colspan, pinned or not. The existing resize case asserted the seam as correct and is corrected here. Reported by @ghiscoding on #1302. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Docking has one horizontal scroll owner: the proxy scrollbar. The methods that positioned pinned rows and chrome by writing a transform per scroll event are from the design that preceded it, and each begins by returning when the proxy scrollbar exists. It always exists when it matters. Configured docking and the proxy scrollbar are introduced and removed together at three sites: activateSingleViewportLayout() on init, the lazy activation in setColumns(), and the teardown in setOptions(). So whenever a docked row or a docked chrome element exists the guard returns, and with no docking configured the loops iterate over an empty band list and a cache with no docked rows. Measured before removing: instrumenting the three methods to count only the invocations that would do work, then calling applyDockingScrollOffsets() directly in 25 states across five examples - as loaded, scrolled, pinning added, pinning removed, pinning re-added - the invariant held in every one and the work count was zero in every one, including states with 44 docked rows and three bands. quirk-docking-scroll-owner.cy.ts now guards that invariant. applyDockingScrollOffsetToRow() goes entirely rather than keeping its proxy half. That half cleared inline transforms on the cell regions and the removed tail was their only writer; the proxy stylesheet sets those transforms with !important, so an inline value never applied. This also leaves one copy of the chrome natural/docked offset geometry, which until now was written out in both placeDockedChromeElement() and applyDockingChromeScrollOffsets(). Suite: 78 specs, 731 passing, 0 failing, 1 pending. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Can the pinning/sticky PRs be made smaller?Evaluation of #1302 ( Status: items 1, 2 and 4 done. Item 3 was implemented, measured, and reverted. Item 5 is open. Correction to this document's first draft. It estimated items 3 and 4 would save about 26 What the feature costsMeasured against the merge base
About 2,900 lines, roughly 2,400 of them code. #1306 adds a further +135/−64 in Two things this ruled out before looking at any method:
The headroom was structural and modest. The realised figure is −71 lines, about 2.4% of the Done1. The non-proxy horizontal-scroll path was unreachable — 71 lines removed
That state cannot occur. Docking-configured and proxy-scroller-exists are held in lockstep in
Verified at runtime before removing anything. A probe instrumented the three methods to count A first attempt at this probe counted calls during scrolling and reported all zeros — but the call Removed:
2. The chrome offset formula existed twice — resolved by item 1
4. One rejection helper replaces three copies — line-neutralif ((forceAlert || !this._invalidPinningAlerted) && this._options.invalidColumnPinningXCallback) {
this._options.invalidColumnPinningXCallback(this._options.invalidColumnPinningXMessage!);
this._invalidPinningAlerted = true;
}
return false;This appeared three times in the validation methods, differing only in callback and message. It is Tried and reverted3. A placement descriptor for
|
| Attempt | Method | Plus interface | Against 73 |
|---|---|---|---|
| Resolve and write as two methods | 101 | +15 | +43 |
One method, if/else chain, single writer |
84 | +15 | +25 |
The two-method split repeats the eight-line parameter list, which costs more than the repetition
it removes. Even collapsed, a descriptor object literal per branch is longer than the sequence of
element.style.x = assignments it replaces, and the shared type has to be declared somewhere. The
original estimate of −18 counted only the repeated property names.
The invariant argument still stands, so this is worth revisiting if the method ever grows a fifth
branch — at which point the scaffolding is amortised over five branches instead of four.
Still open
5. resolveColumns() two-sided reconciliation — about 12 lines
The block that stops a sticky: 'both' column occupying both bands builds two Maps and then
splices each list by findIndex. Comparing the two distances while partitioning in one pass over
activeLeft would be shorter and would not need the maps. Left alone for now; worth doing only if
that method is being touched anyway. On the evidence of items 3 and 4, treat the 12 as optimistic
until measured.
Not worth doing
- Nesting the stylesheet. 29 top-level rules in
_slick-docking.scssbegin with
.slick-row-docked. Collapsing them into one nested block trades repeated selector text for
indentation and braces — close to line-neutral, and it makes a full selector no longer greppable. - Merging the symmetric sticky-activation loops in
resolveColumns(). The left loop walks
forward and the right one backward withunshift; one direction-parameterised loop is shorter
and materially harder to read. - Merging
validatePinnedColumnIndexes()andvalidateColspanPinningSequence(). They answer
different questions and the second is already called by the first. - The three-pass split in
applyDockingToColumnChrome(). It looks verbose but it is the
reason the pass forces one layout instead of one per column, and thegetComputedStyle()
memoisation is what removed two calls per column. This is performance, not ceremony. - The RTL PR. +135/−64 for the whole feature, and its three new helpers are each used at
several call sites (getInlineDirection()6,setInlinePosition()4,
getInlineOffsetFromLeft()2). Nothing to take out. - Dead types or options. Every type in
docking.interface.tsand every field of
DockingOptionis referenced; there is no unused surface to delete.
What each item actually cost
| Item | First estimate | Measured | Status |
|---|---|---|---|
| 1. Unreachable non-proxy scroll path | −55 | −71 | Done; runtime-verified across 25 states, invariant spec added |
| 2. Duplicated chrome offset formula | −10 | 0 | Done, subsumed by item 1 rather than separate |
3. placeDockedChromeElement descriptor |
−18 | +25 | Implemented, measured, reverted |
| 4. Pinning rejection helper | −8 | 0 | Done; triplication gone at no cost |
| 5. Two-sided reconciliation | −12 | not attempted | Open |
slick.grid.ts goes from +2,069 over base to +1,998, and the feature from about 2,900
lines to about 2,829.
The honest summary: one item was a real removal and the rest are readability. If the goal is
strictly fewer lines, item 1 is the whole story — item 3 worked against it and was reverted, and
items 2 and 4 cost nothing but buy nothing either in line terms.
Three validation sites wrote out the same alert-once block, differing only in which callback and message they used: alert when the caller forces it or the grid has not alerted yet, latch the flag, return false. rejectPinning() does that once and returns false, so each site is a single return. The file is the same length either way - the helper costs what the three sites give back - but the rule that a validation running on every render alerts only the first time now lives in one place rather than three. Suite: 78 specs, 731 passing, 0 failing, 1 pending. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
I ran similar audit with ChatGPT Astra and .... ran out of my 6 hours tokens, so I'll provide the final result tomorrow, but there's still potential. Below is what it wrote so far: Audit in Progress SummaryThe local branch includes the reported 71-line removal. I also found the two-sided reconciliation pattern in both the column and row resolvers, which gives us another candidate to measure. I’ll test temporary refactors outside the source tree so the audit can distinguish measured savings from estimates. There’s a stronger candidate in the scroll path: it still walks rendered rows and pinned headers to write transforms that CSS already supplies. Removing that work may reduce both code and per-scroll cost; I’ll check it in the browser. The resolver prototype matched the current implementation across 240,000 layout comparisons, including revision counters. Its line savings are small: six lines for the column resolver and none for the row version. The local timing runs were slightly faster, though they don’t establish browser performance. The browser check caught a reason not to delete the whole scroll helper: a column with both pinned and sticky set still relies on its inline header transform. The proposed deletion moved that header. |
|
re the |
|
ok so here's the final report, please note that when I started the audit, I didn't have your last commit without the 71 lines removal and it might be part of the audit, so below is the full audit and I haven't changed any file, it's just meant to provide to your Claude and let it analyze it (it could be wrong) and if Claude verified it, then let it do the rest. I also can't run these audits too often because just this single audit burned 40-45% of my small weekly plan, so anyway here it goes... Pinning/sticky library LOC auditScope and conclusionAudited local Scope includes library implementation and styles. Examples, demos, Cypress files, interfaces, and generated output are excluded from findings and savings. Existing example pages were used only as browser fixtures. Another 54 physical source lines can potentially be removed, beyond the previous 71-line cleanup, without removing features or compressing formatting. These savings were measured using temporary candidate files. No changes were applied to the repository, and its working tree was restored to clean. The candidates passed focused checks, but this is not a guarantee against every regression or performance impact. The normal regression suite should run before implementation is merged. Measured opportunities
Counts are physical source lines, including block boundaries. The candidates preserve explanatory comments. 1. Remove the redundant rendered-row scan — 14 linesLocation: The method enumerates The stylesheet already provides that transform through the inherited The candidate removes the row enumeration and its unused
This eliminates an array allocation, row checks, group-cell lookups, and associated inline writes from each horizontal scroll. Do not remove the entire method. A broader deletion failed browser comparison when a column had both 2. Merge only the sticky/center placement branches — 8 linesLocation: The previous review tried introducing a placement descriptor, which increased LOC. That does not rule out a narrower refactor. The sticky-transform and center branches share:
They can share one block while preserving:
The measured candidate saves eight lines without a new type, descriptor object, additional DOM reads, or extra style writes. 3. Reuse visible-column normalization — 8 linesLocation: The numeric-reference path contains the same reduction already implemented by Replace it with the existing helper. Its separate empty-visible-list return is also redundant: the existing This preserves the algorithm and avoids adding another abstraction. 4. Use one ordered map for reconciliation — 6 linesLocation: Currently, reconciliation builds maps for both bands and removes conflicts with The candidate:
It preserves insertion order and the existing leading-edge tie rule. Measured savings:
The row change is optional if minimizing diff churn is the priority. This makes the reconciliation step linear rather than potentially quadratic. It does not make the entire resolver linear: other existing operations, including sorting and right-side activation, remain. 5. Share header geometry declarations — 11 linesLocation: The Group those selectors for the shared geometry, while keeping The candidate preserves selector specificity and retains the comments. 6. Share separator pseudo-element setup — 7 linesLocation: Pinned and sticky separator pseudo-elements repeat the same declarations:
Group their existing selectors for those declarations. Keep the left/right shadow rules separately ordered. This saves lines without nesting the stylesheet or changing selector specificity. VerificationCompleted against temporary candidates:
Browser coverage used six fixture pages, both bundled themes, configuration changes, and multiple scroll positions. It included grouping, variable-height rows, clearing/re-adding docking, and columns configured as both pinned and sticky. Cypress's Electron process exited with status 132 before running tests, including outside the sandbox. Browser comparisons therefore used installed headless Brave through CDP. These focused checks do not replace the full regression suite or cross-browser testing. Performance evidenceFive alternating local Node timing rounds of mixed column/row resolution measured approximately:
This supports the reconciliation change but is not a browser frame-rate or allocation benchmark. The stronger performance argument is structural: the scroll candidate removes per-rendered-row work, and reconciliation removes repeated searches and splices. Corrections and qualifications to the previous reviewThe reported rejection helper is absent locallyThe claimed It is not counted in this audit’s savings. The proxy-scroller invariant is not universalOn a freshly loaded ordinary grid, this public call produced configured docking without a proxy scroller: grid.setOptions(
{ pinning: { columns: { left: 0 } } },
false,
true
);Observed: The third argument suppresses This qualifies the earlier dead-code argument. It does not, by itself, prove that the previous 71-line deletion introduced a regression. “Three passes” does not prove one layout flush
Retain the existing batching and measurement cache, but do not treat the pass labels as proof that every path forces at most one layout. Larger conditional opportunity after RTLThe old sticky DOM-reparenting renderer deserves a separate reachability audit after RTL integration:
Those chunks account for 88 physical lines in the audited checkout, before surrounding simplification. They are not included in the 54-line total:
RecommendationConsider the 54-line set as modest, concrete library simplifications. Keep the activation loops, virtualization, geometry guards, reference caches, and measurement batching. Avoid broad descriptor frameworks or comment trimming solely to meet a LOC target. The previous review was broadly correct that the remaining headroom is modest, but its conclusion that the original 71-line removal was effectively the whole opportunity was too restrictive. |
|
@ghiscoding They are all fairly small savings, but happy to go ahead. Do you want to do it, or will I feed this into Claude? |
can you just feed it to Claude? I'd like Claude to confirm the logic and make the change if possible.... I'm working on another bug I found on Firefox |
…lly needs The spec asserted that configured docking and the proxy scrollbar are always in step. They are not: setOptions() with suppressColumnSet skips the lazy creation in setColumns(), so pinning can be configured and resolved into bands with no scrollbar. Reported in the audit on #1302 and reproduced. Nothing is rendered as docked in that state, which is the property the single scroll path relies on, so that is what the spec now asserts: without the scroll owner there are no docked rows, no pinned header classes, and no band entry resolving to a chrome element. Measured in that state: 3 bands, an empty chrome map, no docked rows, and no work for a scroll to do. Covers the suppressColumnSet state directly, including a horizontal scroll while half-configured and the recovery once setColumns() runs. The chrome lookup cache is deliberately excluded from the assertion: it still holds elements from an earlier docking after pinning is removed, which no band resolves to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
From the audit posted to #1302. Each was confirmed before being applied; the candidates it raised that did not hold up are not here. The rendered-row scan in applyDockingProxyScrollOffsets() wrote an inline transform onto each full-width group cell. _slick-docking.scss already applies the identical translate3d to `.slick-row-docked > .slick-cell-full-width-group` from the custom property the method publishes one line earlier. The CSS needs the cell to be a direct child of its row, which it always is: getRowDockingRegion() returns the row itself for a full-width group row, so such a cell is never placed inside a band wrapper. The rest of the method stays - the permanent-column header loop matters when a column is both pinned and sticky. normalizeColumnPinningReferences() built its visible-column list with a reduce that is character-for-character getVisibleColumnIndexes(). Its empty-list early return went with it: with no visible columns, `count` is Math.min(requested, 0), which is 0 on both sides, so the existing `count === 0` guard already returned. The pinned-edge and sticky-edge separators repeated the same five pseudo-element declarations, and the two left rules carried an identical box-shadow, as did the two right ones. Grouped, keeping left before right so cascade order is unchanged. Verified on the built CSS rather than the source: all 12 separator selectors across the three stylesheets still carry the same six declarations, every left selector keeps `inset -1px 0 0` and every right one `inset 1px 0 0`. Suite: 78 specs, 732 passing, 0 failing, 1 pending. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@ghiscoding Confirmed and implemented. Thanks for running that audit — it was worth the weekly budget, because it caught something I had genuinely got wrong. The correction first: your invariant counterclaim was right, mine was over-statedWhen I removed the 71 lines I claimed in the commit message, and asserted in
You were also right that this doesn't prove a regression, and the last two columns are why: in that state the chrome map is empty and no row is docked, so the deleted code had nothing to act on. The deletion stands, but the claim about it was too broad. The spec now asserts the narrower property the single scroll path actually relies on — nothing is rendered as docked unless the scroll owner exists: no docked rows, no pinned header classes, and no band entry resolving to a chrome element. It covers your One thing that surfaced while fixing it: Implemented: candidates 1, 3 and 6 —
|
|
ahh Claude reported couple of min after my post, so let me repost after Claude comment.... @6pac I was trying to fix the new Firefox bug issue that I found but I don't have enough credit to fix the rest and the cheap models aren't too good at fixing it, but I asked it to provide summary of the problems. Also it seems that both Claude and my ChatGPT completely ignored Firefox and that's the start of the problem, so would you mind providing this summary to Claude and hopefully it can fix it better than my AI. Hopefully this is the last thing left to do on the PR since I think you have completed on your side, is that correct? Here's a print screen of the Firefox scrollbar problem 
and below is the AI summary of the problem, please give this to your Claude Firefox scrollbar issueReproductionIn Firefox hides its scrollbars until hover. When the scrollbar appears, the grid can show what looks like a second scrollbar or “ghost” thumb. The native vertical thumb can also look partly obscured: scrolling down by 1–2 pixels reveals the full thumb at its expected position. This suggests a paint, clipping, or stacking issue; the thumb itself may not be positioned incorrectly. Other visible artifacts include a misaligned bottom pinned-row border while the grid is idle, white strips around the scrollable area, and changes to those artifacts on hover. The horizontal scrollbar region also leaves a strip at the bottom. Scroll structureThe grid’s Relevant source locations:
The current Fixes previously attempted
These approaches did not resolve the artifacts. The guessed scrollbar dimensions and compensating offsets were not validated against Firefox’s actual scrollbar geometry and hover behavior. What to investigate nextMeasure the viewport, overlays, clip paths, and scrollbar bounds in Firefox both before and after hovering, and compare their stacking and paint order. Check whether the native scrollbar changes width or opacity on hover, and whether pinned overlays or the horizontal proxy overlap its area. Verify whether the apparent second track is an actual scroll container or only an overlay/paint artifact. Keep the bottom horizontal scrollbar region separate from the vertical scrollbar investigation. Avoid moving the native thumb by an arbitrary pixel offset: the 1–2px scroll observation points toward something obscuring its initial rendering. Extra Notes - Chrome also has vertical bug problemWhen resizing any columns larger than the viewport, the overlay horizontal scrollbar will show over the data row which is partially hiding the data row behind it. The horizontal scrollbar should be shown below the data row, not over it. |
|
OK, I'll add that to the list. However I am running into a couple of issues with the LOC removal. One is that the RTL PR refactors some of the sections that are being trimmed. I split that out so you could evaluate it separately. Could you have a look and if you are happy with it I might merge it back into this PR so that the LOC reductions are no longer blocked. My 2c: I think it's very important to support RTL. We've just never had anyone with the expertise to know how to come up with a good spec for it (or the time to make the changes). |
The last candidate from the audit on #1302. A column configured sticky on both edges is a candidate for each band, and the reconciliation kept two maps, then removed the loser from whichever list with findIndex() + splice() - a scan and a shift per contested column. It now builds one map of the trailing candidates and filters the leading ones against it, deleting from the map when the leading edge wins, so the step is linear. Behaviour is unchanged, including the tie: a column equidistant from both edges still goes to the leading band. Both lists keep their order, filter() preserving the leading one and Map iteration the trailing one. Verified by comparison rather than by the suite, because sticky: 'both' is the case being changed and no shipped example configures one. The built resolver was run over 4,000 deterministic pseudo-random cases before and after - 3,827 of them containing a two-sided sticky column - comparing full band membership, every offset and width, and the revision counter, with each case resolved twice so the counter's change detection is exercised. Identical throughout. Suite: 78 specs, 731 passing, 1 failing, 1 pending; the failure is the timing test in example-0032-row-span-many-columns, which passes on its own (46/46) and exercises a grid with no pinning or sticky columns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch has not been deployed
3 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.
continue to work as before without pinning or sticky columns; RTL combined with docking is
a known gap (see Follow-up work).
example-sticky-financial-report.htmlgrid.setColumnPinning(columnId, side)andgrid.setColumnStickiness(columnId, side)so an application can build its own pinning menu commands. This repository has no Header Menu
pinning command and no Grid State plugin, so neither is added here and
pinningis notserialized for you; read it back from
grid.getOptions().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 on this repository:
tsc --noEmit.eslint src.npm run build:prod(bundles, declarations and Sass).editing, selection, grouping, reordering, variable-row-height and RTL specs.
The accessibility review 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.
Audit
The branch was audited in three rounds and the findings were fixed on it. Highlights of what the
audit changed: row references by dataset id (including
{ id }for numeric ids),pinning: nullclearing pinning likeundefined, the bottom band nesting sticky rows insidepermanent ones like the top band,
setColumns()validating before it mutates and returning aboolean, restoration of the
applyHtmlCode/trigger/set*Visibility/onHeaderKeyDowncontracts, removal of the fork's keyboard focus routing, and the docking hot paths taken off
O(n²) chrome lookups and per-column forced layout. Weakened Cypress assertions were restored.
The third round closed the rest of that list. A colspan that crosses a pinned boundary is now
clipped to its own band, with each continuation carrying the remainder of the content at the
right offset, so it no longer hides the columns scrolling beneath it. A centre column resized
past the right edge now scrolls the grid to follow it, instead of freezing at roughly a viewport
width with the handle off screen. Four docking measurements were corrected: a vertical scrollbar
subtracted twice in scroll-into-view, two different overflow tests deciding whether a horizontal
scrollbar exists, screen pixels mixed with layout pixels in the right-pinned chrome, and a
colspan validation that only looked at rendered rows. The horizontal scroll offset is published
once on the container rather than on every docked region, sticky cell and chrome element.
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.
Measured against the base commit
179c8ac3,src/is+6,621 / -3,888across 26 files(
+2,733net), of whichsrc/slick.grid.tsis+5,463 / -3,406; that file is now 11,558 lines.These figures exclude demos, tests and generated output.
Follow-up work
The following items are intentionally separate from the v6 implementation:
non-proxy geometry path, so a right-pinned column and an activated sticky column are placed
outside the viewport, and the docked hit-test path is skipped for RTL. RTL grids without
docking are unaffected. Documented as a limitation in
docs/pinning-sticky.md.None of these requires restoring the legacy pane architecture or changing the current pinning/sticky
runtime design.
AI / LLM assistance
(Opus 5 / Fable 5.1) for the two audit rounds and their fixes
documentation updates, test maintenance, and validation support.
Checklist
documentation, tests, and cleanup.
Print Screens