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 = ({ = ({