From 87115b28e8aa4d955adc6534103d4cf1fb15485b Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sat, 22 Aug 2026 21:38:17 +0200 Subject: [PATCH] fix(gallery): give masonry tiles their real shape back (#1130, #1131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent causes of the same symptom — an aspect-ratio layout that does not lay anything out. gallery-premium discarded the tile height MasonryPhotoAlbum computed from photos.width/height and set height:auto on both card and image, so the rendered shape came from the intrinsic ratio of whatever rendition was served. With thumbnail_fit seeded 'cover' by migration 040 every rendition is square, so the layout drew identical squares and was indistinguishable from grid. The card now uses the height it is given and the stylesheet's existing height:100% applies. The bundled CSS templates pinned images to a fixed pixel height, which has specificity (0,1,1) and beats the .h-full utility (0,1,0) six of the seven layouts use. Elegant Dark is seeded is_default, so that was the out-of-the-box result for any layout other than grid/timeline. Migrations 052/053 corrected for fresh installs; 181 repairs the rows already seeded. Whitespace-tolerant because sanitizeCSS strips newlines from any template ever saved through the editor — an exact-text migration would have silently no-opped on most real installs. The height property is matched with a lookbehind so line-height/max-height/min-height are untouched, grouped selectors are handled, and nested rules are skipped rather than mis-rewritten. Both reported, measured in the live DOM, by @BraynArts. --- .../181_fix_css_template_photo_height.test.js | 242 ++++++++++++++++++ .../migrations/core/052_add_css_templates.js | 7 +- .../core/053_add_liquid_glass_templates.js | 6 +- .../core/181_fix_css_template_photo_height.js | 123 +++++++++ .../gallery/layouts/GalleryPremiumLayout.tsx | 21 +- 5 files changed, 390 insertions(+), 9 deletions(-) create mode 100644 backend/__tests__/migrations/181_fix_css_template_photo_height.test.js create mode 100644 backend/migrations/core/181_fix_css_template_photo_height.js diff --git a/backend/__tests__/migrations/181_fix_css_template_photo_height.test.js b/backend/__tests__/migrations/181_fix_css_template_photo_height.test.js new file mode 100644 index 00000000..c98a09ed --- /dev/null +++ b/backend/__tests__/migrations/181_fix_css_template_photo_height.test.js @@ -0,0 +1,242 @@ +/** + * Repairing the bundled templates' fixed image height (#1131). + * + * The risk in a migration that rewrites user-visible CSS is doing too much, + * so most of what is pinned here is what it must NOT touch: the other pixel + * heights inside the very same templates (a 1px divider, an 8px scrollbar), + * and any rule a user wrote themselves. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const migration = require('../../migrations/core/181_fix_css_template_photo_height'); + +const ELEGANT_DARK = ` +.photo-card { + border-radius: 12px; +} + +.photo-card img { + width: 100%; + height: 200px; + object-fit: cover; + transition: transform 0.3s ease; +} +`; + +const LIQUID_GLASS_DARK = ` +.gallery-page::after { + content: ''; + height: 1px; + background: linear-gradient(90deg, transparent, #fff, transparent); +} + +.photo-card img { + width: 100%; + height: 240px; + object-fit: cover; + filter: brightness(0.9); +} + +.gallery-page ::-webkit-scrollbar { + width: 8px; + height: 8px; +} + +@media (max-width: 640px) { + .photo-card img { + height: 180px; + } +} +`; + +describe('migration 181 — CSS template image height (#1131)', () => { + let knex; let tmpDir; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-mig181-')); + knex = require('knex')({ + client: 'sqlite3', + connection: { filename: path.join(tmpDir, 'db.sqlite') }, + useNullAsDefault: true, + }); + await knex.schema.createTable('css_templates', (t) => { + t.increments('id').primary(); + t.string('name'); + t.text('css_content'); + }); + }); + + afterAll(async () => { + if (knex) await knex.destroy(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(async () => { await knex('css_templates').del(); }); + + const contentOf = async (name) => + (await knex('css_templates').where({ name }).first()).css_content; + + it('relaxes the default template so the layouts h-full can win', async () => { + await knex('css_templates').insert({ name: 'Elegant Dark', css_content: ELEGANT_DARK }); + + await migration.up(knex); + + const css = await contentOf('Elegant Dark'); + expect(css).toContain('height: 100%'); + expect(css).not.toContain('height: 200px'); + // Everything else about the rule survives. + expect(css).toContain('object-fit: cover'); + expect(css).toContain('transition: transform 0.3s ease'); + }); + + it('fixes both the base rule and the mobile override of the dark glass template', async () => { + await knex('css_templates').insert({ name: 'Liquid Glass Dark', css_content: LIQUID_GLASS_DARK }); + + await migration.up(knex); + + const css = await contentOf('Liquid Glass Dark'); + expect(css).not.toContain('height: 240px'); + expect(css).not.toContain('height: 180px'); + expect(css.match(/height: 100%/g)).toHaveLength(2); + }); + + it('leaves the divider and the scrollbar alone', async () => { + await knex('css_templates').insert({ name: 'Liquid Glass Dark', css_content: LIQUID_GLASS_DARK }); + + await migration.up(knex); + + // The whole reason this matches full rule bodies rather than every + // `height: px`: these are in the same stylesheet and are correct. + const css = await contentOf('Liquid Glass Dark'); + expect(css).toContain('height: 1px'); + expect(css).toContain('width: 8px'); + expect(css).toContain('height: 8px'); + }); + + /** + * The case that forced the scope wider. `sanitizeCSS` strips control + * characters, so any template ever saved through the editor — including a + * save that only changed its name — has had every newline REMOVED. An + * exact-text migration finds nothing on those installs, is recorded as + * applied, and leaves them broken permanently. + */ + it('fixes a template that has been through the editor, newlines and all', async () => { + const { sanitizeCSS } = require('../../src/utils/cssSanitizer'); + const { sanitized } = sanitizeCSS(ELEGANT_DARK); + // Precondition: the sanitizer really did flatten it. + expect(sanitized).not.toContain('\n'); + expect(sanitized).toContain('height: 200px'); + await knex('css_templates').insert({ name: 'Saved Once', css_content: sanitized }); + + await migration.up(knex); + + const css = await contentOf('Saved Once'); + expect(css).not.toContain('200px'); + expect(css).toContain('height: 100%'); + }); + + it('relaxes a user-authored fixed height too, but only on .photo-card img', async () => { + // Deliberately broader than the seeded text — see the migration header. A + // pixel height on the image cannot be right under any of the seven + // layouts, whoever wrote it; a height anywhere else is none of our + // business. + const mine = '.photo-card img {\n height: 220px;\n}\n.hero { height: 400px; }'; + await knex('css_templates').insert({ name: 'My Own', css_content: mine }); + + await migration.up(knex); + + const css = await contentOf('My Own'); + expect(css).toContain('height: 100%'); + expect(css).not.toContain('220px'); + expect(css).toContain('.hero { height: 400px; }'); + }); + + it('does not rewrite other properties that merely end in -height', async () => { + // `line-height: 200px` contains `height: 200px` as a substring, so an + // unanchored pattern silently rewrites it — in a migration that cannot be + // undone. + const mine = [ + '.photo-card img {', + ' line-height: 200px;', + ' max-height: 300px;', + ' min-height: 14px;', + ' --tile-height: 220px;', + ' height: 200px;', + '}', + ].join('\n'); + await knex('css_templates').insert({ name: 'Adjacent Props', css_content: mine }); + + await migration.up(knex); + + const css = await contentOf('Adjacent Props'); + expect(css).toContain('line-height: 200px'); + expect(css).toContain('max-height: 300px'); + expect(css).toContain('min-height: 14px'); + expect(css).toContain('--tile-height: 220px'); + // Only the real one moved. + expect(css).toContain('height: 100%'); + expect(css).not.toMatch(/(? { + // Requiring `{` straight after `img` skipped these entirely — and the + // migration is still recorded as applied, so the template kept the bug. + const mine = '.photo-card img, .thumbnail img {\n height: 200px;\n}'; + await knex('css_templates').insert({ name: 'Grouped', css_content: mine }); + + await migration.up(knex); + + const css = await contentOf('Grouped'); + expect(css).toContain('.photo-card img, .thumbnail img {'); + expect(css).toContain('height: 100%'); + expect(css).not.toContain('200px'); + }); + + it('skips a nested rule rather than rewriting the wrong declaration', async () => { + // Valid nested CSS that passes the validator. A brace-greedy body would + // capture the inner block and rewrite the CAPTION's height, which cannot + // be undone. Leaving it untouched is the lesser evil. + const mine = '.photo-card img {\n & + .caption { height: 200px; }\n}'; + await knex('css_templates').insert({ name: 'Nested', css_content: mine }); + + await migration.up(knex); + + expect(await contentOf('Nested')).toBe(mine); + }); + + it('leaves non-pixel heights on the image alone', async () => { + const mine = '.photo-card img { height: 50vh; }\n.photo-card img { height: auto; }'; + await knex('css_templates').insert({ name: 'Relative', css_content: mine }); + + await migration.up(knex); + + expect(await contentOf('Relative')).toBe(mine); + }); + + it('is idempotent and safe on a row with no CSS', async () => { + await knex('css_templates').insert([ + { name: 'Elegant Dark', css_content: ELEGANT_DARK }, + { name: 'Empty', css_content: null }, + ]); + + await migration.up(knex); + const once = await contentOf('Elegant Dark'); + await migration.up(knex); + + expect(await contentOf('Elegant Dark')).toBe(once); + expect(await contentOf('Empty')).toBeNull(); + }); + + it('no-ops when the table does not exist yet', async () => { + await knex.schema.dropTable('css_templates'); + await expect(migration.up(knex)).resolves.toBeUndefined(); + await knex.schema.createTable('css_templates', (t) => { + t.increments('id').primary(); + t.string('name'); + t.text('css_content'); + }); + }); +}); diff --git a/backend/migrations/core/052_add_css_templates.js b/backend/migrations/core/052_add_css_templates.js index 5006a4bd..27c55667 100644 --- a/backend/migrations/core/052_add_css_templates.js +++ b/backend/migrations/core/052_add_css_templates.js @@ -77,7 +77,12 @@ const DEFAULT_CSS_TEMPLATE = `/* .photo-card img { width: 100%; - height: 200px; + /* 100%, not a fixed pixel height: every aspect-ratio layout (masonry, + justified, mosaic, gallery-premium) gives .photo-card a definite height + computed from photos.width/height, and this rule's specificity (0,1,1) + beats the .h-full utility (0,1,0) the layouts rely on. A fixed height + therefore pinned every image inside a correctly-shaped card — #1131. */ + height: 100%; object-fit: cover; transition: transform 0.3s ease; } diff --git a/backend/migrations/core/053_add_liquid_glass_templates.js b/backend/migrations/core/053_add_liquid_glass_templates.js index af19bdd0..cf3a7d51 100644 --- a/backend/migrations/core/053_add_liquid_glass_templates.js +++ b/backend/migrations/core/053_add_liquid_glass_templates.js @@ -503,7 +503,9 @@ const LIQUID_GLASS_DARK = `/* .photo-card img { width: 100%; - height: 240px; + /* See #1131: a fixed height here beats the layouts' .h-full utility and + detaches the image from its aspect-ratio-sized card. */ + height: 100%; object-fit: cover; transition: transform 0.4s ease, filter 0.4s ease; filter: brightness(0.9); @@ -639,7 +641,7 @@ const LIQUID_GLASS_DARK = `/* } .photo-card img { - height: 180px; + height: 100%; } /* Reduce animation complexity on mobile */ diff --git a/backend/migrations/core/181_fix_css_template_photo_height.js b/backend/migrations/core/181_fix_css_template_photo_height.js new file mode 100644 index 00000000..58202203 --- /dev/null +++ b/backend/migrations/core/181_fix_css_template_photo_height.js @@ -0,0 +1,123 @@ +/** + * The bundled CSS templates pinned every gallery image to a fixed pixel + * height, which broke every aspect-ratio layout (#1131). + * + * Six of the seven layouts size a tile by putting a computed pixel height on + * `.photo-card` and letting the image fill it with `h-full`. A template rule + * of `.photo-card img { height: 200px }` has specificity (0,1,1) and beats + * `.h-full` at (0,1,0), so the image detached from its card: masonry rendered + * correctly-shaped cards with a 200px image glued to the top and empty + * background below — or, where the computed card was shorter than 200px, an + * image taller than its own container. + * + * "Elegant Dark" is seeded `is_default = true`, so this was the out-of-the-box + * result for anyone choosing any layout other than grid/timeline (where a + * fixed square happens to look deliberate). + * + * Migrations 052 and 053 are corrected for fresh installs; this repairs the + * rows already seeded. Templates are referenced by `events.css_template_id` + * and read at serve time rather than copied onto the event, so fixing the row + * fixes every gallery using it. + * + * SCOPE: every `.photo-card img` rule that carries a fixed PIXEL height, in + * every template — not just the two we seeded, and not just their pristine + * copies. + * + * That is broader than it first looks, and deliberately so. It is also not the + * scope this started with: matching the exact seeded text missed every install + * where the template had ever been saved through the editor, because + * `sanitizeCSS` strips newlines. Those are the majority, and a migration that + * silently no-ops on them while being recorded as applied is worse than none. + * + * The cost is that a fixed pixel height a user wrote themselves is rewritten + * too. That is judged acceptable because there is no layout it can be right + * for: all seven give `.photo-card` a definite height and expect the image to + * fill it, so a pixel height on the image can only detach it from its card. + * Anything that is not a fixed px height — %, vh, auto — is left alone, as is + * every declaration outside a `.photo-card img` body. + */ + +/** + * Every `.photo-card img { … }` rule body, however it is spaced. + * + * Matching the exact seeded text does NOT work, and the reason is worth + * stating: `sanitizeCSS` strips all control characters (cssSanitizer.js:61), + * so the moment an admin saves a template through the editor — even only to + * rename it or toggle it — every newline is REMOVED from the stored CSS. The + * shipped `.photo-card img {\n height: 200px;` becomes + * `.photo-card img { height: 200px;`. An exact-match migration would find + * nothing on those installs, be recorded as applied, and leave the galleries + * broken with no second chance. + * + * Scoped to the rule body rather than the whole stylesheet, so the other pixel + * heights in these same templates — a 1px gradient divider, an 8px scrollbar — + * are untouched. + */ +/* + * Two details in this pattern are deliberate: + * + * * the selector part is a LIST, so `.photo-card img, .thumbnail img { … }` + * is recognised. Requiring `{` straight after `img` skipped grouped + * selectors entirely — and the migration would still be recorded as + * applied, so the template kept the bug with no second chance. + * + * * the body excludes braces, so a rule containing a NESTED block is not + * matched at all. `.photo-card img { & + .caption { height: 200px } }` is + * valid, passes the validator, and a `[^}]*` body would have captured the + * nested block and rewritten the caption's height instead. Skipping it + * means such a template keeps a fixed image height; corrupting unrelated + * declarations in a migration that cannot be undone is the worse of the + * two, and nesting does not appear in anything we ship. + */ +const PHOTO_CARD_IMG_RULE = /([^{}]*\.photo-card\s+img[^{}]*)\{([^{}]*)\}/g; + +/** + * Only a fixed PIXEL height is wrong here; %, vh, auto and the rest stay. + * + * The lookbehind is load-bearing rather than defensive: without it the pattern + * matches the TAIL of `line-height`, `max-height`, `min-height` and any custom + * property ending in `-height`, and silently rewrites those instead — in a + * migration whose down() is deliberately irreversible. + */ +const FIXED_PX_HEIGHT = /(? { + // .test() on a /g regex advances lastIndex, so it is reset on both sides + // of the check — leaving it set makes the NEXT rule start matching from an + // arbitrary offset and silently skip declarations. + FIXED_PX_HEIGHT.lastIndex = 0; + if (!FIXED_PX_HEIGHT.test(body)) return whole; + FIXED_PX_HEIGHT.lastIndex = 0; + return `${selectors}{${body.replace(FIXED_PX_HEIGHT, 'height: 100%')}}`; + }); +} + +exports.up = async function up(knex) { + if (!(await knex.schema.hasTable('css_templates'))) return; + + const rows = await knex('css_templates').select('id', 'css_content'); + let fixed = 0; + + for (const row of rows) { + const original = row.css_content; + if (!original || typeof original !== 'string') continue; + + const updated = relaxFixedImageHeights(original); + + if (updated !== original) { + await knex('css_templates').where({ id: row.id }).update({ css_content: updated }); + fixed += 1; + } + } + + if (fixed > 0) { + console.log(` 181: relaxed the fixed image height in ${fixed} CSS template(s)`); + } +}; + +exports.down = async function down() { + // Deliberately irreversible. Putting the pixel heights back would re-break + // every aspect-ratio layout, and the rows may have been edited since — there + // is no version of "restore" here that is safer than doing nothing. +}; diff --git a/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx b/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx index 3e123759..743e2c9e 100644 --- a/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx +++ b/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx @@ -57,7 +57,7 @@ interface PhotoCardProps { const PhotoCard: React.FC = ({ photo, width, - height: _height, + height, onClick, onLike, onSelect, @@ -73,8 +73,13 @@ const PhotoCard: React.FC = ({ allowLikes = false, index }) => { - // Note: height is passed but not used as we maintain aspect ratio via width - void _height; + // The height MasonryPhotoAlbum computed from photos.width/height is used, + // not discarded (#1130). Letting the tile size itself from the image meant + // the rendered shape came from whatever rendition happened to be served — + // and with thumbnail_fit seeded to 'cover' (migration 040) every rendition + // is square, so the masonry laid out 79 identical squares and was + // indistinguishable from the fixed grid. The photo's real aspect ratio is + // in the DB and is what the album already laid out against. const { ref, inView } = useInView({ triggerOnce: true, threshold: 0.1, @@ -97,7 +102,7 @@ const PhotoCard: React.FC = ({ = ({