Repository navigation
Conversation
These are css-overflow-5 pseudo-classes and lightningcss carries all three. Bun's table did not, so they parsed as PseudoClass::Custom and were emitted verbatim while the :target-within beside them in the same table was normalized. A pseudo-class name is ASCII case-insensitive, so :TARGET-CURRENT and :target-current are one selector and only one of the two spellings was canonical. Hard-enabled rather than gated behind a port of lightningcss's SCROLL_NAVIGATION_CONTROLS flag: nothing on the bundler path sets a ParserFlags bit, so a gated entry would be unreachable from bun build. No compat entries, so is_compatible is unchanged: it already sends both PseudoClass::Custom and unmapped known variants to the same return false.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe CSS selector parser now recognizes three CSS Overflow 5 scroll-marker pseudo-classes. The serializer emits their canonical names. A bundler test verifies mixed-case parsing and lowercase minified output. ChangesCSS scroll-marker pseudo-classes
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This change adds support for the three CSS Overflow 5 target pseudo-classes and canonical lowercase output, with regression coverage for mixed-case input and minified serialization. No merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This branch has not been deployed
2 participants
Fixes #42480.
Problem
:target-current,:target-beforeand:target-afterare css-overflow-5 pseudo-classes, and lightningcss carries all three. Bun's table does not, so they parse asPseudoClass::Customand are emitted verbatim while the:target-withinsitting next to them in the same table is normalized. Measured on 1.4.0:A pseudo-class name is ASCII case-insensitive, so those are two spellings of one selector and only one of them is canonical today.
This is also the row that regresses when #38572 lands. That PR corrects the bare-pseudo-class fallback from warning on a
_prefix to the upstream-rule, which is right, and the moment it does, all three of these start printingInvalid selector. Unsupported pseudo-class or pseudo-elementon valid CSS. Following on from #41120 and #41122.Fix
Three variants, three table entries, three serialization arms.
lookup_non_ts_pseudo_classalready matches case-insensitively, so recognition is what buys the canonical spelling.Hard-enabled rather than flag-gated, which was the open question on #41122. lightningcss puts these behind
ParserFlags::SCROLL_NAVIGATION_CONTROLSand defaults it off, so porting the gate faithfully was the alternative. It does not work here:ParserFlagsinsrc/css/css_parser.rs:2989carries three bits, the oneParserOptionsconstruction in the crate usesParserFlags::default(), and repo-wide the type appears only insrc/css/css_parser.rs,src/css/lib.rs,src/css/selectors/parser.rs,src/css_jsc/css_internals.rsand two test files. Nothing on the bundler path sets a bit, so a fourth one would be unreachable frombun buildand the fix would never actually apply. A flag defaulted on is the same as no flag, with more surface to keep.No compat entries, and nothing about the emitted CSS changes except the casing. I told you on #41122 that recognizing these would turn on
:is()lowering under old targets, and that was wrong, so here is the correction. Inis_compatible,Component::NonTsPseudoClasssendsPseudoClass::Custom { .. } => {}and_ => {}to the same trailingreturn false, so an unmapped known variant and an unknown custom one are already treated identically. AddingFeature::TargetCurrent/TargetBeforeAfterwould be the behavior change rather than the status quo, andsrc/css/compat.rsis generated and stamped DO NOT EDIT. #41122 added no compat entries for its four pseudo-elements either, so this matches it.Testing
test/bundler/css/target-current-pseudo-classes-41120.test.ts, one test asserting canonical serialization and an empty log, with:TARGET-WITHINin the same fixture as the in-file control for what an already-known name does.Verified failing, not verified passing, and here is exactly why. Under
USE_SYSTEM_BUN=1 bun testit fails on released 1.4.0 with the difference this PR is about:I could not run the other half, because
bun bddoes not complete on this machine (macOS 27, Homebrewllvm@21, the compilerscripts/build.tsrequires). The Rust side builds clean,bun_cssand every downstream crate, so the change compiles and no exhaustive match elsewhere is broken by the three new variants. The C++ side fails on two things unrelated to this diff, both traced to Homebrew clang against the macOS 27 SDK rather than to Bun's source:NANis undefined because the SDK routes it through<float.h>under__has_feature(modules)and libc++'sfloat.hdoes not implement that protocol, and-Werrorrejects Apple'sstack_protector_ignoreattribute as unknown. Filed separately as #41141 with the measurements.So CI builds this on your toolchain, and the failing-side control above is the half that proves the test is not vacuous.
An earlier draft had a second test on the plain lowercase spelling. It passed with
USE_SYSTEM_BUN=1, because a custom pseudo-class already round-trips unchanged, so it asserted nothing and came back out. Case is the only externally observable difference this change makes, which is why the whole test rests on it.