Skip to content

css: parse the *property hack as a declaration #46751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

css: parse the *property hack as a declaration #46751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 68 additions & 1 deletion src/css/css_parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,17 @@ pub trait DeclarationParser {
name: &[u8],
input: &mut Parser,
) -> CssResult<Self::Declaration>;

/// Parse the value of a `*property: value` declaration (the IE 6/7 star
/// hack). `name` is the source text of the name and starts with the `*`.
/// The default rejects it.
fn parse_star_hack(
_this: &mut Self,
_name: &'static [u8],
input: &mut Parser,
) -> CssResult<Self::Declaration> {
Err(input.new_custom_error(ParserError::invalid_declaration))
}
}

pub trait RuleBodyItemParser: AtRuleParser + QualifiedRuleParser + DeclarationParser {
Expand Down Expand Up @@ -2108,6 +2119,20 @@ mod rule_parsers {
&mut ctx,
)
}

fn parse_star_hack(
this: &mut Self,
name: &'static [u8],
input: &mut Parser,
) -> CssResult<()> {
declaration::parse_star_hack(
name,
input,
&mut this.declarations,
&mut this.important_declarations,
this.options,
)
}
}

#[inline]
Expand Down Expand Up @@ -2874,6 +2899,15 @@ where
_ => {}
}

let token = *tok;
if matches!(token, Token::Delim(delim) if delim == u32::from(b'*'))
&& P::parse_declarations(self.parser)
&& let Some(declaration) =
parse_star_hack_declaration(start.position, self.input, self.parser)
{
return Some(Ok(declaration));
}

let result: CssResult<I<P>> = if P::parse_qualified(self.parser) {
self.input.reset(&start);
let delimiters = if P::parse_declarations(self.parser) {
Expand All @@ -2883,7 +2917,6 @@ where
};
parse_qualified_rule(&start, self.input, self.parser, delimiters)
} else {
let token = *tok;
self.input
.parse_until_after(Delimiters::SEMICOLON, move |_i| {
Err(start.source_location().new_unexpected_token_error(token))
Expand All @@ -2895,6 +2928,40 @@ where
}
}

/// Reads `*property: value;`, a declaration whose name is a `*` directly
/// followed by an identifier. The `*` is at `name_start` and the input is
/// right after it. Returns `None` with the input back at that position when
/// the item is anything else.
#[cold]
fn parse_star_hack_declaration<P: RuleBodyItemParser>(
name_start: usize,
input: &mut Parser,
parser: &mut P,
) -> Option<P::Declaration> {
let after_star = input.state();
if matches!(
input.next_including_whitespace_and_comments(),
Ok(Token::Ident(name)) if !name.starts_with(b"--")
) {
let name = input.slice_from_cloned(name_start);
let result = parse_until_after(
input,
Delimiters::SEMICOLON,
ParseUntilErrorBehavior::Stop,
(parser, name),
|(parser, name), input| {
input.expect_colon()?;
P::parse_star_hack(parser, name, input)
},
);
if let Ok(declaration) = result {
return Some(declaration);
}
}
input.reset(&after_star);
None
}

// ───────────────────────────── ParserOptions ─────────────────────────────

pub struct ParserOptions<'a> {
Expand Down
59 changes: 58 additions & 1 deletion src/css/declaration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::css_properties::align::AlignHandler;
use crate::css_properties::background::BackgroundHandler;
use crate::css_properties::border::BorderHandler;
use crate::css_properties::box_shadow::BoxShadowHandler;
use crate::css_properties::custom::CustomPropertyName;
use crate::css_properties::custom::{CustomProperty, CustomPropertyName};
use crate::css_properties::flex::FlexHandler;
use crate::css_properties::font::FontHandler;
use crate::css_properties::margin_padding::{
Expand All @@ -19,6 +19,7 @@ use crate::css_properties::text::Direction;
use crate::css_properties::transform::TransformHandler;
use crate::css_properties::transition::TransitionHandler;
use crate::css_properties::ui::ColorSchemeHandler;
use crate::css_values::ident::Ident;

pub type DeclarationList<'bump> = bun_alloc::ArenaVec<'bump, css::Property>;

Expand Down Expand Up @@ -46,6 +47,20 @@ impl<'bump> DeclarationBlock<'bump> {
self.declarations.len() + self.important_declarations.len()
}

/// Whether a declaration is an IE star hack (`*zoom: 1`).
pub(crate) fn has_star_hack(&self) -> bool {
self.declarations
.iter()
.chain(self.important_declarations.iter())
.any(|property| {
matches!(
property,
css::Property::Custom(custom)
if matches!(custom.name, CustomPropertyName::StarHack(_))
)
})
}

pub(crate) fn new_in(bump: &'bump Bump) -> Self {
Self {
important_declarations: DeclarationList::new_in(bump),
Expand Down Expand Up @@ -280,6 +295,20 @@ impl<'a, 'bump> css::DeclarationParser for PropertyDeclarationParser<'a, 'bump>
this.options,
)
}

fn parse_star_hack(
this: &mut Self,
name: &'static [u8],
input: &mut css::Parser,
) -> Result<Self::Declaration> {
parse_star_hack(
name,
input,
this.declarations,
this.important_declarations,
this.options,
)
}
}

impl<'a, 'bump> css::RuleBodyItemParser for PropertyDeclarationParser<'a, 'bump> {
Expand Down Expand Up @@ -386,6 +415,34 @@ where
Ok(())
}

/// The value of a `*property` declaration, read as the value of an unknown
/// property is read above. `name` keeps its `*`: it is not `property`.
#[cold]
pub(crate) fn parse_star_hack<'bump>(
name: &'static [u8],
input: &mut css::Parser,
declarations: &mut DeclarationList<'bump>,
important_declarations: &mut DeclarationList<'bump>,
options: &css::ParserOptions,
) -> Result<()> {
let name = CustomPropertyName::StarHack(Ident { v: name });
let value = input.parse_until_before(
css::Delimiters::BANG | css::Delimiters::CURLY_BRACKET,
|input2: &mut css::Parser| CustomProperty::parse(name, input2, options),
)?;
Comment on lines +429 to +432

@claude claude Bot Oct 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Users bundling a sheet whose keyframe block holds *background: url(./missing.png) now get a failed build where main exits 0. The star-hack value is parsed as a CustomProperty at src/css/declaration.rs:429-432, so every url() in it becomes an import record that the bundler must resolve. On main the * made the declaration an error, the recovering body dropped the block, and the build went on. Legacy sheets whose IE-only assets were pruned are exactly the population this hack appears in. Fix: a url() inside a *property value should not fail the build (resolve it best-effort and keep the raw text, or warn), for keyframes, @ page and style rules alike.

Why this was flagged

Trigger: a stylesheet containing @ keyframes k{from{*background:url(./missing.png)}to{top:2px}} or @ page{*background:url(./missing.png)} passed to bun build. The new path src/css/css_parser.rs:2903-2909 accepts the *ident and calls parse_star_hack at src/css/declaration.rs:421; CustomProperty::parse at src/css/declaration.rs:431 records the url() via add_import_record (src/css/css_parser.rs:3153) and the declaration succeeds, so reset never truncates it. On the base commit the * reaches the else branch at src/css/css_parser.rs:2920 as an unexpected token; the keyframe body drops the block and the build exits 0. After merge the bundler reports Could not resolve: "./missing.png" and the build fails. Population: legacy sheets with star-hacked IE asset paths whose images were removed. Remedy: do not turn a url() in a star-hack value into a hard import, or at least degrade it to a warning like esbuild does.

Verification: Trigger: a star-hack declaration inside a @ keyframes block whose url() names a file that does not exist. src/css/declaration.rs:429-432 parse_star_hack parses the value with CustomProperty::parse, which calls input.add_import_record(...); the bundler treats an unresolvable record as a hard error. On base the keyframes loop at src/css/css_parser.rs:1780-1784 drops the block and exits 0.

@robobun robobun Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept on purpose. A url() in the value of any unknown property is an import on main: _background: url(./missing.png) fails the build in the same way, in a style rule and in a keyframe block. A hack value that kept its url() as text would point at a file that the bundler did not copy, also when the file exists. The description lists the keyframe case as a new failure, and css/StarHackURLMissingFile pins the error. If a maintainer wants a different policy for hack values, it is a change in parse_star_hack only.

let important = input.try_parse(css::parse_important).is_ok();
input.expect_exhausted()?;

let property = css::Property::Custom(value);
if important {
important_declarations.push(property);
} else {
declarations.push(property);
Comment on lines +436 to +440

@claude claude Bot Oct 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Users minifying .a{width:1px;*width:2px;height:3px} get .a{*width:2px;width:1px;height:3px}: the hack is printed before the property it was written to override, inverting the IE cascade the author of the sheet relied on. The star-hack property is pushed as a plain Custom property at src/css/declaration.rs:436-440, and DeclarationBlock::minify at src/css/declaration.rs:93-101 moves unhandled Custom properties into hndlr.decls immediately while SizeHandler emits width only at finalize (src/css/declaration.rs:114). On main these sheets did not build at all, so no user got silently reordered output. Fix: keep a *prop (and _prop) in source order relative to the property it names, e.g. flush the handler's held property before pushing the hack, as #45825's flush_before_alias does.

Why this was flagged

Trigger: any stylesheet with width:1px;*width:2px (about 1,500 of 3,658 corpus hacks follow the property they name, per the PR) built with minify, through bun build --minify or Bun.CSS. parse_star_hack pushes Property::Custom into declarations at src/css/declaration.rs:440. In minify, handle at src/css/declaration.rs:90-103 calls handle_property; SizeHandler takes width and holds it, while the StarHack Custom property is not handled and is pushed to hndlr.decls at line 101; finalize at line 114 then appends width. Output is *width:2px;width:1px, pinned by test/js/bun/css/css.test.ts:195-196. The base branch rejected the whole sheet with Unexpected end of input, so this output is new for every sheet the PR unlocks (231 of 3,129 npm sheets). The _width precedent the dismissal relies on is the same bug, not a justification. Remedy: preserve relative order between a hack and the property it names (flush the held property before appending the hack, or never reorder unknown properties past handler-held ones).

Verification: A star-hack declaration that follows a property a shorthand handler owns (width:1px;*width:2px) is pushed as Property::Custom at src/css/declaration.rs:436-440 and goes to hndlr.decls at line 99-101, while width is only re-emitted in finalize, so it lands after the hack. For IE 6/7, width:1px wins instead of 2px. The base branch rejects the input outright, so nothing regresses.

@robobun robobun Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented under "Position" in the notes and pinned by a test beside its _width twin. No browser that Bun can target reads either hack, so the order changes nothing for them. A flush for StarHack alone makes *width and _width print in different orders in one rule. A flush for both hacks changes the output of sheets that build today. #45825 adds the flush mechanism (flush_before_alias), so I left the order of unknown properties to that work.

}

Ok(())
}

/// Per-shorthand-group handler state used by `DeclarationBlock::minify`.
pub struct DeclarationHandler<'bump> {
pub(crate) background: BackgroundHandler,
Expand Down
8 changes: 7 additions & 1 deletion src/css/properties/custom.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1561,6 +1561,9 @@ pub enum CustomPropertyName {
Custom(DashedIdent),
/// An unknown CSS property.
Unknown(Ident),
/// The IE 6/7 star hack, `*zoom`. Not an identifier: the source text of
/// the name with its `*`.
StarHack(Ident),
}

impl CustomPropertyName {
Expand All @@ -1576,6 +1579,9 @@ impl CustomPropertyName {
let v = unsafe { crate::arena_str(unknown.v) };
dest.serialize_identifier(v)
}
// Escaping the `*` would print the identifier `\*zoom`, which is
// not the hack. An escape in the name can end with a newline.
CustomPropertyName::StarHack(name) => dest.write_bytes(name.v()),
}
}

Expand All @@ -1594,7 +1600,7 @@ impl CustomPropertyName {
fn as_ptr(&self) -> *const [u8] {
match self {
CustomPropertyName::Custom(custom) => custom.v,
CustomPropertyName::Unknown(unknown) => unknown.v,
CustomPropertyName::Unknown(name) | CustomPropertyName::StarHack(name) => name.v,
}
}

Expand Down
14 changes: 14 additions & 0 deletions src/css/rules/page.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,20 @@ const _: () = {
this.options,
)
}

fn parse_star_hack(
this: &mut Self,
name: &'static [u8],
input: &mut Parser,
) -> Result<Self::Declaration> {
css::declaration::parse_star_hack(
name,
input,
&mut this.declarations.declarations,
&mut this.declarations.important_declarations,
this.options,
)
}
}

impl<'a> RuleBodyItemParser for PageRuleParser<'a> {
Expand Down
4 changes: 4 additions & 0 deletions src/css/rules/style.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,10 @@ impl<R> StyleRule<R> {
}
true
}
// `PropertyId` equality takes `*zoom` for `zoom`, and the loops do
// not pair a normal declaration with an important one.
&& !self.declarations.has_star_hack()
&& !other.declarations.has_star_hack()
Comment on lines +499 to +502

@claude claude Bot Oct 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: pre-existing: minified output still drops a whole style rule when an unknown property such as _zoom is re-declared later, because the new guard only exempts *zoom. is_duplicate at src/css/rules/style.rs:465 compares the total declaration count, then zips only the shorter of the normal and important lists, and PropertyId equality ignores custom names, so .a{zoom:1}.b{color:red}.a{_zoom:2!important} still erases .a{zoom:1}. Fix: make is_duplicate compare the normal and important lists separately by length and compare PropertyId::Custom names, which covers *zoom, _zoom and every other unknown property and lets has_star_hack() go.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Input .a{zoom:1}.b{color:red}.a{_zoom:2!important} (or any two unknown-property rules on the same selector, one normal and one !important) reaches StyleRuleKeyMap::remove_duplicate at src/css/rules/mod.rs:953 during minify. hash_key at src/css/rules/style.rs:44 hashes only the Custom tag, so both rules land in one bucket. is_duplicate at src/css/rules/style.rs:466 sees total lengths 1 == 1, then at 469-496 takes min(1, 0) and min(0, 1) as loop bounds, compares nothing, and returns true. The earlier rule is removed from the output. The diff adds has_star_hack() at src/css/rules/style.rs:501-502 which exempts only CustomPropertyName::StarHack; _zoom and other Unknown names still take the pre-existing wrong path, so users get the same lost rule the base branch already produces. The author notes #33684 as the follow-up for the name comparison; the normal-vs-important length mismatch is a separate defect that a two-line length check in this function would close for every property kind.

Verification: pre-existing; acknowledged in diff: the guard added at src/css/rules/style.rs:501-502 exempts only CustomPropertyName::StarHack, so every other unknown property (_zoom, etc.) still hits it. Triggering condition: two non-adjacent style rules with identical selectors and the same number of unknown-property declarations. The base branch drops .a{zoom:1} by the identical route for _zoom.

@robobun robobun Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the defect that #33684 fixes at its owner: PropertyId equality compares custom names there, and is_duplicate compares the normal and the important list separately. It exists on main for _zoom and for every other unknown property. This PR does not copy that change. The guard only keeps the new StarHack variant from reaching the defect. When #33684 lands, has_star_hack() and the guard can be deleted.

}
}

Expand Down
Loading
Loading