Skip to content

fix(id): escape values assigned through the value setter - #326

Open
mahirhir wants to merge 1 commit into
postcss:mainfrom
mahirhir:fix-id-value-escaping
Open

fix(id): escape values assigned through the value setter#326
mahirhir wants to merge 1 commit into
postcss:mainfrom
mahirhir:fix-id-value-escaping

Conversation

@mahirhir

Copy link
Copy Markdown

ID doesn't override value, so assigning to id.value stores the string as-is and toString() emits it unescaped. ClassName has had an escaping value setter since d4dda8c5 ("Escape class name values by default", 2018), but ID never got the same one, even though class and id selectors are both identifiers and follow the same escaping rules.

The result is wrong output for any id value that contains characters needing escapes:

const parser = require("postcss-selector-parser");

const cls = parser.className({ value: "x" });
cls.value = "foo.bar";
cls.toString(); // ".foo\\.bar"   (correct)

const id = parser.id({ value: "x" });
id.value = "foo.bar";
id.toString(); // "#foo.bar"   (wrong: re-parses to id "foo" + class "bar")

Leading digits, spaces and # break the same way: #1abc, #a b, #a#b.

This adds the same setter ClassName uses (cssesc(v, { isIdentifier: true }) stored into raws.value), so id values escape on assignment:

id.value = "foo.bar";
id.toString(); // "#foo\\.bar"

The new test mirrors the existing ClassName#set value test. npm test passes (lint, typecheck, full suite).

ID had no value setter, so a programmatically assigned id value was
emitted verbatim instead of escaped. ClassName has escaped assigned
values since 2018; mirror the same setter on ID.
@MoOx

MoOx commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks. This is a real gap, and reusing ClassName's setter verbatim is the right call. ID and ClassName are both identifiers; there's no reason they'd escape differently.

I ran the branch against main: merges cleanly, suite green. I also compared id.value = x against className.value = x across a range of values (foo.bar, 1abc, a b, a#b, @md:flex, non-ASCII) — the output is now identical between the two node types, which is exactly what I wanted to confirm.

This fixes more than the description says

On an ID parsed with an escape, reassigning value had no effect at all, because raws.value survived:

const id = parser().astSync('#fo\\o').first.first;
id.value = 'bar';
id.toString(); // main: '#fo\o'   branch: '#bar'

Your test covers it (id.raws going back to {}), it just isn't called out. Could you add a line about it to the PR description? It's the strongest argument for merging and I'd like it in the changelog.

Docs

API.md doesn't document the escaping behaviour for className either, so I won't make it a condition here. But if you feel like adding a short note under parser.id([props]) and parser.className([props]) — values are escaped on assignment, and setPropertyWithoutEscape() is the escape hatch for pre-escaped input — that'd be a welcome bonus. setPropertyWithoutEscape is currently undocumented, which is part of why this trips people up.

On the release

This changes observable behaviour for existing code: anyone assigning an already-escaped value now gets a double escape.

id.value = 'foo\\.bar'; // main: '#foo\.bar'   branch: '#foo\\\.bar'

That's the same footgun ClassName has had since d4dda8c (2018-03-26), so it's consistency rather than a new hazard, and the previous behaviour was emitting invalid selectors (#foo.bar re-parses as an id plus a class). My inclination is to ship it as a minor (7.2.0) with an explicit changelog note, not a major.

This package sits deep in a lot of dependency trees, so I'd like a second opinion: does minor seem right to you, or would you go major?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants