fix(styles): keep the height of wrappers whose children are all out of flow (#467) - #469
Merged
tinchox5 merged 1 commit intoJul 24, 2026
Conversation
…f 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 zumerlab#467
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #467.
Problem
stripHeightForWrappersdropsheight/block-sizefrom the style snapshot of a "transparent flow wrapper", relying onhasFlowFastto prove the element has in-flow content that will re-establish the box in the clone. Inside theforeignObjectthe authored stylesheet is gone, so when that assumption is wrong nothing restores the height and the wrapper collapses to 0 — every following section shifts up and paints over it.Both of
hasFlowFast's signals answer a different question than the one being asked:el.textContentalso sees text inside absolutely positioned descendants, so a wrapper holding only an abspos overlay with a caption reads as having in-flow text.scrollHeight > paddingTop + paddingBottomis unreliable becausescrollHeightis floored atclientHeight. A non-scrolling block always reports at least its own used height, whatever its children are. The old comment said abspos children don't contribute — true of the content height, but the probe never drops to the padding it's compared against.For a
height:300pxhero whose children are allposition:absolute,textContentis non-blank andscrollHeightis 300. Both signals say "has flow content", so the height is stripped.Fix
hasFlowFastnow asks the real question — does this element have in-flow content?textContent, which reaches into out-of-flow descendants)<br>(unchanged)display:none,absoluteandfixedThe children walk runs only after the cheap text/
<br>paths miss, andgetStylememoizes per node (cache.computedStyle), so children whose styles are needed later aren't recomputed.csis no longer used and is dropped from the signature; the single caller is updated.The direct-text-node loop uses the same
nodeType === 3 && /\S/.test(...)idiom already present in this file atstyles.js:222.Behaviour change, scoped
The new check returns
falsein exactly the intended case (all children out of flow ordisplay:none). It returnstruein one case the old probe returnedfalse: an in-flow child whenscrollHeight === 0. That can't cause a regression —scrollHeightis floored atclientHeight, soscrollHeight === 0only when the element's own used height is already 0, making the strip a no-op. Guard 2b (|usedH - scrollHeight| > TOL) covers every other path.Tests
__tests__/module.styles.abspos-wrapper.test.js, 4 cases:inlineAllStylesdisplay:none-only wrapper is treated as having no flow content3 of the 4 fail on
main; the negative control passes both ways by design.Note for reviewers: the wrapper height must come from a stylesheet. An inline
style="height:…"is already respected by guard 1 and masks the bug.Full suite: 693 passed / 1 skipped, against 689 / 1 on
main— no regressions.npm run lintandnpm run test:typesclean. Verified on Chromium; I don't have the Firefox/WebKit Playwright binaries locally.