Skip to content

Preserve spacing for container feature functions#4441

Open
puneetdixit200 wants to merge 2 commits into
less:masterfrom
puneetdixit200:4235-container-style-query
Open

Preserve spacing for container feature functions#4441
puneetdixit200 wants to merge 2 commits into
less:masterfrom
puneetdixit200:4235-container-style-query

Conversation

@puneetdixit200
Copy link
Copy Markdown

@puneetdixit200 puneetdixit200 commented May 21, 2026

Fixes #4235

Summary

  • preserve function-style spacing for container style and scroll-state queries
  • recognize CSS custom identifiers, uppercase names, underscores, escapes, and non-ASCII names when deciding whether a container feature had source spacing
  • add container fixture coverage for boolean style queries, query lists, scroll-state lists, and named containers

Testing

  • COREPACK_HOME=$PWD/.corepack PNPM_HOME=$PWD/.pnpm-home pnpm install --frozen-lockfile --store-dir $PWD/.pnpm-store
  • COREPACK_HOME=$PWD/.corepack PNPM_HOME=$PWD/.pnpm-home pnpm --dir packages/less exec node test/index.js container
  • COREPACK_HOME=$PWD/.corepack PNPM_HOME=$PWD/.pnpm-home pnpm --dir packages/less exec eslint --no-ignore lib/less/parser/parser.js
  • COREPACK_HOME=$PWD/.corepack PNPM_HOME=$PWD/.pnpm-home pnpm --dir packages/less run typecheck
  • COREPACK_HOME=$PWD/.corepack PNPM_HOME=$PWD/.pnpm-home pnpm --dir packages/less run test:node
  • git diff --check

Note: the commit hook also ran the repo npm test path. Node/build portions passed; the browser runner reported missing local Playwright browser binaries and therefore did not execute those browser pages in this environment.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of container queries and media-feature spacing so combined queries and parenthesized conditions render and apply consistently.
  • Tests

    • Added extensive unit tests for container queries: combined conditions, scroll-state and style() conditions, escaped and non‑ASCII identifiers, and varied container naming patterns.

Review Change Stack

  • COREPACK_HOME=$PWD/.corepack PNPM_HOME=$PWD/.pnpm-home pnpm --dir packages/less exec eslint --no-ignore lib/less/parser/parser.js
  • COREPACK_HOME=$PWD/.corepack PNPM_HOME=$PWD/.pnpm-home pnpm --dir packages/less run typecheck
  • Added coverage for non-ASCII container names after node test/index.js container exposed the parser gap.

Signed-off-by: Puneet Dixit <236133619+puneetdixit200@users.noreply.github.com>
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label May 21, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

📝 Walkthrough

Walkthrough

Parser media feature logic is updated to correctly detect and propagate whitespace behavior for parentheses-wrapped query and declaration nodes. The spacing detection regex is broadened, tree.Paren nodes conditionally receive noSpacing markers when no spacing is detected, and final expressions matching a keyword/variable-plus-paren pattern are also marked noSpacing. Container query test cases validate the fix across style queries, scroll-state conditions, and various container-name formats.

Changes

Container Style Query Spacing

Layer / File(s) Summary
Media feature spacing and noSpacing propagation
packages/less/lib/less/parser/parser.js
Regex for spacing detection before parentheses is broadened to match additional token patterns. tree.Paren nodes are now constructed into variables to conditionally set noSpacing when spacing is absent. Final tree.Expression is marked noSpacing when it matches a two-node pattern: a Keyword/Variable followed by a Paren already marked noSpacing.
Container query test coverage
packages/test-data/tests-unit/container/container.css, packages/test-data/tests-unit/container/container.less
Test data expanded to cover @container style queries with scroll-state(stuck: top), multiple style-query variants (style(--theme), style((--theme: one) or (--theme: two))), and container-name forms including contactBody, underscored, dashed, escaped, and non-ASCII identifiers. New sidebar container query with scroll-state condition updated to set #sticky-child font-size to 80%.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • less/less.js#4427: Modifies media feature parsing in the same parser file to adjust Paren construction and closure/spacing handling for query-in-parentheses nodes.

Suggested labels

size:S

Poem

🐰 A space slipped in where none belong,
The parser hummed and fixed the song.
noSpacing now holds firm and tight,
Container queries snug and right.
Hops of joy — styles back in sight!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and accurately describes the main change: preserving spacing for container feature functions, which aligns with the parser modification and the core objective of fixing spacing issues in container queries.
Linked Issues check ✅ Passed The code changes address issue #4235's requirements by enhancing whitespace detection heuristics in the parser to recognize valid CSS identifiers, and the test additions cover various container-query scenarios with different identifier styles.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the issue objectives: parser modifications for spacing detection, container test cases covering various identifier and query types, with no unrelated alterations.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/test-data/tests-unit/container/container.less (1)

180-202: ⚡ Quick win

Add one explicit non-ASCII container-name fixture case.

The new fixtures cover many identifier forms, but they still miss a direct non-ASCII container name case called out in the objective. Add one example here and mirror it in packages/test-data/tests-unit/container/container.css.

Suggested fixture addition
+@container café (min-width: 1300px) {
+    .unicode-container {
+        width: 25%;
+    }
+}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/test-data/tests-unit/container/container.less` around lines 180 -
202, Add one explicit non-ASCII container-name fixture by inserting an
`@container` rule with a non-ASCII identifier (for example `@container` контейнер
(min-width: 1300px) { .nonascii-container { width: 25%; } }) into
packages/test-data/tests-unit/container/container.less alongside the existing
`@container` rules (contactBody, _body, --body, contact\.body), and mirror that
exact rule (same container name and selector .nonascii-container) in
packages/test-data/tests-unit/container/container.css so the test fixture covers
a direct non-ASCII container-name case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/test-data/tests-unit/container/container.less`:
- Around line 180-202: Add one explicit non-ASCII container-name fixture by
inserting an `@container` rule with a non-ASCII identifier (for example `@container`
контейнер (min-width: 1300px) { .nonascii-container { width: 25%; } }) into
packages/test-data/tests-unit/container/container.less alongside the existing
`@container` rules (contactBody, _body, --body, contact\.body), and mirror that
exact rule (same container name and selector .nonascii-container) in
packages/test-data/tests-unit/container/container.css so the test fixture covers
a direct non-ASCII container-name case.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8a2ce99b-6e52-4375-b229-a9c90a0299c8

📥 Commits

Reviewing files that changed from the base of the PR and between 060fd7f and dd7e0d7.

📒 Files selected for processing (3)
  • packages/less/lib/less/parser/parser.js
  • packages/test-data/tests-unit/container/container.css
  • packages/test-data/tests-unit/container/container.less

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

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Container style queries unexpected space

1 participant