From b450cf7176d51d1edc7987bf594eaa83ec68329c Mon Sep 17 00:00:00 2001 From: Brian Krabach Date: Tue, 31 Mar 2026 07:47:54 -0700 Subject: [PATCH] =?UTF-8?q?refactor:=20fit=20view=20=E2=80=94=20pure=20CSS?= =?UTF-8?q?=20layout,=20no=20DOM=20measurement=20in=20applyFitLayout?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of repeated fit view failures: applyFitLayout() measured clientHeight/clientWidth/getComputedStyle and set inline tile heights. This failed in multiple ways: - clientHeight = 0 when container is display:none (returning from session) - inline tile.style.height destroyed every 2s by innerHTML rebuild in pollSessions - getComputedStyle forces style recalc with potentially stale values - rAF wrappers couldn't reliably fix timing issues Fix: pure arithmetic applyFitLayout — no DOM measurement, no inline heights. The grid already has a definite height from CSS (flex:1 inside height:100dvh). grid-template-rows: repeat(rows, 1fr) divides that space without JS measurement. CSS changes: - .session-grid--fit { align-content: stretch } - .session-grid--fit .session-tile { height: auto } — CSS grid cell controls sizing JS changes: - applyFitLayout() removes all clientHeight/clientWidth/getComputedStyle/style.height - applyFitLayout() is now pure arithmetic: count tiles, compute cols×rows, set 1fr templates - Removed rAF wrappers from all call sites (renderGrid, closeSession, resize handler, applyDisplaySettings) — safe to call synchronously since no layout measurement needed - applyDisplaySettings() no longer clears tile heights (never set them anymore) Tests updated: - Added test_fit_view_session_tile_has_height_auto (CSS) - Added 'applyFitLayout does NOT measure DOM dimensions' (JS) - Updated 'applyFitLayout clears stale...' → 'sets gridTemplateColumns/Rows via arithmetic' - Updated rAF-wrapper test to match new direct-call behavior --- muxplex/frontend/app.js | 63 ++++++-------------- muxplex/frontend/style.css | 9 +++ muxplex/frontend/tests/test_app.mjs | 89 ++++++++++++++++++----------- muxplex/tests/test_frontend_css.py | 22 +++++++ 4 files changed, 107 insertions(+), 76 deletions(-) diff --git a/muxplex/frontend/app.js b/muxplex/frontend/app.js index 7c08a9c..9ee3158 100644 --- a/muxplex/frontend/app.js +++ b/muxplex/frontend/app.js @@ -654,10 +654,7 @@ function renderGrid(sessions) { var currentMode = currentDs.viewMode || 'auto'; if (currentMode === 'fit' && grid) { grid.classList.add('session-grid--fit'); - requestAnimationFrame(function() { - applyFitLayout(grid); - // No scrollTop hack needed — CSS flex + justify-content:flex-end anchors content to bottom - }); + applyFitLayout(grid); } } @@ -1014,7 +1011,7 @@ function closeSession() { var _closGrid = document.getElementById('session-grid'); if (_closGrid) { _closGrid.classList.add('session-grid--fit'); - requestAnimationFrame(function() { applyFitLayout(_closGrid); }); + applyFitLayout(_closGrid); } } @@ -1098,36 +1095,24 @@ function saveDisplaySettings(settings) { } /** - * Calculate and apply grid layout to fill the viewport exactly (Fit mode). - * Determines optimal cols × rows based on tile count and available space. + * Set grid template for fit mode based on tile count. + * Pure arithmetic — no DOM measurement, no getComputedStyle, no clientHeight. + * Safe to call at any time regardless of display state or layout phase. + * + * The grid already has a definite height from CSS (flex: 1 inside height: 100dvh). + * Setting grid-template-rows: repeat(rows, 1fr) lets the browser divide that height + * equally without JS needing to know the pixel dimensions. Tiles use height: auto + * (set in CSS) so they fill their grid cells without inline style overrides. + * * @param {Element} grid - The session grid element */ function applyFitLayout(grid) { - // Clear stale layout from previous calls (prevents interference on page reload and - // when returning from a session where the grid was display:none during measurement) - grid.style.removeProperty('grid-template-rows'); - grid.querySelectorAll('.session-tile').forEach(function(t) { - t.style.removeProperty('height'); - }); - var count = grid.querySelectorAll('.session-tile').length; - if (count === 0) return; - - // Available space — use grid's parent container - var parent = grid.parentElement; - var availH = parent ? parent.clientHeight : window.innerHeight; - var availW = grid.clientWidth; - - // Subtract padding and gap - var style = getComputedStyle(grid); - var padT = parseFloat(style.paddingTop) || 0; - var padB = parseFloat(style.paddingBottom) || 0; - var padL = parseFloat(style.paddingLeft) || 0; - var padR = parseFloat(style.paddingRight) || 0; - var gap = parseFloat(style.gap) || 8; - - var innerW = availW - padL - padR; - var innerH = availH - padT - padB; + if (count === 0) { + grid.style.removeProperty('grid-template-columns'); + grid.style.removeProperty('grid-template-rows'); + return; + } // Calculate optimal cols/rows — start with square root var cols = Math.ceil(Math.sqrt(count)); @@ -1143,16 +1128,8 @@ function applyFitLayout(grid) { } } - // Tile height from available space - var tileH = (innerH - gap * (rows - 1)) / rows; - grid.style.gridTemplateColumns = 'repeat(' + cols + ', 1fr)'; grid.style.gridTemplateRows = 'repeat(' + rows + ', 1fr)'; - - // Override tile height so tiles fill the grid rows - grid.querySelectorAll('.session-tile').forEach(function(t) { - t.style.height = tileH + 'px'; - }); } /** @@ -1199,9 +1176,7 @@ function applyDisplaySettings(ds) { // Reset any inline styles from previous fit calculation grid.style.removeProperty('grid-template-rows'); - grid.querySelectorAll('.session-tile').forEach(function(t) { - t.style.removeProperty('height'); - }); + grid.style.removeProperty('grid-template-columns'); if (mode === 'auto') { // Restore grid columns setting @@ -1213,7 +1188,7 @@ function applyDisplaySettings(ds) { } else if (mode === 'fit') { grid.classList.add('session-grid--fit'); - requestAnimationFrame(function() { applyFitLayout(grid); }); + applyFitLayout(grid); } } @@ -1880,7 +1855,7 @@ window.addEventListener('resize', function() { var ds = loadDisplaySettings(); if ((ds.viewMode || 'auto') === 'fit') { var grid = document.getElementById('session-grid'); - if (grid) requestAnimationFrame(function() { applyFitLayout(grid); }); + if (grid) applyFitLayout(grid); } }); diff --git a/muxplex/frontend/style.css b/muxplex/frontend/style.css index 57cf1b8..fbc3d9e 100644 --- a/muxplex/frontend/style.css +++ b/muxplex/frontend/style.css @@ -1380,6 +1380,15 @@ body { /* Fit view — tiles fill viewport, no scroll */ .session-grid--fit { overflow: hidden; + align-content: stretch; /* let 1fr rows fill container height */ +} + +/* In fit mode, tiles must use height:auto so the CSS grid 1fr rows control sizing. + The base .session-tile uses var(--tile-height) which is a fixed pixel value — + overriding with height:auto lets the grid cell (from grid-template-rows: repeat(n,1fr)) + determine the tile height instead. JS sets only grid-template-columns/rows, not inline heights. */ +.session-grid--fit .session-tile { + height: auto; } /* In fit mode, tile body and pre use the base CSS: diff --git a/muxplex/frontend/tests/test_app.mjs b/muxplex/frontend/tests/test_app.mjs index c83fab2..3159387 100644 --- a/muxplex/frontend/tests/test_app.mjs +++ b/muxplex/frontend/tests/test_app.mjs @@ -2205,11 +2205,22 @@ test('bindStaticEventListeners wires view-mode-btn click to cycleViewMode', () = ); }); -test('applyFitLayout is called via requestAnimationFrame for correct timing', () => { +test('applyFitLayout is called directly (no requestAnimationFrame needed — pure arithmetic)', () => { + // Pure arithmetic applyFitLayout is safe to call synchronously at any time — + // it does not measure DOM dimensions so there is no layout-timing dependency. + // Call sites (renderGrid, closeSession, applyDisplaySettings, resize handler) + // should call it directly, not via requestAnimationFrame. const source = fs.readFileSync(new URL('../app.js', import.meta.url), 'utf8'); + // Verify applyFitLayout exists as a direct call (not only inside rAF callbacks) assert.ok( - source.includes('requestAnimationFrame') && source.includes('applyFitLayout'), - 'applyFitLayout must be deferred via requestAnimationFrame' + source.includes('applyFitLayout'), + 'app.js must define and call applyFitLayout' + ); + // The function body itself must not measure DOM — verified by the separate + // "does NOT measure DOM dimensions" test. Here just confirm the function exists. + assert.ok( + source.includes('function applyFitLayout('), + 'applyFitLayout function must be declared' ); }); @@ -2256,48 +2267,62 @@ test('CSS style.css uses base position:absolute bottom:0 for fit mode content an ); }); -// --- Fit view Bug 1: applyFitLayout clears stale layout before recalculating --- +// --- Fit view layout: applyFitLayout sets grid template via pure arithmetic --- -test('applyFitLayout clears stale grid-template-rows and tile heights before recalculating', () => { - const removedGridProps = []; - const removedTileProps = []; +test('applyFitLayout sets gridTemplateColumns and gridTemplateRows via pure arithmetic', () => { + const assignedProps = {}; - const mockTile = { - style: { removeProperty: (prop) => removedTileProps.push(prop) }, - }; + const mockTile = { style: { removeProperty: () => {} } }; const mockGrid = { - style: { - removeProperty: (prop) => removedGridProps.push(prop), - gridTemplateColumns: '', - gridTemplateRows: 'repeat(2, 1fr)', // stale value from previous call - }, + style: new Proxy(assignedProps, { + set(target, prop, value) { target[prop] = value; return true; }, + get(target, prop) { return target[prop]; }, + }), querySelectorAll: (sel) => { - if (sel === '.session-tile') return [mockTile]; + if (sel === '.session-tile') return [mockTile, mockTile, mockTile, mockTile]; return []; }, - clientWidth: 800, - parentElement: { clientHeight: 600 }, }; - const origGetComputedStyle = globalThis.getComputedStyle; - globalThis.getComputedStyle = () => ({ - paddingTop: '16px', paddingBottom: '16px', - paddingLeft: '16px', paddingRight: '16px', - gap: '8px', - }); - app.applyFitLayout(mockGrid); assert.ok( - removedGridProps.includes('grid-template-rows'), - 'applyFitLayout must removeProperty("grid-template-rows") before recalculating to prevent stale layout interference' + typeof assignedProps.gridTemplateColumns === 'string' && assignedProps.gridTemplateColumns.includes('1fr'), + 'applyFitLayout must set gridTemplateColumns with 1fr tracks' ); assert.ok( - removedTileProps.includes('height'), - 'applyFitLayout must removeProperty("height") from each tile before recalculating' + typeof assignedProps.gridTemplateRows === 'string' && assignedProps.gridTemplateRows.includes('1fr'), + 'applyFitLayout must set gridTemplateRows with 1fr tracks' ); - - if (origGetComputedStyle) globalThis.getComputedStyle = origGetComputedStyle; - else delete globalThis.getComputedStyle; +}); + +// --- Pure CSS fit layout: applyFitLayout must not measure DOM --- + +test('applyFitLayout does NOT measure DOM dimensions (pure arithmetic)', () => { + const source = fs.readFileSync(new URL('../app.js', import.meta.url), 'utf8'); + const fnStart = source.indexOf('function applyFitLayout('); + assert.ok(fnStart !== -1, 'applyFitLayout function must exist'); + // Find end of function: next \n} at the same nesting level + let depth = 0; + let pos = source.indexOf('{', fnStart); + const fnBodyStart = pos; + while (pos < source.length) { + if (source[pos] === '{') depth++; + else if (source[pos] === '}') { + depth--; + if (depth === 0) break; + } + pos++; + } + const fnBody = source.substring(fnBodyStart, pos + 1); + + assert.ok(!fnBody.includes('clientHeight'), + 'applyFitLayout must NOT read clientHeight — causes wrong values when container is display:none'); + assert.ok(!fnBody.includes('clientWidth'), + 'applyFitLayout must NOT read clientWidth — pure 1fr CSS handles width'); + assert.ok(!fnBody.includes('getComputedStyle'), + 'applyFitLayout must NOT call getComputedStyle — pure 1fr CSS handles gap/padding'); + assert.ok(!fnBody.includes('.style.height'), + 'applyFitLayout must NOT set inline tile heights — CSS grid 1fr rows handle sizing'); }); diff --git a/muxplex/tests/test_frontend_css.py b/muxplex/tests/test_frontend_css.py index 78164e9..ce1e460 100644 --- a/muxplex/tests/test_frontend_css.py +++ b/muxplex/tests/test_frontend_css.py @@ -1762,6 +1762,28 @@ def test_fit_view_no_tile_body_flex_override() -> None: ) +def test_fit_view_session_tile_has_height_auto() -> None: + """Pure CSS fit layout: .session-grid--fit .session-tile must use height: auto. + + JS was measuring clientHeight and setting tile.style.height = tileH + 'px'. + This failed when the grid was display:none (clientHeight = 0) and after + innerHTML rebuilds destroyed inline styles every 2s. + + Pure CSS fix: the grid has a definite height (flex:1 inside 100dvh). + grid-template-rows: repeat(rows, 1fr) divides that height equally. + height: auto on tiles lets them fill their grid cells without JS measurement. + """ + css = read_css() + assert ".session-grid--fit .session-tile {" in css, ( + ".session-grid--fit .session-tile rule must exist for pure CSS fit layout" + ) + block = _extract_rule_block(css, ".session-grid--fit .session-tile {") + assert "height: auto" in block or "height:auto" in block, ( + ".session-grid--fit .session-tile must use height: auto — " + "JS inline height setting was unreliable (lost on innerHTML rebuild every 2s)" + ) + + def test_fit_view_no_pre_static_override() -> None: """Bug fix: .session-grid--fit .tile-body pre must NOT override position to static.