From 75e6b2b20c306aeec1cbe5869f6c46b627f5eb1f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 23:38:37 +0000 Subject: [PATCH 1/4] fix(web): style editor no longer drops layout-only fields it never rendered CodeRabbit flagged elementKeys() in style-editor.js: render() claims the whole customization.layout child as the widget's own (removing it from the generic fallback renderer, since posting the same offset twice is worse), but elementKeys() only listed keys that also have their own top-level style block. A hand-written schema can put a key under layout that never got one -- a logo, a timeout indicator, a possession arrow with a position but no font or colour -- and that key's only control silently disappeared: no row in the style editor's table (elementKeys never listed it) and no fallback section either (layout was removed wholesale). elementKeys() now appends any layout-declared key not already covered by a style element, so table() renders a row for it (layout columns only, no style columns) and the wholesale layout ownership claim stays truthful. Verified against current code before fixing. New regression test (test/js/unit/test_style_editor_element_keys.js, following this repo's existing eval-extraction pattern for testing widget JS without a browser) fails against the reverted function and passes with the fix; added to run_all.js and the suite table in test/js/README.md. Full pytest suite: 4887 passed, 62 skipped, 2 failed -- both the pre-existing Europe/Kiev/Asia/Calcutta tzdata-alias gap on this sandbox, identical on origin/main, unrelated to this change. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Dpg3HLWohdCUdzz2QNHanm --- test/js/README.md | 1 + test/js/run_all.js | 2 +- .../js/unit/test_style_editor_element_keys.js | 109 ++++++++++++++++++ .../static/v3/js/widgets/style-editor.js | 14 ++- 4 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 test/js/unit/test_style_editor_element_keys.js diff --git a/test/js/README.md b/test/js/README.md index 023e0d9a..01c1e7a4 100644 --- a/test/js/README.md +++ b/test/js/README.md @@ -36,6 +36,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 | | `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 b10a9827..562d2548 100755 --- a/test/js/run_all.js +++ b/test/js/run_all.js @@ -15,7 +15,7 @@ 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_html_escaping.js', 'unit/test_style_editor_element_keys.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_element_keys.js b/test/js/unit/test_style_editor_element_keys.js new file mode 100644 index 00000000..6b0ab09c --- /dev/null +++ b/test/js/unit/test_style_editor_element_keys.js @@ -0,0 +1,109 @@ +// Regression test for style-editor.js's elementKeys(), extracted verbatim +// from the shipped widget (own/ownObj/elementKeys have no DOM dependency) so +// the test can't drift from the real implementation. +// +// render() in style-editor.js claims the whole `layout` child of a +// customization schema as the widget's own -- removing it from the generic +// fallback renderer entirely, because posting the same offset twice from +// two controls is worse than owning slightly too much. That is only safe +// when the style editor's own table actually renders a row for every key +// declared under `layout`. A hand-written schema can put a key under +// `layout` that never got its own top-level style block -- a logo, a +// timeout indicator, a possession arrow with a position but no font or +// colour -- and elementKeys() is what table() uses to decide which rows to +// draw. Miss one there and the fallback section that used to show it +// disappears with nothing replacing it. +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 titleOf(schema, key)'; +const end = src.indexOf(endMark); +if (start === -1 || end === -1) { + console.error('FAIL: could not locate own()/elementKeys() in style-editor.js -- slice markers need updating'); + process.exit(1); +} +eval(src.slice(start, end)); // defines own/ownObj/elementKeys + +let failures = 0; +function ok(desc, cond) { + if (cond) { console.log(`ok - ${desc}`); } + else { console.log(`FAIL - ${desc}`); failures++; } +} + +// A hand-written schema: score_text has a real style block (and a layout +// offset), home_logo has ONLY a layout offset + scale -- no font/colour of +// its own, the shape _element_block_from_spec never gives a plain +// offsets-only element -- and "possession" lives only under layout, with no +// sibling block in customization.properties at all (the case a hand-authored +// schema can produce that the compact x-style-elements expander cannot). +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: {} } }, + }, + }, + }, +}; + +const keys = elementKeys(schema); +ok('the declared style element is included', keys.indexOf('score_text') !== -1); +ok('a layout-only key with no style block of its own is still included', + keys.indexOf('possession') !== -1); +ok('layout and modes containers themselves are never treated as elements', + keys.indexOf('layout') === -1 && keys.indexOf('modes') === -1); +ok('nothing is duplicated', keys.length === new Set(keys).size); + +// A schema with no layout block at all must behave exactly as before -- +// this is the overwhelmingly common case and must not gain a phantom row. +const noLayout = { + properties: { + score_text: { + type: 'object', + 'x-style-managed': true, + properties: { font: { type: 'string' } }, + }, + }, +}; +ok('a schema without a layout block is unaffected', + elementKeys(noLayout).join(',') === 'score_text'); + +// Every layout key already covered by a style element must not be listed +// twice (order: declared elements first, then layout-only extras). +const covered = { + properties: { + score_text: { + type: 'object', 'x-style-managed': true, + properties: { font: { type: 'string' } }, + }, + home_logo: { + type: 'object', 'x-style-managed': true, + properties: {}, + }, + layout: { + type: 'object', + properties: { + score_text: { type: 'object', properties: { x_offset: {} } }, + home_logo: { type: 'object', properties: { x_offset: {}, scale: {} } }, + }, + }, + }, +}; +ok('a layout key matching an existing element is not appended again', + elementKeys(covered).join(',') === 'score_text,home_logo'); + +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 29d0b359..24d77feb 100644 --- a/web_interface/static/v3/js/widgets/style-editor.js +++ b/web_interface/static/v3/js/widgets/style-editor.js @@ -159,7 +159,19 @@ }); // Nothing marked means the schema never went through expansion, so // fall back to the shape test rather than rendering an empty table. - return managed.length ? managed : shaped; + var keys = managed.length ? managed : shaped; + + // A hand-written layout block can offset something that never got + // a style block of its own -- a logo, a timeout indicator, a + // possession arrow with a position but no font, colour or size to + // set. render() claims the whole `layout` child as ours regardless + // (posting every covered offset twice from two controls is worse), + // so a key that only lives under layout still needs a row here, or + // it loses its only control the moment that fallback section is + // dropped. + var layoutKeys = Object.keys(ownObj(props, 'layout').properties || {}); + var extra = layoutKeys.filter(function (k) { return keys.indexOf(k) === -1; }); + return keys.concat(extra); } function titleOf(schema, key) { From 324a7eac877998abfe4e3385374ef6e8e1d66e33 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 03:36:14 +0000 Subject: [PATCH 2/4] 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, From 03d784e0edd415fd92eca1c6518ee5169c6e5562 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 04:27:40 +0000 Subject: [PATCH 3/4] fix(web): keep layout-leaf style-editor columns distinct from name collisions columnsFor() keyed a layout-only leaf field's column by its bare field name. If an unrelated element's style block or another element's layout axis block happened to declare a sub-field with that same name, the `!seen.has(key)` guard skipped creating the leaf's column, silently dropping its only control again -- the same failure the leaf-column fix was meant to close, just reached through a name collision (CodeRabbit review on 324a7ea). Key layout-leaf columns under a namespaced id so they can never be shadowed by an unrelated column sharing their name. Co-Authored-By: Claude Opus 5 --- test/js/README.md | 1 + test/js/run_all.js | 3 +- ...test_style_editor_layout_leaf_collision.js | 76 +++++++++++++++++++ .../static/v3/js/widgets/style-editor.js | 18 +++-- 4 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 test/js/unit/test_style_editor_layout_leaf_collision.js diff --git a/test/js/README.md b/test/js/README.md index c5f4013a..3fa449a3 100644 --- a/test/js/README.md +++ b/test/js/README.md @@ -38,6 +38,7 @@ nothing is listening, so it stays useful in a bare checkout. | `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 | +| `unit/test_style_editor_layout_leaf_collision.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only leaf key still gets its own column even when its name collides with an unrelated element's style sub-field or another layout axis's sub-field | | `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 a5b0c31a..99ddaa01 100755 --- a/test/js/run_all.js +++ b/test/js/run_all.js @@ -16,7 +16,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_style_editor_layout_leaf_columns.js']; + 'unit/test_style_editor_layout_leaf_columns.js', + 'unit/test_style_editor_layout_leaf_collision.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_collision.js b/test/js/unit/test_style_editor_layout_leaf_collision.js new file mode 100644 index 00000000..c128f242 --- /dev/null +++ b/test/js/unit/test_style_editor_layout_leaf_collision.js @@ -0,0 +1,76 @@ +// Regression test for style-editor.js's columnsFor(), extracted verbatim +// from the shipped widget (see test_style_editor_layout_leaf_columns.js for +// the base case this builds on). +// +// A layout-only leaf key (e.g. a bare "scale" toggle directly under +// `layout`, no x/y object underneath) used to be keyed in the internal +// `seen` map by its own bare field name. If some *other* element's style +// block, or another element's layout axis block, happened to declare a +// sub-field with that exact same name (e.g. "scale" is also a real +// COLUMN_ORDER axis name most elements use), the `!seen.has(key)` guard +// skipped creating the leaf's column -- it silently reused the existing +// 'element'/'layout' column instead. That column's row binding in +// elementRow() then looked the field up under the wrong schema location for +// the leaf's own row, found nothing, and rendered a blank cell: the same +// "silently disappears" bug the leaf-column fix was meant to close, just +// reached through a name collision instead of a missing column altogether. +// This pins the fix: layout-leaf columns are keyed under a namespaced id so +// they can never be shadowed by an unrelated column sharing their name. +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++; } +} + +// `possession` is a real style element whose layout axis block declares a +// "scale" sub-field, which seeds seen.set('scale', 'layout'). `timeout_flag` +// is layout-only and a bare leaf valued directly under layout, but its own +// key is *also* "scale" -- an unrelated collision with that same name. +const schema = { + properties: { + possession: { + type: 'object', + 'x-style-managed': true, + properties: { font: { type: 'string' } }, + }, + scale: { + // The collision: a second element literally named "scale", entirely + // unrelated to the layout-only leaf below sharing that string. + type: 'boolean', + }, + layout: { + type: 'object', + properties: { + possession: { type: 'object', properties: { scale: { type: 'number' } } }, + scale: { type: 'boolean', default: true }, + }, + }, + }, +}; + +const columns = columnsFor(schema); +const leafColumns = columns.filter(function (c) { return c.where === 'layout-leaf'; }); + +ok('the colliding layout axis sub-field still gets its own column', + columns.some(function (c) { return c.key === 'scale' && c.where === 'layout'; })); +ok('the layout-only leaf key still gets its own column despite the name collision', + leafColumns.length === 1 && leafColumns[0].key === 'scale'); +ok('the leaf column is distinct from the colliding element/layout column', + columns.filter(function (c) { return c.key === 'scale'; }).length === 2); + +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 ca28a7de..da46094a 100644 --- a/web_interface/static/v3/js/widgets/style-editor.js +++ b/web_interface/static/v3/js/widgets/style-editor.js @@ -403,12 +403,18 @@ if (layoutEntry && typeof layoutEntry === 'object' && layoutEntry.properties) { Object.keys(layoutEntry.properties).forEach( function (f) { seen.set(f, 'layout'); }); - } else if (layoutEntry && !seen.has(key)) { + } else if (layoutEntry) { // 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'); + // fallback (#569 review). Keyed under a namespaced id, not + // the bare field name -- an unrelated element's own style + // sub-field or another layout axis can share this exact + // name, and reusing that shared slot would give this row a + // column typed 'element'/'layout' that it doesn't declare, + // dropping the control right back out (#569 follow-up). + var leafId = 'layout-leaf:' + key; + if (!seen.has(leafId)) { seen.set(leafId, 'layout-leaf'); } } }); var known = COLUMN_ORDER.filter(function (f) { return seen.get(f); }); @@ -418,10 +424,12 @@ return COLUMN_ORDER.indexOf(f) === -1; }).sort(); return known.concat(extra).map(function (f) { + var isLeafId = f.indexOf('layout-leaf:') === 0; + var fieldKey = isLeafId ? f.slice('layout-leaf:'.length) : f; return { - key: f, + key: fieldKey, where: seen.get(f), - label: own(COLUMN_LABELS, f) || f.replace(/_/g, ' ') + label: own(COLUMN_LABELS, fieldKey) || fieldKey.replace(/_/g, ' ') }; }); } From 6ad3ff5abaf6bb84a0f54310d558318219517512 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 05:35:24 +0000 Subject: [PATCH 4/4] fix(web): CSS.escape() the owned key before it becomes a selector container.dataset.ownedKeys round-trips schema property keys through a DOM dataset attribute, and the takeover handoff spliced each one straight into '[data-child-key="' + k + '"]' with no escaping -- inconsistent with this codebase's own convention elsewhere (plugin-file-manager.js, app-shell.js's escapeCssSelector) for building a selector from a dynamic value. A key containing a quote or backslash would break the selector or be steerable; Codacy's static analysis flagged this pattern (1 high ErrorProne finding on PR #569, current head at the time) as a new issue, though its dashboard is unreachable from this sandbox (egress to app.codacy.com is blocked) and the check-run API returned no detail text -- verified and fixed by reading the diff directly rather than the tool's own description. Added a source-assertion regression test alongside this file's existing ones (this behavior lives in an inline script no Python test executes). Full suite: 4888 passed, 62 skipped, 2 failed -- both the pre-existing Europe/Kiev/Asia/Calcutta tzdata-alias gap in this sandbox, identical on origin/main, unrelated to this change. Co-Authored-By: Claude Opus 5 --- test/web_interface/test_style_editor_takeover.py | 10 ++++++++++ web_interface/templates/v3/partials/plugin_config.html | 7 ++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/test/web_interface/test_style_editor_takeover.py b/test/web_interface/test_style_editor_takeover.py index 4831fb97..00ea9791 100644 --- a/test/web_interface/test_style_editor_takeover.py +++ b/test/web_interface/test_style_editor_takeover.py @@ -88,6 +88,16 @@ class TestItTakesOverOnlyItsOwnBlocks: entirely styling looks exactly as it did before.""" assert "!fallback.querySelector('[data-child-key]')" in source + def test_the_owned_key_is_escaped_before_it_becomes_a_selector(self, source): + """ownedKeys comes back off a dataset attribute, not a literal -- + splicing it into '[data-child-key="' + k + '"]' unescaped breaks (or + is steerable) on a key containing a quote or backslash. The rest of + this codebase (plugin-file-manager.js, app-shell.js) always routes a + dynamic value through CSS.escape() before it lands in a selector.""" + assert "CSS.escape(k)" in source, ( + "an owned key must be CSS.escape()'d before being interpolated " + "into the data-child-key attribute selector") + class TestTheSchemaSaysWhichBlocksAreStyling: def test_adopted_blocks_are_marked_and_others_are_not(self): diff --git a/web_interface/templates/v3/partials/plugin_config.html b/web_interface/templates/v3/partials/plugin_config.html index 8d4752a1..ae3263f4 100644 --- a/web_interface/templates/v3/partials/plugin_config.html +++ b/web_interface/templates/v3/partials/plugin_config.html @@ -147,8 +147,13 @@ var owned = (container.dataset.ownedKeys || '') .split(',').filter(function (k) { return k; }); owned.forEach(function (k) { + // CSS.escape: k is a schema property key read + // back off dataset.ownedKeys, not a literal -- + // building the attribute selector by raw + // concatenation would break (or be steerable) + // on a key containing a quote or backslash. var node = fallback.querySelector( - '[data-child-key="' + k + '"]'); + '[data-child-key="' + CSS.escape(k) + '"]'); if (node) { node.remove(); } }); // Nothing left but the section heading, or a widget