fix(components): stop declaring query containers nothing queries - #3082
Open
mfal wants to merge 9 commits into
Open
fix(components): stop declaring query containers nothing queries#3082mfal wants to merge 9 commits into
mfal wants to merge 9 commits into
Conversation
Contributor
Coverage Report for ./packages/components/
File CoverageNo changed files found. |
Contributor
✅ Visual Regression Tests PassedAll visual snapshots match the committed baselines. |
Contributor
🚀 Preview DeploymentPreview environments are ready:
Images:
|
`Section` declared `container-type: inline-size` for a `@container` query on its own header. That query left with the `Header` component. Nothing queries a section any more — every `@container` in the repo resolves against a container the querying component declares itself. The containment was therefore pure cost, and the cost is a Blink layout-invalidation bug (#2655): under a `container-type` element an async descendant size change can fail to invalidate the container's block-size layout, so it keeps the stale short height and the ancestor clips the revealed content until a later style recalc — a hover anywhere on the page — forces the flush. Sections nest, so `Section > Section` stacked two containers around the toggled content. `min-width: 0` keeps the one load-bearing side effect: inline-size containment zeroed the intrinsic inline contribution, which is what stops a section with unbreakable content from blowing out a `ColumnLayout` grid track. Measured in Chromium 149 / WebKit 26.5 / Firefox 151, a 400px two-track grid gives 200/200 with either containment or `min-width: 0`, and 693/8 with neither. The containing-block and stacking-context side effects are already gone in all three engines (Chrome 129 aligned to spec), so nothing could depend on them. Adds a visual scenario for the reporter's structure — a `Switch` inside nested sections inside a `LayoutCard` revealing taller content. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mfal
force-pushed
the
fix/section-container-type-2655
branch
from
September 2, 2026 11:53
56b0cca to
0490af2
Compare
Co-authored-by: mfal <4696750+mfal@users.noreply.github.com>
mfal
marked this pull request as ready for review
September 3, 2026 12:12
The decision and the measurements behind it live in the PR body and the commit that made the change. Repeating them as a 24-line block above `.section` only adds a second copy to keep in sync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Section.module.scss:5` was the `container-type` line this branch removes, so the pattern cited an example that no longer exists. `ColumnLayout`'s `.columnLayoutContainer` is the canonical case, and the convention every `@container` in the repo follows is that the querying component declares its own container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same dead containment `Section` loses in this branch: `.list` declared `container-type: inline-size`, but every `@container` in the repo resolves against a container the querying component declares itself — `ColumnLayout` on `.columnLayoutContainer`, `List`'s own `Header` on `.header`, `SegmentedControl` on `.segmentedControl`. Nothing lands on `.list`, so the containment only bought exposure to the Blink layout-invalidation bug from #2655, and a `List` nests inside a `Section` in most real layouts. `min-width: 0` keeps the one load-bearing side effect. Measured in webkit with a `List` in a 400px two-track grid, tracks (list/sibling): | .list | list view | tiles view | table view | | ----------------- | --------- | ---------- | ---------- | | `container-type` | 196/196 | 196/196 | 196/196 | | `min-width: 0` | 196/196 | 196/196 | 196/196 | | neither | 479/49 | 230/162 | 704/49 | `.items` clipping its overflow is not enough on its own — the numbers above are with that in place. The new test asserts the equality in all three view modes; it has to wait for the async items, because an empty list cannot blow out a track and measures 196/196 either way. Also drops ListItemView's `@use` of the container-breakpoint mixin, which it never included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-type-2655 # Conflicts: # packages/components/src/components/List/List.browser.test.tsx
The branch already moved the container-type rationale out of the code (d4b2775, 272741d) and left this copy behind — the `minmax(auto, Nfr)` mechanism and the measured 196/196 vs 479/49 numbers belong in the PR body, not in a test file. What stays is what a reader of the test needs: which declaration the list depends on, and what breaks without it. Co-Authored-By: Claude Opus 5 <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.
Sectionstops declaringcontainer-type: inline-size. The underlying bug is a Blink layout-invalidation bug, not a Flow bug — this change removes Flow's exposure to it, it does not fix Chrome. Under acontainer-typeelement an async descendant size change can fail to invalidate the container's block-size layout, so it renders at the stale short height and the ancestor clips the content until a later style recalc (the reporter's hover, anywhere on the page) forces the flush.Which container queries actually depend on
SectionNone. An exhaustive sweep (
@container,container-type,container-name,cq*units acrosspackages/andapps/, all file types) finds nine@containersites, and every one resolves against a container the querying component declares itself:@containersiteColumnLayout.module.scss(3).columnLayoutContainer, its own parent elementList/components/Header(5).header, the element the rules are nested incontainerBreakpointSizesmixin viaSegmentedControl.segmentedControlapps/docspage.module.scss:global(.flow--column-layout--column-layout-container), named in the selectorapps/docsComposingCodeExample.columnLayoutContainer— it is a direct child ofColumnLayout, not of aSectionSectiongot itscontainer-typein 2becdb5 ("feat: support Switch in Sections") for a@container (max-width: 550px)query on its own.header. That query left whenHeaderbecame its own component. The containment has been unused since.So the narrowest correct scope for
Sectionis not a container at all, which also answers the nesting question:Section > Sectioncurrently stacks two query containers that nothing queries — after this change, zero.A named container is not an alternative.
container-namedoes not exclude an element from unnamed@containerqueries; it only adds a way to target it explicitly. Naming would change neither query resolution nor the containment, so it fixes nothing here.Side effects of
container-typethat were checkedProbed directly in Chromium 149, WebKit 26.5 and Firefox 151 (Playwright, no repo code). All three engines agreed on every result below.
1. Intrinsic inline contribution — real, and load-bearing. Inline-size containment zeroes it. In a 400px grid with
grid-template-columns: 1fr 1frand unbreakable content in the first item:container-type: inline-sizemin-width: 0ColumnLayoutuses plainNfrtracks, andNfr=minmax(auto, Nfr)with a content-basedautominimum, so this mattered. Hencemin-width: 0on.section. (LayoutCardalready carriesmin-width: 0for the same reason.)Where
min-width: 0is not equivalent, and why it cannot matter.min-width: 0zeroes only the minimum; containment zeroed the max-content contribution too. Probed every context where that distinction shows:min-width: 0autotrackwidth: fit-contentparentdisplay: inline-blockparentThe three that differ are all shrink-to-fit contexts, and in every one containment collapses the parent to 0 width. A
Sectionin such a context would be invisible today, so no such layout exists in Flow — and if one ever appears,min-width: 0is the correct behaviour, not a regression. Confirmed independently against the source: everyauto/max-content/fit-contentgrid or shrink-to-fit rule in the repo is on a leaf component (Radio,RadioButton,LabeledValue,ProgressBar,Segment,NumberField,Button,ComboBox,Badge,Link, …) that cannot contain aSection; the only grid aSectionis ever an item of isColumnLayout, whosegetColumnsemitsNfrexclusively.Modalsizes its dialog fromwidth: inheritplus amax-widthtoken — definite, content-independent.2. Containing block for absolutely positioned descendants — already gone. An
position: absolute; left: 0child of acontainer-typeelement sitting atleft: 50pxlands atleft: 0, i.e. against the outer positioned ancestor, in all three engines. Chrome 129 aligned this to spec; nothing in the repo could depend on it, because it would already be broken.3. Stacking context — already gone. A
z-index: 5descendant of acontainer-typeelement still paints over az-index: 2sibling of that element in all three engines. Not trapped, so not a stacking context.4. Block-axis containment — never applied.
inline-sizenever contained the block axis, so nothing aboutSection's height behaviour changes.Verification
pnpm nx test:unit componentspnpm nx test:compile componentspnpm nx test:compile remote-react-componentspnpm nx test:browser components --browser.name=webkitpnpm nx test:browser remote-react-components --browser.name=webkitpnpm lintpnpm nx test:visual remote-react-components --browser.name=webkit(local, darwin)build:scss-typesproduces no diff — no class name changed.CI visual suite — full run, both browsers (
run-visual-tests, run 33610994607)The label run renders every file in both browsers, so it covers
webkit-linux(light) andfirefox-linux(dark), Local and Remote. Result: 5 of 6 shards fully green, and the only 4 failures in the whole suite areExactly the four references the new scenario does not have yet. Zero
pixels (ratio …) differ, zerodoes not match, zeroCould not captureanywhere in that run. No existing baseline moved, in either browser or either theme.CI visual suite —
Run testsshards (run 33610980760,webkit-linux)Same conclusion, plus one unrelated infrastructure failure worth naming:
No existing reference screenshot found.for the new scenario (Local + Remote). IncludesColumnLayout— passed, andSection's two pre-existing baselines — passed.Could not capture a stable screenshot within 5000ms.(111 tests) — a whole-shard capture collapse, the known shard-stability issue. Re-run: all 21 passed, no diffs.LayoutCard,Table,SegmentedControlShard 3's first attempt was never this change: it is a capture-stability timeout, not a mismatch; it hit every file in the shard uniformly, including
AlertIcon,Color,IconandAvatar, which contain noSectionat all; not one pixel comparison completed; and removing containment reduces layout work rather than preventing the page from settling. Re-running the same commit turned it green, which settles it.Every load-bearing candidate is verified green against the Linux baselines:
ColumnLayout(the only grid aSectionis an item of),LayoutCard(the clipping ancestor from the report),Table,SegmentedControl,List, andSectionitself.The one local failure,
Initials, is not a finding either: reported as1 pixels (ratio 0.01) differ, and opening the diff shows 296 raw pixels differing entirely inside the 😄 glyph's box (x 81–90, y 34–45) with per-channel deltas of 1–15 and no geometry change — emoji rasterisation noise under CPU load. It passes in isolation both with and without this change, and that test rendersFlex > Initialswith noSectionanywhere. It passed in CI.The section above documents the state before a0b466a; the baselines it calls missing are committed since.
New visual scenario
Section growing inside a LayoutCardreproduces the reporter's structure —LayoutCard > Section > Section > Switchwhere the switch reveals anAlertplus aColumnLayoutof fields. Verified locally in both environments: the card grows to fit, nothing is clipped, andLocalandRemoteagree pixel-for-pixel against the same reference.It cannot reproduce the bug itself — that is Blink-specific and Chrome-version-specific, and this suite runs WebKit and Firefox. It guards the height path so a future regression shows up as a diff.
Its baselines were created by the
update-screenshotsrun on a0b466a and are committed. On the current head the whole suite is green in both paths — thevisualgate (4 shards) and a verify-onlyrun-visual-tests(6 shards) — so no baseline differs any more and nothing is left to update.Still open from the issue
Steps 2 and 3 of the issue's suggested next steps: have the reporter verify in the affected Chrome, and optionally file a minimal repro on
issues.chromium.org.List— the same dead containment.listdeclaredcontainer-type: inline-sizewith nothing resolving against it either: its ownHeaderdeclares.header, andListItemView@used thecontainerBreakpointSizesmixin without ever including it (that dead@useis gone too). AListsits inside aSectionin most real layouts, so it carried the same #2655 exposure.min-width: 0is load-bearing here as well. Track widths (list / sibling) for aListwith unbreakable content in a 400px two-track grid, measured in webkit:.listcontainer-type: inline-sizemin-width: 0.itemsclipping its overflow does not cover this on its own — those numbers are with the clipping in place. The new test inList.browser.test.tsxasserts the equality across all three view modes. It waits for the async items first: an empty list cannot blow out a track and measures 196/196 either way, which is how a first (wrong) measurement of this made the containment look free.fixes #2655
🤖 Generated with Claude Code