Expand modern CSS grammar coverage - #57
Expand modern CSS grammar coverage#57Kristofer Baxter (kristofer-baxter) wants to merge 1 commit into
Conversation
Add grammar and regression coverage for modern at-rules, functions, properties, selectors, and media queries. Include `@container` coverage for the gap first identified in PR microsoft#15, and accept digit-prefixed `@property` names consistently with PR microsoft#43. Dedicated prelude regions end only at `)`, so an unclosed parenthesis would run to end of file where the generic at-rule header on main recovers at the first `{`. Guard the parenthesised regions reachable inside a prelude so they recover at `{` only when the rest of the line does not close the parenthesis, preserving legal balanced blocks in declaration values. The test is lexical rather than a plain `[^)]*`, because a `)` inside a comment, string or escape does not close anything: without that, `@container (width > 1px{ /* ) */` stayed open where main recovered. The test also accepts an incomplete trailing escape, both bare and inside an unterminated string, because a line ending in an odd number of backslashes otherwise matches no alternative and the newline-escape rule holds the prelude open. Guarding only the outermost regions is not enough, because an outer `end` cannot be evaluated while an inner rule is active. Do not use `;` as a bail-out: `<general-enclosed>` permits top-level semicolons through `<any-value>`, and legal preludes may span lines. Leave `var()` and custom function calls unguarded. Both are declaration values, and `<declaration-value>` admits a balanced curly block that legally spans lines, as in `--x: --foo({ ... });`. The legal `--foo({` and the malformed `--foo(a{` are indistinguishable within a single line, so recovering there would cost legal CSS that main scopes correctly. Main does not recover an unclosed `var(` either. Add `#condition-string` to the `@media` and `@supports` condition regions. The bail-out only inspects text from the candidate `{` onwards, so with no string rule active it read the closing quote of `(future: "a{b")` as an opening one, took the real `)` for shielded, and opened the body at the brace inside the string. `@container` keeps no string rule, because there an unterminated string would instead shield a malformed `{` from the bail-out; that case tokenizes as it already does on main. `#condition-string` is a condition-local copy of `#string` rather than `#string` itself. It differs in one respect: its newline escape ends at `^` instead of the shared rule's `^(?<!\G)`, which cannot match while the condition is still open and so swallowed the rest of the stylesheet after a legal `@media (future: "a{\` continuation. Keeping the copy local leaves string tokenization everywhere else byte-for-byte as it is on main. Give the `:lang()` string rules the end-of-line fallback the shared `#string` rule already uses, so an unterminated language range no longer runs past its line. Leave `animation-timeline`, `animation-range`, `animation-range-start`, and `animation-range-end` entirely to PR microsoft#32.
|
@microsoft-github-policy-service agree company="Microsoft" |
|
Hi Kristofer Baxter (@kristofer-baxter), It seems that this PR contains three kinds of changes:
Does it make sense to split it up to reduce the PR size? |
|
Yes, that split makes sense. Thanks for reading through it. Mapping your three buckets onto the diff gives this, with the caveat that the Fixes to existing features. One change, applied uniformly at 36 sites: an New features. Four at-rules ( Test coverage. This is the part that cannot stand alone. Nearly every added So the plan is five stacked pull requests:
Each lands with the tests that pin it. Roughly 300 to 600 lines apiece. Rather than churn this repository while the shape is still in question, the One question on ordering: number 1 is independent of the other four and fixes |
yes please :) |
|
Romain Menke (@romainmenke) Split, as you asked. The fix is up at #58 and the four feature
Each one is based on the one above it, runs its own test suite green, and does Splitting this turned out not to be a packaging exercise. Gating each piece
Every one of those existed in some intermediate state only. The endpoint The description here was also wrong on two counts, both corrected in #58. The Closing this in favour of the five. Thanks for pushing back on the size; the |
2 participants
Expand modern CSS grammar coverage
Coverage
This broad update adds modern at-rule, function, property, selector, and media-query coverage. The main review risk is recovery around the parenthesized regions, which gets most of the detail below.
The grammar diff adds dedicated coverage for:
@containersize, named,style(), andscroll-state()queries, including logical operators and size or scroll-state features@scopestart and limit preludes,@starting-style, and@propertyenv(),anchor(),anchor-size(), and additional math and value functions:nth-child(... of ...),:state(), and argument-specific functional pseudo-elements@propertyaccepts digit-prefixed custom-property names. Ordinary identifiers added by this change use the stricter first-character pattern.Everything lives in
grammars/css.cson; the specs exercise complete scope arrays withdeepStrictEqual.Recovery guard
Dedicated parenthesized regions improve coverage, but an unclosed region can retain its scopes beyond the prelude. To preserve recovery, all 36 reachable regions end at
{only when the rest of the line does not close the parenthesis.A plain
[^)]*is not enough. A)inside a comment, string or escape closes nothing, and counting it suppressed recovery thatorigin/mainperformed:@container (width > 1px{ /* ) */stayed open. Trailing comments containing parentheses are ordinary in real stylesheets.Instead, the lexical bail-out skips complete and unterminated comments, quoted strings, and escapes while looking for a real closing parenthesis. Escapes include incomplete ones. If a line ends in an odd number of backslashes, bare or inside an unterminated string, no complete-escape alternative matches; the inherited newline-escape rule would otherwise hold the prelude open across the line break.
Every reachable nested region needs the guard because an outer
endcannot be evaluated while an inner rule is active.A recovery oracle checks every prefix of 24 representative preludes, with
{and{also appended: 2,412 cases total, 1,715 of them inside the guard contract. Againstorigin/main, this branch has 186 improvements, 42 regressions, and 147 residual contract violations also present onmain. All 42 regressions are@scope (inputs containing an unclosed[.A differential over legal snippets covering every guarded construct, plus Bootstrap and 176k lines of Tailwind output, found no genuine losses.
Known limitation
@scope ([data-x{leaks, and an unterminatedurl(a{inside a prelude stays open until a later)or guard-eligible{.That first one is
main's existing attribute-selector leak:a[data-x{also leaks there. New@scopecoverage exposes it in one additional context. Guarding the attribute selector was tried and reverted, because it broke a legal multi-line attribute selector covered by a pre-existing passing test.The
url()case has the same shape.url()'s argument pattern,[^'")\s]+, is byte-identical tomain's and consumes the{before the region'sendcan be tested. That meansa { background: url(x{leaks onmaintoo, and the region stays open across however many lines precede the next)or guard-eligible{, not one.Tightening the class to exclude
{was also tried and measured. It splits the legal valueurl(https://x/a{b})into three tokens and drops the brace out ofvariable.parameter.url.css. An unquoted url-token may legally contain{, so the pattern is left alone.Both leaks are inherited rather than introduced. The new
@scopeand@containerpreludes reuse existing selector and function rules, exposing their pre-existing leaks in contexts wheremainnever applied them.On balance the branch removes four pre-existing leak cases while inheriting these. It does not solve every malformed-input leak, and makes no claim to.
Two narrower cases remain. A closing parenthesis inside a nested parenthesised construct, as in
@container (width > 1px{ url("x)"), still suppresses recovery because balanced-parenthesis tracking is beyond a line-local lookahead. Separately,@document'surl-prefix(),domain()andregexp()do not recover with or without the guard: their argument patterns consume the{before the region'sendis tested, andmainbehaves identically.There is also an
@container-only case.<general-enclosed>accepts<any-value>, so@container (future: "a{b")is legal and the brace inside the string is not a body brace. The bail-out inspects only the text from the candidate brace onwards; with no string rule active, it reads the closing quote as an opening one, treats the real)as shielded, and opens the body early.@mediaand@supportsare fixed by giving their condition regions a#condition-stringrule, which keeps the enclosingendfrom being evaluated inside a string. A new test covers both.#condition-stringis a condition-local copy of#stringrather than#stringitself, differing only in that its newline escape ends at^rather than the shared rule's^(?<!\G).The shared form cannot match while the condition is still open, so a legal
@media (future: "a{\line continuation swallowed the rest of the stylesheet. The local copy fixes that while leaving string tokenization in every other context byte-for-byte as it is onmain, checked across 15 line-continuation constructs.@supportsroutes the same string through the shared property-value context, wheremainswallows it too; that case is left at parity rather than fixed.The same fix was tried for
@containerand reverted. There, an unterminated string would shield a malformed{from the bail-out and reintroduce 68 leak cases that recover today.@containertherefore mis-tokenizes this legal input exactly asorigin/maindoes. It is not a differential regression, but it is not fixed either.One last case is specific to the new
style()coverage. CSS Conditional Rules 5 defines a style query's<style-feature-value>as a<declaration-value>, so a balanced curly block may legally span lines inside it. The line-local guard treats that opening{as the container body when the)is on a later line, so@container style(--x: {followed by a block is mis-tokenized.This matches
origin/main's generic at-rule header, so it is not a differential regression, though the dedicatedstyle()region does not cover it. Removing the guard fromstyle()would trade this edge case for all recovery on malformed style queries. The guard was kept, but this is the judgment call with the least confidence behind it, and pushback is welcome.Scope naming
color-mix()moves frommeta.function.misc.csstometa.function.color.css, the scopemainalready gives every other color function, includingrgb(). The function name keepssupport.function.misc.css.A theme targeting the
meta.function.misc.csscontainer will no longer matchcolor-mix()or its contents. The reclassification looks like the better fit, but maintainers should push back if preserving the old container matters more.var()and custom function trade-offvar()and custom function calls are not guarded. Both are declaration values, and<declaration-value>admits a balanced curly block that legally spans lines, as invar(--fb, { ... })and--x: --foo({ ... });. Applying the guard truncated those legal values.origin/mainscopes them correctly, so the guard was a regression there.The legal
--foo({and malformed--foo(a{are lexically identical within a single line; no line-local test can separate them. Legal CSS wins.maindoes not recovervar(--x{either, and mutation-verified tests pin both legal forms.Related work and credit
This work overlaps several open pull requests. Credit belongs to their authors, named below.
@containerat-rule support and is the largest overlap with this branch. This branch covers the same gap as part of broader container-query grammar, recovery, and regression coverage.animation-timeline,animation-range,animation-range-start, andanimation-range-end. Those four properties were removed from this branch and left to PR Adds new scroll driven Animation properties #32.@propertycustom-property names while adding six uses of the strict first-character pattern for ordinary identifiers.@page#36 by romainmenke contributes@pagehandling, and PR Add support for system colors #37 contributes CSS4 system colors. Both were checked; neither overlaps this branch, and this change neither duplicates nor supersedes them.style(), so it does not overlap that work.Verification
3 files changed, 2,120 insertions, 90 deletions, in one commit
302 tests total: 296 passing, 0 failing, 6 skipped under Node 20.18.0 after a clean
npm ci85
it(blocks addedorigin/mainalso has 6 skipped tests, so this branch adds no skipsguard-site mutation campaign, at two strengths, each applied to one of the 36 sites at a time:
@document's argument functions, cannot recover with or without the guard, as onmain.[^)]*$it replaces: only 10 of 36 caught. The 36 guard bodies are byte-identical, so this says which regions have a test exercising a shielded), not that there are 26 differing implementations. It also means most sites would not catch a regression to the naive form on their own. The distinction is exercised directly at the sites listed above.These totals come from a harness that is not part of this branch, so they are not reproducible from the repository as submitted.
malformed-input comparison:
mainleaks on 8 of 10 sampled cases; this branch fixes 4 and introduces 0backward compatibility, checked mechanically against
origin/main: no CSS identifier is lost (244 gained), no scope name is removed (32 added), no#includereference dangles or goes unreferenced, and all 311 begin/end/match patterns compile with every capture index in rangescope-loss differential over real stylesheets: across 176,011 lines of Tailwind 2.2.19 output, no character loses a scope
mainassigned. Bootstrap 5.3.3 differs in five constructs, all gains or corrections:prefers-reduced-motion(32),reduce(31) andno-preference(1) gain media-feature and value scopes instead of plain header text, and::file-selector-button(6) and:placeholder-shown(4) are corrected fromentity.name.tag.custom.cssto a pseudo-element and a pseudo-class@containerand@scopepreludes now carrymeta.at-rule.container.header.cssandmeta.at-rule.scope.header.cssrespectively, rather than the genericmeta.at-rule.header.css, matching the namingmainalready uses for@mediaand@supportsno catastrophic backtracking: six adversarial inputs, including 200 nested
calc(and a 5,000-character prelude, tokenize in 244 ms or lessPerformance
Both grammars were loaded in one process and tokenized alternately, taking min-of-7 per grammar, over six full runs. The machine started idle at a load average of 3.2, but the benchmark drives its own load up over the sequence: sampled per-run load ranged from 3.8 to 13.2. Every run is reported, including the ugly ones.
Below are the median wall time per grammar, the median of the six paired per-run percentage deltas, and the full range of those paired deltas.
origin/main;{Four of the seven workloads have the same sign in all six runs and can be read as measured effects: Bootstrap +4.8%, Tailwind +5.3%, the container-query corpus -65.2%, and the scroll-state case +582%. Neither corpus contains a single
@containerrule or container property, so the first two measure the cost on ordinary CSS, at roughly 5%.Three ranges cross zero and resolve nothing. The brace-heavy synthetic line is the noisiest workload measured; its +5.5% median should not be read as a cost.
The scroll-state figure is a large ratio on a small absolute base: 2 ms to 16 ms on a 20 KB file consisting of a single unterminated
scroll-state()query. It reflects repeated guard evaluation against a line that never closes.Be aware that the synthetic workloads above are generated fixtures, and they are not part of this branch. Only the Bootstrap and Tailwind rows can be reproduced from public sources.
The lexical bail-out was also compared with the simpler
[^)]*$form, holding everything else in the grammar constant, over three further interleaved runs. Every workload landed between -6.0% and +2.7%, with most deltas negative, so no workload showed the lexical form to be measurably more expensive. The roughly 5% cost on ordinary CSS comes from the added region coverage, not the guard's complexity.Container queries come out about 3x faster because precise regions replace the generic fallback's retries.