refactor: fit view — pure CSS layout, no DOM measurement in applyFitLayout
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
This commit is contained in:
+19
-44
@@ -654,10 +654,7 @@ function renderGrid(sessions) {
|
|||||||
var currentMode = currentDs.viewMode || 'auto';
|
var currentMode = currentDs.viewMode || 'auto';
|
||||||
if (currentMode === 'fit' && grid) {
|
if (currentMode === 'fit' && grid) {
|
||||||
grid.classList.add('session-grid--fit');
|
grid.classList.add('session-grid--fit');
|
||||||
requestAnimationFrame(function() {
|
applyFitLayout(grid);
|
||||||
applyFitLayout(grid);
|
|
||||||
// No scrollTop hack needed — CSS flex + justify-content:flex-end anchors content to bottom
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
@@ -1014,7 +1011,7 @@ function closeSession() {
|
|||||||
var _closGrid = document.getElementById('session-grid');
|
var _closGrid = document.getElementById('session-grid');
|
||||||
if (_closGrid) {
|
if (_closGrid) {
|
||||||
_closGrid.classList.add('session-grid--fit');
|
_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).
|
* Set grid template for fit mode based on tile count.
|
||||||
* Determines optimal cols × rows based on tile count and available space.
|
* 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
|
* @param {Element} grid - The session grid element
|
||||||
*/
|
*/
|
||||||
function applyFitLayout(grid) {
|
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;
|
var count = grid.querySelectorAll('.session-tile').length;
|
||||||
if (count === 0) return;
|
if (count === 0) {
|
||||||
|
grid.style.removeProperty('grid-template-columns');
|
||||||
// Available space — use grid's parent container
|
grid.style.removeProperty('grid-template-rows');
|
||||||
var parent = grid.parentElement;
|
return;
|
||||||
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;
|
|
||||||
|
|
||||||
// Calculate optimal cols/rows — start with square root
|
// Calculate optimal cols/rows — start with square root
|
||||||
var cols = Math.ceil(Math.sqrt(count));
|
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.gridTemplateColumns = 'repeat(' + cols + ', 1fr)';
|
||||||
grid.style.gridTemplateRows = 'repeat(' + rows + ', 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
|
// Reset any inline styles from previous fit calculation
|
||||||
grid.style.removeProperty('grid-template-rows');
|
grid.style.removeProperty('grid-template-rows');
|
||||||
grid.querySelectorAll('.session-tile').forEach(function(t) {
|
grid.style.removeProperty('grid-template-columns');
|
||||||
t.style.removeProperty('height');
|
|
||||||
});
|
|
||||||
|
|
||||||
if (mode === 'auto') {
|
if (mode === 'auto') {
|
||||||
// Restore grid columns setting
|
// Restore grid columns setting
|
||||||
@@ -1213,7 +1188,7 @@ function applyDisplaySettings(ds) {
|
|||||||
|
|
||||||
} else if (mode === 'fit') {
|
} else if (mode === 'fit') {
|
||||||
grid.classList.add('session-grid--fit');
|
grid.classList.add('session-grid--fit');
|
||||||
requestAnimationFrame(function() { applyFitLayout(grid); });
|
applyFitLayout(grid);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1880,7 +1855,7 @@ window.addEventListener('resize', function() {
|
|||||||
var ds = loadDisplaySettings();
|
var ds = loadDisplaySettings();
|
||||||
if ((ds.viewMode || 'auto') === 'fit') {
|
if ((ds.viewMode || 'auto') === 'fit') {
|
||||||
var grid = document.getElementById('session-grid');
|
var grid = document.getElementById('session-grid');
|
||||||
if (grid) requestAnimationFrame(function() { applyFitLayout(grid); });
|
if (grid) applyFitLayout(grid);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -1380,6 +1380,15 @@ body {
|
|||||||
/* Fit view — tiles fill viewport, no scroll */
|
/* Fit view — tiles fill viewport, no scroll */
|
||||||
.session-grid--fit {
|
.session-grid--fit {
|
||||||
overflow: hidden;
|
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:
|
/* In fit mode, tile body and pre use the base CSS:
|
||||||
|
|||||||
@@ -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');
|
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(
|
assert.ok(
|
||||||
source.includes('requestAnimationFrame') && source.includes('applyFitLayout'),
|
source.includes('applyFitLayout'),
|
||||||
'applyFitLayout must be deferred via requestAnimationFrame'
|
'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', () => {
|
test('applyFitLayout sets gridTemplateColumns and gridTemplateRows via pure arithmetic', () => {
|
||||||
const removedGridProps = [];
|
const assignedProps = {};
|
||||||
const removedTileProps = [];
|
|
||||||
|
|
||||||
const mockTile = {
|
const mockTile = { style: { removeProperty: () => {} } };
|
||||||
style: { removeProperty: (prop) => removedTileProps.push(prop) },
|
|
||||||
};
|
|
||||||
|
|
||||||
const mockGrid = {
|
const mockGrid = {
|
||||||
style: {
|
style: new Proxy(assignedProps, {
|
||||||
removeProperty: (prop) => removedGridProps.push(prop),
|
set(target, prop, value) { target[prop] = value; return true; },
|
||||||
gridTemplateColumns: '',
|
get(target, prop) { return target[prop]; },
|
||||||
gridTemplateRows: 'repeat(2, 1fr)', // stale value from previous call
|
}),
|
||||||
},
|
|
||||||
querySelectorAll: (sel) => {
|
querySelectorAll: (sel) => {
|
||||||
if (sel === '.session-tile') return [mockTile];
|
if (sel === '.session-tile') return [mockTile, mockTile, mockTile, mockTile];
|
||||||
return [];
|
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);
|
app.applyFitLayout(mockGrid);
|
||||||
|
|
||||||
assert.ok(
|
assert.ok(
|
||||||
removedGridProps.includes('grid-template-rows'),
|
typeof assignedProps.gridTemplateColumns === 'string' && assignedProps.gridTemplateColumns.includes('1fr'),
|
||||||
'applyFitLayout must removeProperty("grid-template-rows") before recalculating to prevent stale layout interference'
|
'applyFitLayout must set gridTemplateColumns with 1fr tracks'
|
||||||
);
|
);
|
||||||
assert.ok(
|
assert.ok(
|
||||||
removedTileProps.includes('height'),
|
typeof assignedProps.gridTemplateRows === 'string' && assignedProps.gridTemplateRows.includes('1fr'),
|
||||||
'applyFitLayout must removeProperty("height") from each tile before recalculating'
|
'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');
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -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:
|
def test_fit_view_no_pre_static_override() -> None:
|
||||||
"""Bug fix: .session-grid--fit .tile-body pre must NOT override position to static.
|
"""Bug fix: .session-grid--fit .tile-body pre must NOT override position to static.
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user