Skip to content

Fix #297799: color picker in private fields #307902

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

Open
martacbraga wants to merge 1 commit into
base: main
Choose a base branch
from
+15 −2
Open

Fix #297799: color picker in private fields #307902

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
2 changes: 1 addition & 1 deletion src/vs/editor/common/languages/defaultDocumentColorsComputer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ function _findMatches(model: IDocumentColorComputerTarget | string, regex: RegEx
function computeColors(model: IDocumentColorComputerTarget): IColorInformation[] {
const result: IColorInformation[] = [];
// Early validation for RGB and HSL (including CSS Level 4 syntax with / separator)
const initialValidationRegex = /\b(rgb|rgba|hsl|hsla)(\([0-9\s,.\%\/]*\))|^(#)([A-Fa-f0-9]{3})\b|^(#)([A-Fa-f0-9]{4})\b|^(#)([A-Fa-f0-9]{6})\b|^(#)([A-Fa-f0-9]{8})\b|(?<=['"\s])(#)([A-Fa-f0-9]{3})\b|(?<=['"\s])(#)([A-Fa-f0-9]{4})\b|(?<=['"\s])(#)([A-Fa-f0-9]{6})\b|(?<=['"\s])(#)([A-Fa-f0-9]{8})\b/gm;
const initialValidationRegex = /\b(rgb|rgba|hsl|hsla)(\([0-9\s,.\%\/]*\))|^(#)([A-Fa-f0-9]{3})\b|^(#)([A-Fa-f0-9]{4})\b|^(#)([A-Fa-f0-9]{6})\b|^(#)([A-Fa-f0-9]{8})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{3})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{4})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{6})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{8})\b/gm;
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

This regex still matches bare hex colors at the start of a line via the ^(#)([A-Fa-f0-9]{...}) alternatives. That means a private field written without indentation (e.g. class Foo {\n#facade;\n}) would still be detected as a color, so the false positive isn’t fully eliminated. Consider removing the start-of-line hex alternatives (to align with the PR description of only matching inside strings), or otherwise tightening the non-string matching so it can’t match valid private field syntax.

Suggested change
const initialValidationRegex = /\b(rgb|rgba|hsl|hsla)(\([0-9\s,.\%\/]*\))|^(#)([A-Fa-f0-9]{3})\b|^(#)([A-Fa-f0-9]{4})\b|^(#)([A-Fa-f0-9]{6})\b|^(#)([A-Fa-f0-9]{8})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{3})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{4})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{6})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{8})\b/gm;
const initialValidationRegex = /\b(rgb|rgba|hsl|hsla)(\([0-9\s,.\%\/]*\))|(?<=['"][ ]*)(#)([A-Fa-f0-9]{3})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{4})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{6})\b|(?<=['"][ ]*)(#)([A-Fa-f0-9]{8})\b/gm;

Copilot uses AI. Check for mistakes.
const initialValidationMatches = _findMatches(model, initialValidationRegex);
Comment on lines 103 to 105
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

The new lookbehind (?<=['"][ ]*) is variable-length because of *, which is not supported by the JS RegExp engine and will throw a SyntaxError when this module is parsed. Please rewrite this part of the regex without variable-length lookbehind (e.g. match the quote/whitespace in the overall match and adjust the computed range accordingly, or use only fixed-length lookbehinds).

Copilot uses AI. Check for mistakes.

// Potential colors have been found, validate the parameters
Expand Down
15 changes: 14 additions & 1 deletion src/vs/editor/test/common/languages/defaultDocumentColorsComputer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ suite('Default Document Colors Computer', () => {
const testCases = [
{ content: `const color = ' #ff0000';`, name: 'hex with space before' },
{ content: '#ff0000', name: 'hex at start of line' },
{ content: ' #ff0000', name: 'hex with whitespace before' }
];

testCases.forEach(testCase => {
Expand Down Expand Up @@ -188,4 +187,18 @@ suite('Default Document Colors Computer', () => {
assert.strictEqual(colors.length, 1, `Should detect rgb/rgba color with ${testCase.name}: ${testCase.content}`);
});
});

test('JavaScript private fields with # should not be recognized as colors', () => {
// Test case from issue #297799
const testCases = [
{ content: 'class Foo {\n\t#facade\n}', name: 'private field #facade should not be a color' },
{ content: 'class Component {\n\treadonly #facade = inject(Facade);\n}', name: 'private field #facade with injection should not be a color' },
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

The added regression test only covers a 6-hex-digit private field name (#facade). Since the matcher has separate branches for 3/4/6/8-digit hex, it would be good to add a couple of additional cases (e.g. #fed, #fedc, #fedbca0/#fedbcaff) and also a no-indentation variant to ensure private fields aren’t detected regardless of formatting.

Suggested change
{ content: 'class Component {\n\treadonly #facade = inject(Facade);\n}', name: 'private field #facade with injection should not be a color' },
{ content: 'class Component {\n\treadonly #facade = inject(Facade);\n}', name: 'private field #facade with injection should not be a color' },
{ content: 'class Foo {\n\t#fed\n}', name: 'private field #fed should not be a color' },
{ content: 'class Foo {\n\t#fedc\n}', name: 'private field #fedc should not be a color' },
{ content: 'class Foo {\n\t#fedbca0\n}', name: 'private field #fedbca0 should not be a color' },
{ content: 'class Foo {\n\t#fedbcaff\n}', name: 'private field #fedbcaff should not be a color' },
{ content: 'class Foo {\n#fed\n}', name: 'private field #fed without indentation should not be a color' },

Copilot uses AI. Check for mistakes.
];

testCases.forEach(testCase => {
const model = new TestDocumentModel(testCase.content);
const colors = computeDefaultDocumentColors(model);
assert.strictEqual(colors.length, 0, `Should not detect color with ${testCase.name}: ${testCase.content}`);
});
});
});
Loading