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 whatever rendition was served. With thumbnail_fit seeded 'cover' every rendition is square, so the masonry drew identical squares. The bundled CSS templates pinned images to a fixed pixel height, which beats the .h-full utility six of the seven layouts rely on. 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; 175 repairs the rows already seeded. The repair is whitespace-tolerant because sanitizeCSS strips newlines from any template ever saved through the editor, matches the height property with a lookbehind so line-height/max-height are untouched, handles grouped selectors and skips nested rules. Stable twin of #1135.
This commit is contained in:
Paul Nothaft
@@ -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/175_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 175 — CSS template image height (#1131)', () => {
|
||||
let knex; let tmpDir;
|
||||
|
||||
beforeAll(async () => {
|
||||
tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-mig175-'));
|
||||
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: <n>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(/(?<![\w-])height:\s*200px/);
|
||||
});
|
||||
|
||||
it('handles a grouped selector list', async () => {
|
||||
// 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');
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -77,7 +77,11 @@ 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 — #1131. */
|
||||
height: 100%;
|
||||
object-fit: cover;
|
||||
transition: transform 0.3s ease;
|
||||
}
|
||||
|
||||
@@ -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 */
|
||||
|
||||
@@ -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 = /(?<![\w-])height\s*:\s*\d+(?:\.\d+)?px/gi;
|
||||
|
||||
function relaxFixedImageHeights(css) {
|
||||
return css.replace(PHOTO_CARD_IMG_RULE, (whole, selectors, body) => {
|
||||
// .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(` 175: 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.
|
||||
};
|
||||
Reference in New Issue
Block a user