refactor!: render grid header and footer declaratively with Lit#12134
Conversation
6d72e75 to
639259c
Compare
|
/code-review |
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 6 comments.
| Finding | |
|---|---|
An all-empty header/footer row is no longer hidden, and has-header/has-footer can stay stale. |
|
| ⚡ | _swapColumnOrders re-renders the entire header and footer on every swap inside the drag loop. |
| 🧹 | The header/footer branch of __initRow is now dead code. |
| ⚡ | _resizableChanged re-renders the whole header per column with no batching. |
| 👀 | Cell references are reconstructed by scraping the rendered DOM instead of being assigned during render. |
| 🧹 | The header and footer render methods are near-identical copies. |
| // Header and footer rows are rendered by Lit, which orders their cells by | ||
| // `_order`. Re-render them so Lit reorders the cells. Moving Lit-managed | ||
| // cells manually would corrupt Lit's internal state. | ||
| this.__renderHeader(); |
There was a problem hiding this comment.
⚡ _swapColumnOrders re-renders the entire header and footer on every swap inside the drag loop.
_onTrack calls _swapColumnOrders in a for loop, once per intermediate column between the dragged column and the target. Each call now runs this.__renderHeader() and this.__renderFooter(), and each of those re-renders the full template for every level and every column. So one track event that advances K positions does 2*K full header+footer renders, and track fires continuously during a drag. The previous code moved only the two affected cells directly in the DOM.
This turns a cheap two-cell move into dozens of full-tree renders across a drag, causing visible jank on wide or multi-level grids. Swap the _order values (and move body/sizer cells) inside the loop, then call __renderHeader()/__renderFooter() once after the loop in _onTrack.
vaadin-grid-column-reordering-mixin.js:450 · efficiency · confirmed
There was a problem hiding this comment.
Fixed by calling __scheduleRenderHeaderFooter which schedules a microtask debouncer instead of rendering them right away. I still added a __renderHeaderFooter call to _onTrack after the loop to flush the debouncer immediately because the tests rely on the header and footer being rendered synchronously in that case.
The grid attaches cell event listeners in different places and in different ways: the tooltip mouse listeners are created as new closures for every cell in `_createCell`, while the `keydown` listener is bound per column and attached in two separate places in `__initRow`, but only for columns that define `_onCellKeyDown`. This moves all cell event handling to the grid itself: - The grid gets shared bound methods (`__onCellMouseEnter`, `__onCellMouseLeave`, `__onCellMouseDown`, `__onCellKeyDown`) that are attached once per cell in `_createCell`. - On `keydown`, the cell delegates to its column through `column.__onCellKeyDown`, so the per-column listener setup in `__initRow` is no longer needed. The selection column's `_onCellKeyDown` is renamed to `__onCellKeyDown` to match. - Cells without a column (the row details cell) skip the delegation, so behavior stays the same. This also prepares for rendering header and footer rows declaratively with Lit, where the same handler references can be used directly in the template. Extracted from #12134 Part of #10789 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3216d40 to
397e375
Compare
c9e6ee0 to
17efc7e
Compare
|
/code-review |
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 7 comments.
| Finding | |
|---|---|
#isHeaderRowVisible now hides a column whose header is null even when it has a headerRenderer, reversing the old precedence. |
|
| ⚡ | _onTrack calls the synchronous __renderHeaderFooter() on every drag-over event, even when no column swap happened. |
| ⚡ | _resizableChanged runs a synchronous full header/footer re-render per column, unlike the debounced renderer-change path. |
| 👀 | _renderColumnTree re-queries the DOM after rendering to rebuild _headerCell/_footerCell/_emptyCells, keeping two representations of the same cells. |
| ⚡ | _recalculateColumnWidths now forces an unconditional full header/footer re-render instead of flushing a pending one. |
| 🧹 | #renderHeaderRow/#renderFooterRow and #isHeaderRowVisible/#isFooterRowVisible are near-identical copies. |
| 👀 | The test asserting header/footer visibility updates run a bounded number of times was removed with no replacement. |
| } | ||
| }); | ||
| } | ||
| this._grid.__renderHeaderFooter?.(); |
There was a problem hiding this comment.
⚡ _resizableChanged runs a synchronous full header/footer re-render per column, unlike the debounced renderer-change path.
Toggling one column's resizable calls this._grid.__renderHeaderFooter?.() synchronously, which re-renders every header and footer row and runs _resetKeyboardNavigation() + __a11yUpdateGridSize(). The old code only added or removed a single resize-handle <div> on that column's cells.
Applying resizable to N columns in a loop (or a framework re-applying properties) triggers N full re-renders in one task instead of one coalesced render. The header/footer renderer observers already use the debounced __scheduleRenderHeaderFooter(); using it here too would coalesce the work and keep the two paths consistent.
vaadin-grid-column-mixin.js:506 · efficiency · confirmed
There was a problem hiding this comment.
Fixed, but it changes the timing a little bit, e.g. it required adding flushGrid here:
|
|
||
| iterateChildren(this.$.header, (headerRow, index) => { | ||
| this.__initRow(headerRow, columnTree[index], 'header', index === columnTree.length - 1); | ||
| this.$.header.querySelectorAll('.header-cell').forEach((cell) => { |
There was a problem hiding this comment.
👀 _renderColumnTree re-queries the DOM after rendering to rebuild _headerCell/_footerCell/_emptyCells, keeping two representations of the same cells.
After __renderHeaderFooter() the code runs querySelectorAll('.header-cell') and .footer-cell and re-derives each column's cell references by DOM position (parentElement === lastElementChild / firstElementChild). The render already sets ._column on every cell and knows its level and column-row status, so this repeats work the template already did and depends on class strings and child order matching the column order.
This rebuild runs only in _renderColumnTree, not in the __renderHeaderFooter path used by __scheduleRenderHeaderFooter (reorder, resizable, renderer change). Keyed repeat plus cache reuse the same nodes today, so refs stay valid — but any future change on that path that creates or drops a th/td would leave _headerCell/_footerCell stale, and the width/frozen/part observers would act on the wrong element. Assigning these references during render (e.g. in CellContentDirective.update) would remove both the DOM re-query and the fragile position heuristic.
vaadin-grid-mixin.js:601 · altitude · plausible
There was a problem hiding this comment.
This will be improved in the future.
| this.__updateHeaderFooterRowParts('header'); | ||
| } | ||
|
|
||
| #renderHeaderRow(columns, level) { |
There was a problem hiding this comment.
🧹 #renderHeaderRow/#renderFooterRow and #isHeaderRowVisible/#isFooterRowVisible are near-identical copies.
The two ~35-line row templates differ only in th vs td, role, the header-cell/footer-cell part+class token, the slot-name prefix, and the header's extra resize handle; the <tr>, the keyed repeat, the cache(nothing) hidden branch, all four @mouse*/@keydown handlers, tabindex, and ._column are copy-pasted. The two visibility helpers likewise repeat the isColumnRow / isEmptyCell / column.hidden guard.
A future change to the cell template (a new event handler, an ARIA attribute) or to the empty-cell rule must be made in both places; miss one and header and footer silently diverge. A single #renderRow(section, columns, level) parameterized on tag/role/part/slot-prefix, plus a shared visibility helper taking a per-section predicate, would remove the duplication.
vaadin-grid-column-rendering-mixin.js:77 · simplification · plausible
There was a problem hiding this comment.
Let's keep them separate for now since not all the logic has been moved to Lit templates yet.
DiegoCardoso
left a comment
There was a problem hiding this comment.
I haven't been able to check all the changes yet, but tried running FC ITs against this branch and these tests seem to be affected by the refactor:
[ERROR] GridHeaderFooterRowIT.addFootersAfterGridIsRendered_cellsAreRenderedInCorrectOrder:116->assertFooterOrder:420->lambda$assertFooterOrder$16:421 Unexpected footer cell content expected:<[1]> but was:<[]>
[ERROR] GridHeaderFooterRowIT.addHeadersAfterGridIsRendered_cellsAreRenderedInCorrectOrder:104->assertHeaderOrder:384->lambda$assertHeaderOrder$10:385 Unexpected header cell content expected:<[1]> but was:<[]>
[ERROR] GridHeaderFooterRowIT.columnSetHeader_alwaysTargetsTheFirstCreatedHeader:279->assertHeaderOrder:384->lambda$assertHeaderOrder$10:385 Unexpected header cell content expected:<[1]> but was:<[]>
[ERROR] GridHeaderFooterRowIT.joinCells:300->assertHeaderOrder:384->lambda$assertHeaderOrder$10:385 Unexpected header cell content expected:<[0]> but was:<[]>
I confirmed that these are the only failures. The grid connector wraps header and footer renderers in With the new declarative rendering, cells can be re-created on column tree changes. For example, adding a header row wraps each column in a column group, which moves the column to a new header row. The grid re-creates the cell with an empty content element and calls the column's header renderer again to fill it, but the single-time renderer no longer renders anything, so the cell stays empty. Reusing the cell elements like before is not something Lit is designed to support. Lit reconciles DOM per container: the header is rendered as a list of Based on this, the right move seems to be getting rid of single-time renderers in the grid connector. I've already tried replicating the old behavior, but it complicates the code too much and moves away from declarative rendering back to imperative patterns |
e12caba to
bb28366
Compare
When a column is removed, the grid also removes the column's `<vaadin-grid-cell-content>` elements from its light DOM, and adds them back when the column is added again. This behavior wasn't covered by tests for header and footer cells. The declarative header/footer rendering refactor changes how header and footer cell content is managed, so this adds tests to lock in the current behavior first. Extracted from #12134 Part of #10789 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
dbbe5dc to
616c701
Compare
The grid web component is moving to rendering its header and footer declaratively with Lit (vaadin/web-components#12134). With that change, header and footer cells can be re-created, which re-runs the column renderers. Rendering only once would leave re-created cells empty. The sorter element is cached between runs because re-creating it would reset the sort direction and notify the server. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Grid ITs are passing now: https://github.com/vaadin/flow-components/actions/runs/29920480694 |
A null header should only suppress the default header text, not hide a row rendered by headerRenderer. Also extract CellContentDirective into its own module and turn the row visibility checks into pure functions.
Calling __renderHeaderFooter() directly re-renders header and footer on every column width recalculation, even when no render is pending. Flushing the debouncer applies only a scheduled render.
Avoids redundant re-renders on dragover events that don't change column order.
Column observers rendered header and footer synchronously on every property change. Scheduling coalesces multiple changes into one render.
Header and footer cells are re-created by the scheduled header/footer render when columns are attached or detached, so their content no longer needs manual transfer.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
395dab4 to
95a10ca
Compare
Cell references (`_headerCell`, `_footerCell`, `_emptyCells`) must be refreshed on every header/footer render, not only on column tree updates, since the rendering mixin can re-render rows on its own.
|



The grid currently builds its header and footer imperatively: rows and cells are created and removed by hand, row visibility is updated through per-row debouncers, and column reordering moves cells around in the DOM. This makes the rendering logic hard to follow and easy to get out of sync with the column tree.
This PR takes the first step in moving the header and footer rendering to declarative Lit templates: it covers the initial rendering of their rows and cells, reordering, visibility changes, and resize handles. Dynamic parts, classes, and various other cell and row state are still applied imperatively on top of the rendered DOM. They will be gradually moved into the templates in future PRs.
Non-obvious changes
column._id), and cells of hidden columns are kept alive with thecachedirective, so cells and their content are reused when columns are reordered, hidden, or shown.vaadin-grid-column-group. The cell and its content element are then recreated, and the column's header or footer renderer runs again to fill the new content element. Renderers now need to handle running multiple times; previously the cell element was moved between rows, so its content survived such changes without a re-render: refactor: allow grid header and footer renderers to run multiple times flow-components#9757__updateHeaderFooterRowVisibilitydebouncers. Column property changes that affect the header or footer now schedule a single render of both sections, debounced on a microtask. This also changes the timing ofresizable: the resize handle used to be added or removed synchronously, now it appears after the microtask.vaadin-grid-header-cell-content-0-1) so they stay stable across re-renders. Body cell slot names keep the old format, but their numbering shifts because header and footer cells no longer share the same counter.vaadin-grid-sorteris now appended to the cell content only after its direction is set. With declarative rendering, the content element is already connected when the default header renderer runs, and appending an unconfigured sorter would reset the column's sort direction.allRowsVisiblemode where the scroller height update could be skipped, leaving the grid clipped after it grows. That issue was fixed separately in fix: render all rows on deep expand with all-rows-visible #12146Depends on
Part of #10789