Skip to content

Commit 714cdb4

Browse files
Dylan Bradshawclaude
Dylan Bradshaw
and
claude
committed
fix(styles): keep the height of wrappers whose children are all out of flow
stripHeightForWrappers dropped height/block-size from any "transparent flow wrapper", relying on hasFlowFast to prove the element had in-flow content that would re-establish its box in the clone. Both of hasFlowFast's signals answer a different question than the one being asked: - `textContent` also sees text inside absolutely positioned descendants, so a wrapper holding only an abspos overlay with a caption reads as having text. - `scrollHeight` is floored by clientHeight, so a non-scrolling block reports its own used height no matter what its children are. The old comment claimed abspos children do not contribute; in practice the probe never drops to the padding it was compared against. A hero banner sized `height:100vh` whose children are all `position:absolute` therefore lost its height. Inside the foreignObject the authored stylesheet is gone, nothing restores it and the wrapper collapses to 0 — every later section shifts up by the banner's height and paints over it. hasFlowFast now asks the real question: direct non-blank text nodes, an immediate <br>, or an element child that is itself in flow. The children walk runs only after the cheap text/<br> paths miss, and skips display:none. `cs` is no longer needed, so it is dropped from the signature. Regression test covers the abspos-only wrapper end to end (size survives into the exported SVG) and via the clone style key, plus a display:none-only wrapper. A negative control keeps a genuinely transparent wrapper's height stripped, so the pass still does its job. Without the fix 3 of the 4 fail. Closes #467 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 3d95744 commit 714cdb4

2 files changed

Lines changed: 127 additions & 13 deletions

File tree

‎__tests__/module.styles.abspos-wrapper.test.js‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import { describe, it, expect, afterEach } from 'vitest'
2+
import { snapdom } from '../src/index'
3+
import { inlineAllStyles } from '../src/modules/styles.js'
4+
5+
/**
6+
* A wrapper sized only by the viewport (height:100vh) whose children are all
7+
* absolutely positioned had its height/block-size stripped as a "transparent flow wrapper".
8+
* The site stylesheet is gone inside the foreignObject, so nothing restored that height and
9+
* the wrapper collapsed to 0: every following section shifted up and painted over it.
10+
*/
11+
12+
function freshSession() {
13+
return { styleMap: new Map(), styleCache: new WeakMap(), nodeMap: new Map() }
14+
}
15+
16+
/** Size declaration snapdom emits for `el` in the exported SVG, or '' when absent. */
17+
function sizeRuleFor(svg, selector) {
18+
// result.url is a data: URL — drop everything before the markup itself.
19+
const markup = svg.slice(svg.indexOf('<'))
20+
const doc = new DOMParser().parseFromString(markup, 'image/svg+xml')
21+
const el = doc.querySelector(`foreignObject ${selector}`)
22+
if (!el) return null
23+
const css = [...doc.querySelectorAll('style')].map((s) => s.textContent).join('\n')
24+
const classes = (el.getAttribute('class') || '').split(/\s+/).filter((c) => /^c\d+$/.test(c))
25+
for (const c of classes) {
26+
const rule = css.match(new RegExp(`\\.${c}\\s*\\{([^}]*)\\}`))
27+
if (!rule) continue
28+
const size = rule[1].match(/(?:^|;)\s*(?:block-size|height)\s*:\s*([^;]+)/)
29+
if (size) return size[1].trim()
30+
}
31+
return ''
32+
}
33+
34+
describe('stripHeightForWrappers — wrappers whose children are all out of flow', () => {
35+
let root, sheet
36+
afterEach(() => { root?.remove(); sheet?.remove() })
37+
38+
function mount(css, html) {
39+
sheet = document.createElement('style')
40+
sheet.textContent = css
41+
document.head.appendChild(sheet)
42+
root = document.createElement('div')
43+
root.innerHTML = html
44+
document.body.appendChild(root)
45+
return root
46+
}
47+
48+
it('keeps the height of a wrapper whose only children are absolutely positioned', async () => {
49+
// Height must come from a stylesheet: an inline height is already respected (guard 1).
50+
mount(
51+
'.sd-hero{position:relative;height:300px}' +
52+
'.sd-hero > .sd-slide{position:absolute;top:0;left:0;right:0;bottom:0}',
53+
'<div class="sd-hero"><div class="sd-slide">caption text</div></div>' +
54+
'<div class="sd-after">below the hero</div>',
55+
)
56+
57+
const hero = root.querySelector('.sd-hero')
58+
expect(getComputedStyle(hero).height).toBe('300px')
59+
// The trap: textContent and scrollHeight both look like real in-flow content.
60+
expect(/\S/.test(hero.textContent)).toBe(true)
61+
expect(hero.scrollHeight).toBe(300)
62+
63+
const svg = decodeURIComponent((await snapdom(root)).url)
64+
expect(sizeRuleFor(svg, '.sd-hero')).toBe('300px')
65+
})
66+
67+
it('records the size on the clone style key rather than dropping it', async () => {
68+
mount(
69+
'.sd-hero2{position:relative;height:250px}' +
70+
'.sd-hero2 > .sd-slide{position:absolute;inset:0}',
71+
'<div class="sd-hero2"><div class="sd-slide">caption</div></div>',
72+
)
73+
const hero = root.querySelector('.sd-hero2')
74+
const clone = hero.cloneNode(true)
75+
const session = freshSession()
76+
77+
await inlineAllStyles(hero, clone, session)
78+
79+
expect(session.styleMap.get(clone)).toMatch(/(?:^|;)\s*(?:block-size|height)\s*:\s*250px/)
80+
})
81+
82+
it('still strips the height of a genuinely transparent flow wrapper', async () => {
83+
// Negative control: in-flow child of the same height — the wrapper is redundant and
84+
// removing its height lets the clone reflow naturally, which is the point of the pass.
85+
mount(
86+
'.sd-wrap{position:relative;height:300px}' +
87+
'.sd-wrap > .sd-inner{height:300px}',
88+
'<div class="sd-wrap"><div class="sd-inner">in flow</div></div>',
89+
)
90+
const svg = decodeURIComponent((await snapdom(root)).url)
91+
expect(sizeRuleFor(svg, '.sd-wrap')).toBe('')
92+
})
93+
94+
it('treats a wrapper holding only display:none children as having no flow content', async () => {
95+
mount(
96+
'.sd-hidden-host{position:relative;height:120px}' +
97+
'.sd-hidden-host > .sd-gone{display:none}',
98+
'<div class="sd-hidden-host"><div class="sd-gone">hidden</div></div>',
99+
)
100+
const svg = decodeURIComponent((await snapdom(root)).url)
101+
expect(sizeRuleFor(svg, '.sd-hidden-host')).toBe('120px')
102+
})
103+
})

‎src/modules/styles.js‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -415,24 +415,35 @@ function isFlexOrGridItem(el) {
415415

416416
/**
417417
* ¿Hay contenido en flujo? Versión rápida:
418-
* - Texto no vacío → true (no dispara layout).
418+
* - Nodo de texto directo no vacío → true (no dispara layout).
419419
* - <br> inmediato → true.
420-
* - Geometry probe: scrollHeight > padding (abspos NO suma) → true.
420+
* - Algún hijo elemento en flujo → true.
421+
*
422+
* Both questions are about THIS element's own flow, so neither `textContent` nor a
423+
* scrollHeight probe can answer them: `textContent` also sees text inside absolutely
424+
* positioned descendants, and scrollHeight is floored at clientHeight, so a wrapper whose
425+
* children are all out of flow still reports its own used height. Trusting either one made
426+
* stripHeightForWrappers drop the height of such a wrapper, which then collapsed to 0 inside
427+
* the foreignObject — every following section shifted up over it.
421428
* @param {Element} el
422-
* @param {CSSStyleDeclaration} cs // ya lo tenemos en mano
423429
*/
424-
function hasFlowFast(el, cs) {
425-
if (el.textContent && /\S/.test(el.textContent)) return true
430+
function hasFlowFast(el) {
431+
// Only direct text nodes belong to this element's flow.
432+
for (let n = el.firstChild; n; n = n.nextSibling) {
433+
if (n.nodeType === 3 && /\S/.test(n.nodeValue)) return true
434+
}
426435
const f = el.firstElementChild, l = el.lastElementChild
427436
if ((f && f.tagName === 'BR') || (l && l.tagName === 'BR')) return true
428437

429-
// Probe geométrico (1 lectura de layout): evita recorrer hijos
430-
// Nota: scrollHeight no incluye hijos absolute; si sólo hay absolute → ≈ padding
431-
const sh = el.scrollHeight
432-
if (sh === 0) return false
433-
const pt = parseFloat(cs.paddingTop) || 0
434-
const pb = parseFloat(cs.paddingBottom) || 0
435-
return sh > pt + pb
438+
// An element child contributes flow content only when it is itself in flow. getStyle
439+
// memoizes per node, and this runs only after the cheap text/<br> paths miss.
440+
for (let c = el.firstElementChild; c; c = c.nextElementSibling) {
441+
const s = getStyle(c)
442+
if (s.display === 'none') continue
443+
const pos = s.position
444+
if (pos !== 'absolute' && pos !== 'fixed') return true
445+
}
446+
return false
436447
}
437448

438449
/**
@@ -484,7 +495,7 @@ function stripHeightForWrappers(el, cs, snap) {
484495
if (cs.visibility === 'hidden' || cs.opacity === '0') return
485496

486497
// 6) Solo wrappers "en flujo" realmente neutros
487-
if (!hasFlowFast(el, cs)) return
498+
if (!hasFlowFast(el)) return
488499

489500
// 7) Ahora sí: quitamos height y block-size del snapshot
490501
delete snap.height

0 commit comments

Comments
 (0)