From 324a7eac877998abfe4e3385374ef6e8e1d66e33 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 03:36:14 +0000 Subject: [PATCH] fix(web): style editor no longer strands leaf-valued layout fields A prior fix on this PR made elementKeys() append any layout-only key with no style block of its own (a logo, a timeout indicator, a possession arrow), so table() draws a row for it instead of losing it when the wholesale `layout` claim removes the generic fallback. That covers a layout-only key shaped like an object (x_offset/y_offset, ...), because columnsFor() only ever produced columns from a key's *sub-fields*. It missed the case where the layout-only key's own value is itself a leaf -- a plain "show_logo" boolean directly under layout, no x/y object underneath. elementKeys() still lists it (any row: no matching column), so it renders as an uneditable blank row and its only control -- the generic fallback checkbox -- is still gone. Confirmed by executing the real widget's render() against a synthetic schema in Node (a DOM-stub harness, not committed): the field's name never appeared as an . columnsFor() now gives such a leaf key a column keyed to itself ('layout-leaf'), and elementRow() binds it to the leaf's own path (customization.layout., matching the name the fallback would have used) instead of leaving every cell blank. New regression test (test/js/unit/test_style_editor_layout_leaf_columns.js, following this PR's existing eval-extraction pattern) checks the leaf column is produced, is self-keyed, doesn't duplicate, and that a schema with no leaf-valued layout key is unaffected; wired into run_all.js and the suite table in test/js/README.md. test/js/run_all.js: 84 + 6 + 6 = all suites passed (jsdom unavailable here, DOM suites skip as before). Python suite untouched by this change; test_style_editor_extra_fields.py, test_style_editor_save_roundtrip.py and the one PIL-dependent style_editor_takeover.py case fail identically before this commit -- missing flask/PIL in this sandbox, not this PR. Co-Authored-By: Claude Opus 5 --- test/js/README.md | 1 + test/js/run_all.js | 3 +- .../test_style_editor_layout_leaf_columns.js | 89 +++++++++++++++++++ .../static/v3/js/widgets/style-editor.js | 36 ++++++-- 4 files changed, 119 insertions(+), 10 deletions(-) create mode 100644 test/js/unit/test_style_editor_layout_leaf_columns.js diff --git a/test/js/README.md b/test/js/README.md index 01c1e7a4..c5f4013a 100644 --- a/test/js/README.md +++ b/test/js/README.md @@ -37,6 +37,7 @@ nothing is listening, so it stays useful in a bare checkout. | `unit/test_list_filter.js` | no | `ListFilter` search/filter/sort/count/sticky, and the installed-plugins config **extracted verbatim** from `plugins_manager.js` so the test can't drift from it | | `unit/test_render_cards.js` | no | `renderInstalledCards` markup, both empty states, and HTML-escaping of hostile plugin metadata | | `unit/test_style_editor_element_keys.js` | no | `elementKeys()` from `widgets/style-editor.js`: a hand-written `customization.layout` entry with no style block of its own still gets a row, since the widget claims the whole `layout` block from the generic fallback renderer | +| `unit/test_style_editor_layout_leaf_columns.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only key whose own value is a leaf (no x/y sub-object, e.g. a `show_logo` toggle) gets a self-keyed column instead of a blank, uneditable row | | `dom/test_installed_dom.js` | yes | The toolbar in a real DOM: pill/search/sort interaction, the HTMX partial re-swap, and a `getComputedStyle` check that `.filter-pill[data-active]` really matches the emitted markup | | `dom/test_store_dom.js` | yes | Store pagination, per-page, category, tri-state Installed button, and persistence across a re-boot, against the live registry | | `dom/test_no_double_fetch.js` | yes | Loads the **whole** `plugins_manager.js` and counts requests: typing in the store search must filter the cached list, not refetch `/api/v3/plugins/store/list` | diff --git a/test/js/run_all.js b/test/js/run_all.js index 562d2548..a5b0c31a 100755 --- a/test/js/run_all.js +++ b/test/js/run_all.js @@ -15,7 +15,8 @@ const fs = require('fs'); const BASE = process.env.BASE || 'http://localhost:5000'; const UNIT = ['unit/test_list_filter.js', 'unit/test_render_cards.js', - 'unit/test_html_escaping.js', 'unit/test_style_editor_element_keys.js']; + 'unit/test_html_escaping.js', 'unit/test_style_editor_element_keys.js', + 'unit/test_style_editor_layout_leaf_columns.js']; const DOM = ['dom/test_installed_dom.js', 'dom/test_store_dom.js', 'dom/test_no_double_fetch.js', 'dom/test_tools_sections.js']; diff --git a/test/js/unit/test_style_editor_layout_leaf_columns.js b/test/js/unit/test_style_editor_layout_leaf_columns.js new file mode 100644 index 00000000..4d495ace --- /dev/null +++ b/test/js/unit/test_style_editor_layout_leaf_columns.js @@ -0,0 +1,89 @@ +// Regression test for style-editor.js's columnsFor(), extracted verbatim +// from the shipped widget (own/ownObj/elementKeys/columnsFor have no DOM +// dependency) so the test can't drift from the real implementation. +// +// elementKeys() already appends a layout-only key that never got its own +// top-level style block (see test_style_editor_element_keys.js), so table() +// draws a row for it. But render() still claims the whole `layout` child as +// the widget's own regardless of what that row actually shows -- and +// columnsFor() only produced a column for a layout-only key whose own value +// is an *object* with sub-fields (x_offset, y_offset, ...). A hand-written +// schema can instead put a plain leaf value directly under layout -- a +// "show_logo" toggle, a timeout indicator's on/off flag -- with no x/y +// object underneath it. That key still got a row (data-element="show_logo"), +// but zero columns ever matched it, so the row rendered with every cell +// blank: no control, and no way back to the fallback since layout was +// removed wholesale. This pins the fix: such a key gets its own +// self-keyed 'layout-leaf' column. +const fs = require('fs'); +const path = require('path'); +const V3 = path.resolve(__dirname, '../../../web_interface/static/v3'); + +const src = fs.readFileSync(path.join(V3, 'js/widgets/style-editor.js'), 'utf8'); +const start = src.indexOf('function own(obj, key)'); +const endMark = 'function control(opts)'; +const end = src.indexOf(endMark); +if (start === -1 || end === -1) { + console.error('FAIL: could not locate own()/columnsFor() in style-editor.js -- slice markers need updating'); + process.exit(1); +} +eval(src.slice(start, end)); // defines own/ownObj/elementKeys/columnsFor + +let failures = 0; +function ok(desc, cond) { + if (cond) { console.log(`ok - ${desc}`); } + else { console.log(`FAIL - ${desc}`); failures++; } +} + +// score_text is a real style element; possession is layout-only but shaped +// like an object (x/y); show_logo is layout-only and a bare leaf value. +const schema = { + properties: { + score_text: { + type: 'object', + 'x-style-managed': true, + properties: { font: { type: 'string' }, text_color: { type: 'array' } }, + }, + layout: { + type: 'object', + properties: { + score_text: { type: 'object', properties: { x_offset: {}, y_offset: {} } }, + possession: { type: 'object', properties: { x_offset: {}, y_offset: {} } }, + show_logo: { type: 'boolean', default: true }, + }, + }, + }, +}; + +const columns = columnsFor(schema); +const byKey = {}; +columns.forEach(function (c) { byKey[c.key] = c; }); + +ok('an element style field still gets its own column', byKey.font && byKey.font.where === 'element'); +ok('a layout-only object key still contributes its x/y columns', + byKey.x_offset && byKey.x_offset.where === 'layout'); +ok('a layout-only leaf key gets a column keyed to itself', + !!byKey.show_logo); +ok('...marked as a layout-leaf column, not a shared sub-field', + byKey.show_logo && byKey.show_logo.where === 'layout-leaf'); +ok('nothing is duplicated', columns.length === new Set(columns.map(function (c) { return c.key; })).size); + +// A schema with no leaf-valued layout key must not gain a phantom column -- +// this is the common case (every existing scoreboard) and must be unaffected. +const noLeaf = { + properties: { + score_text: { + type: 'object', 'x-style-managed': true, + properties: { font: { type: 'string' } }, + }, + layout: { + type: 'object', + properties: { score_text: { type: 'object', properties: { x_offset: {} } } }, + }, + }, +}; +ok('a schema with only object-shaped layout keys gets no layout-leaf column', + columnsFor(noLeaf).every(function (c) { return c.where !== 'layout-leaf'; })); + +if (failures) { console.log(`\n${failures} failure(s)`); process.exit(1); } +console.log('\nall checks passed'); diff --git a/web_interface/static/v3/js/widgets/style-editor.js b/web_interface/static/v3/js/widgets/style-editor.js index 24d77feb..ca28a7de 100644 --- a/web_interface/static/v3/js/widgets/style-editor.js +++ b/web_interface/static/v3/js/widgets/style-editor.js @@ -382,9 +382,12 @@ /** * The columns a table needs, derived from the schema rather than fixed. * - * Two sources: the sub-fields elements declare (font, font_size, - * text_color, visible, align) and the sub-fields their layout blocks - * declare (x_offset, y_offset, scale). + * Three sources: the sub-fields elements declare (font, font_size, + * text_color, visible, align), the sub-fields their layout blocks + * declare (x_offset, y_offset, scale), and a layout-only key whose own + * value *is* the field to set -- a plain "show_logo" toggle has no x/y + * object underneath it, so it gets a column keyed to itself rather than + * to a shared sub-field name. */ function columnsFor(schema) { var props = schema.properties || {}; @@ -396,8 +399,17 @@ elementKeys(schema).forEach(function (key) { Object.keys(ownObj(props, key).properties || {}).forEach( function (f) { seen.set(f, 'element'); }); - Object.keys(ownObj(layoutProps, key).properties || {}).forEach( - function (f) { seen.set(f, 'layout'); }); + var layoutEntry = own(layoutProps, key); + if (layoutEntry && typeof layoutEntry === 'object' && layoutEntry.properties) { + Object.keys(layoutEntry.properties).forEach( + function (f) { seen.set(f, 'layout'); }); + } else if (layoutEntry && !seen.has(key)) { + // layout-only and a leaf: nothing else will share this + // column, but leaving it out drops the field's only control + // the moment the wholesale `layout` claim removes its + // fallback (#569 review). + seen.set(key, 'layout-leaf'); + } }); var known = COLUMN_ORDER.filter(function (f) { return seen.get(f); }); // Anything the schema declares that this file has never heard of @@ -483,8 +495,13 @@ } opts.columns.forEach(function (col) { + var isLeaf = col.where === 'layout-leaf'; var inLayout = col.where === 'layout'; - var prop = inLayout ? own(axes, col.key) : own(props, col.key); + // A leaf column only applies to the one row named after it -- + // every other row leaves it blank, same as an element that + // doesn't declare a shared sub-field. + var prop = isLeaf ? (col.key === key ? own(layoutProps, key) : null) + : inLayout ? own(axes, col.key) : own(props, col.key); var cell; if (!prop) { // This element does not declare that field; keep the grid @@ -492,13 +509,14 @@ row.appendChild(el('span')); return; } - var path = inLayout ? ['layout', key, col.key] : [key, col.key]; - var base = inLayout ? opts.layoutPrefix + '.' + key + var path = isLeaf ? ['layout', key] + : inLayout ? ['layout', key, col.key] : [key, col.key]; + var base = (isLeaf || inLayout) ? opts.layoutPrefix + '.' + key : opts.prefix + '.' + key; var node = control({ key: col.key, prop: prop, - name: base + '.' + col.key, + name: isLeaf ? base : base + '.' + col.key, current: effective(value, path, prop, optional), optional: optional, fonts: opts.fonts,