diff --git a/CLAUDE.md b/CLAUDE.md index f84e1e7..3ae23e7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,6 +160,20 @@ controls and handlers a person does; it is NOT a second rendering path. follows it, hits an error it cannot interpret, and improvises — the failure this interface exists to prevent. Keep examples complete and runnable, not fragments. +- Quiescence uses a canvas SIGNATURE (type, position, size, angle, text of + every object), not an object count. A late image load can replace an object + without changing the count — `addLogo()` removes the old logo and adds a + flattened one — so a count-based wait returns mid-change. Three independent + external reviews flagged the count version, two as critical. +- Everything `objects()` reports as `editable: false` — canvas surface, + background photo, organisation logo — is refused by every mutating method, + not just `remove()`. `bringToFront(0)` used to cover the whole image in flat + green. +- `export()` takes its default DPI from the current template, as the download + button does. Templates do not all use 200. +- Order of operations: `resize()` and `rotate()` BEFORE `place()`. Fabric + rotates around the centre by default, so rotating after placing moves the + element. - `lastAdded()` exists because `canvas.getObjects()` ends with the organisation LOGO, not with the element just added — `addLogo()` and the QR handler both call `bringLogoToFront()`. Anything that operates on "the last object" moves diff --git a/e2e/review-findings.spec.js b/e2e/review-findings.spec.js new file mode 100644 index 0000000..43ee698 --- /dev/null +++ b/e2e/review-findings.spec.js @@ -0,0 +1,158 @@ +import { test, expect } from '@playwright/test'; + +/** + * Regression tests for the findings of the external review (Claude Opus 5, + * GPT-5.6, Antigravity). Each test pins one defect they found, so it cannot + * come back quietly. + */ + +const TINY_PNG = + 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAIAAAACCAYAAABytg0kAAAAFUlEQVR42mP8z8BQz0AEYBxVSF+FAP5FDvcfRYWgAAAAAElFTkSuQmCC'; + +test.describe('Review findings', () => { + test.beforeEach(async ({ page }) => { + await page.goto('/'); + await page.waitForFunction(() => typeof window.Bildgenerator !== 'undefined'); + page.on('dialog', (dialog) => dialog.accept()); + }); + + test('insideMargin checks all four edges, not just the origin', async ({ page }) => { + const result = await page.evaluate(async () => { + const B = window.Bildgenerator; + await B.setTemplate('artikel_23'); + await B.setLogoEnabled(false); + await B.addText('Ein bewusst sehr langer Text der nicht hineinpasst'); + await B.resize(3); // far wider than the canvas + await B.place('top-left'); // origin is AT the margin + const wide = B.objects().find((o) => o.type === 'text'); + + await B.resize(0.3); + await B.place('center'); + const fitting = B.objects().find((o) => o.type === 'text'); + return { wide, fitting, canvasWidth: canvas.width }; + }); + + // Origin sits on the margin, so a left/top-only check called this "inside". + expect(result.wide.left).toBeLessThanOrEqual(result.wide.width); + expect(result.wide.width).toBeGreaterThan(result.canvasWidth); + expect(result.wide.insideMargin).toBe(false); + expect(result.fitting.insideMargin).toBe(true); + }); + + test('place, resize and rotate accept an index from objects()', async ({ page }) => { + const result = await page.evaluate(async () => { + const B = window.Bildgenerator; + await B.setTemplate('feed_post_45'); + await B.setLogoEnabled(false); + await B.addText('Erster'); + await B.addShape('cross'); + + // The first text is no longer the tracked element. + const text = B.objects().find((o) => o.type === 'text'); + // Position LAST: fabric rotates around the centre by default + // (centeredRotation), so rotating after placing moves the element. + await B.rotate(10, { target: text.index }); + await B.resize(0.5, { target: text.index }); + await B.place('top-left', { target: text.index }); + + const after = B.objects().find((o) => o.type === 'text'); + return { angle: after.angle, left: after.left, margin: B.protectiveMargin() }; + }); + + // llms.txt documents { target: index }; before the fix a number was used + // as the object itself and silently did nothing useful. + expect(result.angle).toBe(10); + expect(result.left).toBeCloseTo(result.margin, 0); + }); + + test('the logo, background and canvas surface cannot be moved or restyled', async ({ page }) => { + const errors = await page.evaluate(async (png) => { + const B = window.Bildgenerator; + await B.setTemplate('feed_post_45'); + await B.setBackground(png); + await B.setLogo('HERZOGENBURG'); + + const out = {}; + for (const role of ['logo', 'background', 'canvas']) { + const item = B.objects().find((o) => o.role === role); + out[role] = {}; + for (const method of ['place', 'resize', 'rotate', 'bringToFront', 'update']) { + try { + if (method === 'place') await B.place('top-left', { target: item.index }); + else if (method === 'resize') await B.resize(2, { target: item.index }); + else if (method === 'rotate') await B.rotate(45, { target: item.index }); + else if (method === 'bringToFront') await B.bringToFront(item.index); + else await B.update({ color: '#FFED00' }, { target: item.index }); + out[role][method] = null; + } catch (e) { + out[role][method] = e.message; + } + } + } + return out; + }, TINY_PNG); + + // objects() advertises these as editable:false; every mutating method + // must honour that, not just remove(). + for (const role of ['logo', 'background', 'canvas']) { + for (const method of ['place', 'resize', 'rotate', 'bringToFront', 'update']) { + expect(errors[role][method], `${method}() on ${role}`).toBeTruthy(); + } + } + }); + + test('export uses the DPI of the template, as the download button does', async ({ page }) => { + const result = await page.evaluate(async () => { + const B = window.Bildgenerator; + const out = {}; + for (const name of ['facebook_header', 'a6']) { + await B.setTemplate(name); + const exported = await B.export(); + const template = window.TemplateConstants.getTemplate(name); + out[name] = { got: exported.dpi, expected: template.dpi }; + } + return out; + }); + + // Templates do not all use 200; hardcoding it produced a different + // resolution than the same template gives a person. + expect(result.facebook_header.got).toBe(result.facebook_header.expected); + expect(result.a6.got).toBe(result.a6.expected); + }); + + test('text and shapes stay behind the organisation logo', async ({ page }) => { + const result = await page.evaluate(async () => { + const B = window.Bildgenerator; + await B.setTemplate('feed_post_45'); + await B.setLogo('HERZOGENBURG'); + + const order = []; + await B.addText('Text'); + order.push(B.objects().map((o) => o.role).pop()); + await B.addShape('pinkCircle'); + order.push(B.objects().map((o) => o.role).pop()); + await B.addShape('cross'); + order.push(B.objects().map((o) => o.role).pop()); + return order; + }); + + // bringLogoToFront() was wired for images and QR codes but not for text, + // the Störer or the Wahlkreuz — those rendered over the logo. + expect(result).toEqual(['logo', 'logo', 'logo']); + }); + + test('quiescence notices a change that leaves the object count untouched', async ({ page }) => { + const signatures = await page.evaluate(async () => { + const B = window.Bildgenerator; + await B.setTemplate('feed_post_45'); + await B.setLogoEnabled(false); + await B.addShape('pinkCircle'); + const before = B._canvasSignature(); + await B.place('top-left'); + return { before, after: B._canvasSignature() }; + }); + + // Counting objects cannot see a move or a replacement; the signature can. + expect(signatures.after).not.toBe(signatures.before); + }); +}); diff --git a/llms.txt b/llms.txt index 900301b..25100da 100644 --- a/llms.txt +++ b/llms.txt @@ -49,7 +49,7 @@ weitere Freigabe. Seite laden, warten bis `window.Bildgenerator` existiert, dann aufrufen. Alle Methoden sind asynchron und liefern erst zurück, wenn das Canvas seine -Ruhelage erreicht hat. Aktuelle Schnittstellenversion: `Bildgenerator.VERSION === 9`. +Ruhelage erreicht hat. Aktuelle Schnittstellenversion: `Bildgenerator.VERSION === 10`. ```js const bild = await window.Bildgenerator.render({ @@ -393,6 +393,11 @@ Bewährte Verteilungen: prüft aber nicht, ob zwei Elemente einander überdecken. Setz sie auf gegenüberliegende Felder. +**Reihenfolge:** Erst `resize()` und `rotate()`, dann `place()`. Gedreht wird +um den Mittelpunkt, dadurch verschiebt sich die Position — wer vorher +positioniert, findet das Element hinterher woanders. `render()` hält diese +Reihenfolge von selbst ein. + ### Störer Der magentafarbene Kreis (`addShape('pinkCircle')`) ist der Störer aus dem CD. @@ -459,7 +464,15 @@ console.log(B.objects()); | `text` | der Inhalt, bei Textelementen | | `color`, `left`, `top`, `width`, `height`, `angle`, `scale` | Zustand | | `editable` | `false` bei Logo, Hintergrund und Grundfläche | -| `insideMargin` | ob das Element die Schutzzone einhält | +| `insideMargin` | ob das Element **vollständig** in der Schutzzone liegt, alle vier Kanten | + +Elemente mit `editable: false` lassen sich nicht verschieben, skalieren, +drehen, umfärben oder löschen — die Aufrufe werfen einen Fehler mit dem +Hinweis, was stattdessen zu tun ist. Für das Logo etwa `setLogoEnabled(false)` +oder `setLogo()`, für den Hintergrund ein neues `setBackground()`. + +Alle ändernden Aufrufe nehmen `{ target: index }` und wirken dann auf dieses +Element statt auf das zuletzt hinzugefügte. **Verlass dich nicht auf die Reihenfolge.** Das letzte Element in der Liste ist das Organisationslogo, nicht das, was du zuletzt hinzugefügt hast — der diff --git a/resources/js/api.js b/resources/js/api.js index fdb504d..8cf340c 100644 --- a/resources/js/api.js +++ b/resources/js/api.js @@ -31,7 +31,7 @@ * breakage surfaces in e2e/api.spec.js instead of in somebody else's script. */ const Bildgenerator = { - VERSION: 9, + VERSION: 10, /** The object the most recent add* call put on the canvas. */ _lastAdded: null, @@ -309,10 +309,13 @@ const Bildgenerator = { async place(position, options) { const opts = options || {}; const objects = canvas.getObjects(); - const target = opts.target || this._lastAdded; + const target = opts.target !== undefined + ? this._resolveTarget(opts.target) + : this._lastAdded; if (!target || !objects.includes(target)) { throw new Error("place() needs an element that was added first"); } + this._assertEditable(target, "place"); const margin = this.protectiveMargin(); const width = target.getScaledWidth(); @@ -415,8 +418,11 @@ const Bildgenerator = { */ async resize(factor, options) { const opts = options || {}; - const target = opts.target || this.lastAdded(); + const target = opts.target !== undefined + ? this._resolveTarget(opts.target) + : this.lastAdded(); if (!target) throw new Error("resize() needs an element that was added first"); + this._assertEditable(target, "resize"); if (typeof factor !== "number" || !(factor > 0)) { throw new Error("resize() needs a positive number"); } @@ -458,8 +464,11 @@ const Bildgenerator = { */ async rotate(degrees, options) { const opts = options || {}; - const target = opts.target || this.lastAdded(); + const target = opts.target !== undefined + ? this._resolveTarget(opts.target) + : this.lastAdded(); if (!target) throw new Error("rotate() needs an element that was added first"); + this._assertEditable(target, "rotate"); if (typeof degrees !== "number" || !isFinite(degrees)) { throw new Error("rotate() needs a number of degrees"); } @@ -546,7 +555,15 @@ const Bildgenerator = { height: Math.round(o.getScaledHeight()), angle: Math.round(o.angle || 0), scale: Math.round((o.scaleX || 1) * 1000) / 1000, - insideMargin: o.left >= margin - 0.5 && o.top >= margin - 0.5, + // All four edges, not just the origin. An element wider or + // taller than the usable area sits at the margin on the left + // and still runs off the right — reporting that as "inside" + // is worse than not reporting at all. + insideMargin: + o.left >= margin - 0.5 && + o.top >= margin - 0.5 && + o.left + o.getScaledWidth() <= canvas.width - margin + 0.5 && + o.top + o.getScaledHeight() <= canvas.height - margin + 0.5, editable: o !== logoObject && o !== contentImage && o.type !== "rect", }; }); @@ -581,6 +598,7 @@ const Bildgenerator = { const target = this._resolveTarget( opts.target !== undefined ? opts.target : this.lastAdded() ); + this._assertEditable(target, "update"); const spec = changes || {}; if (spec.text !== undefined) { @@ -632,15 +650,25 @@ const Bildgenerator = { if (target === contentImage) { throw new Error("The background image cannot be removed; set a different one"); } + if (target === contentRect) { + throw new Error("The canvas surface cannot be removed"); + } canvas.remove(target); if (this._lastAdded === target) this._lastAdded = null; canvas.renderAll(); return this; }, - /** Move an element to the front of the stack. */ + /** + * Move an element to the front of the stack. + * + * The UI's own handler guards contentImage and contentRect + * (event-handlers.js) — bringing the canvas surface to the front would + * cover the entire image in flat green. + */ async bringToFront(which) { const target = this._resolveTarget(which !== undefined ? which : this.lastAdded()); + this._assertEditable(target, "bringToFront"); canvas.bringToFront(target); // The organisation logo is meant to stay on top. CanvasUtils.bringLogoToFront(); @@ -648,6 +676,34 @@ const Bildgenerator = { return this; }, + /** + * Refuse to move, scale, rotate or restyle the things objects() reports as + * editable:false — the plain canvas rectangle, the background photo and + * the organisation logo. + * + * The UI protects these through its own handlers; the facade advertised + * them as off-limits in objects() but let every other method touch them. + * Dragging the logo out of its position or scaling the background breaks + * the brand, and the caller would have no way to put it back. + */ + _assertEditable(target, what) { + if (target === this._logoObject()) { + throw new Error( + `${what}() will not touch the organisation logo — ` + + `use setLogoEnabled(false) or setLogo() instead` + ); + } + if (target === contentImage) { + throw new Error( + `${what}() will not touch the background image — ` + + `use setBackground() to replace it` + ); + } + if (target === contentRect) { + throw new Error(`${what}() will not touch the canvas surface`); + } + }, + _resolveTarget(which) { if (which && typeof which === "object") return which; if (typeof which === "number") { @@ -676,10 +732,15 @@ const Bildgenerator = { */ async export(options) { const opts = options || {}; + // The download button reads the DPI from the template + // (event-handlers.js); a hardcoded 200 silently produced a different + // resolution than the same template yields for a person. + const template = TemplateConstants.getCurrentTemplate(); + const defaultDpi = (template && template.dpi) || 200; const result = await CanvasUtils.exportCanvas( opts.format || "png", typeof opts.quality === "number" ? opts.quality : 1, - typeof opts.dpi === "number" ? opts.dpi : 200 + typeof opts.dpi === "number" ? opts.dpi : defaultDpi ); const scale = result.actualDPI / 72; return { @@ -922,14 +983,43 @@ const Bildgenerator = { * canvas state rather than events — addLogo() alone is two async image * loads deep — so quiescence is the only honest completion signal. */ - async _waitUntilQuiet(settleMs = 400, timeout = 15000) { + /** + * A cheap fingerprint of everything the canvas currently shows. + * + * Counting objects is not enough. A late image load can replace an object + * or move one without changing the count — addLogo() removes the old logo + * and adds the flattened one, and replaceCanvas() re-adds a logo whose + * geometry only settles once both image decodes finish. The count is + * identical before and after; the picture is not. + */ + _canvasSignature() { + if (!canvas) return ""; + return canvas.getObjects().map(function (o) { + return [ + o.type, + Math.round(o.left), + Math.round(o.top), + Math.round(o.getScaledWidth()), + Math.round(o.getScaledHeight()), + Math.round(o.angle || 0), + o.type === "text" ? o.text : "", + ].join(","); + }).join("|"); + }, + + /** + * Wait until the canvas stops changing. The app offers no completion + * signal — addLogo() alone is two async image decodes deep — so stability + * of the rendered state is the only honest one available. + */ + async _waitUntilQuiet(settleMs = 500, timeout = 15000) { const started = Date.now(); - let last = -1; + let last = null; let stableSince = 0; while (Date.now() - started < timeout) { - const count = canvas ? canvas.getObjects().length : -1; - if (count !== last) { - last = count; + const signature = this._canvasSignature(); + if (signature !== last) { + last = signature; stableSince = Date.now(); } else if (Date.now() - stableSince >= settleMs) { return; diff --git a/resources/js/event-handlers.js b/resources/js/event-handlers.js index e50fd7c..b166c00 100644 --- a/resources/js/event-handlers.js +++ b/resources/js/event-handlers.js @@ -190,6 +190,10 @@ const EventHandlerUtils = { text.setCoords(); canvas.setActiveObject(text); updateScale(text); + // Keep the organisation logo on top. Without this a headline + // placed near the bottom covers it, which the other add handlers + // already guard against. + CanvasUtils.bringLogoToFront(); canvas.renderAll(); }); }, @@ -276,7 +280,9 @@ const EventHandlerUtils = { canvas.bringToFront(object); } }); - + + // ...and the organisation logo above those. + CanvasUtils.bringLogoToFront(); canvas.renderAll(); }); }, @@ -291,6 +297,9 @@ const EventHandlerUtils = { canvas.add(image); canvas.centerObject(image); canvas.bringToFront(image); + // The Wahlkreuz is brought to the very front above, which + // would otherwise put it over the organisation logo. + CanvasUtils.bringLogoToFront(); } ); });