Skip to content

Preserve spacing for container feature functions#4441

Open
puneetdixit200 wants to merge 1 commit into
less:masterfrom
puneetdixit200:4235-container-style-query
Open

Preserve spacing for container feature functions#4441
puneetdixit200 wants to merge 1 commit 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 parser handling of container queries and media features with enhanced whitespace detection and spacing propagation.
  • Tests

    • Added comprehensive test coverage for CSS container queries, including support for scroll-state conditions, complex style-based query conditions, escaped identifiers, and various container naming patterns.

Review Change Stack

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, and escaped identifiers. New sidebar container query with scroll-state condition added.

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.

Poem

🐰 A space crept in where none should be,
Between the keyword and the curly spree!
No more—the parser marks its spacing clear,
noSpacing whispers: remove that tear.
Container queries bloom in perfect form,
A rabbit's fix to styling norm! 🌸

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly matches the PR's main purpose: preserving spacing for container feature functions by fixing the parser.
Linked Issues check ✅ Passed The code changes address issue #4235 by updating the parser to detect spacing around parentheses-wrapped nodes and conditionally mark them as noSpacing to preserve function-style formatting.
Out of Scope Changes check ✅ Passed All changes directly support the objective: parser updates for spacing detection and test fixtures for container queries with various identifier forms and style/scroll-state queries.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

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

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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