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, ' ') }; }); }