Skip to content

Commit 56bf191

Browse files
xxiaoxiongxxiaoxiong
committed
fix(#6250 round-6): tokenize attribute selectors; apply attributeMatchers on parsed tokens only
PerishCode round-6 CHANGES_REQUESTED on PR #6250 (commit 7ea2570): The round-5 implementation removed `selectorMatchers` from the classification path but kept `attributeMatchers` running against the raw selector string. PerishCode reproduced three cross-attribution violations: 1. `:not([type=submit])` was admitted into Buttons.selectors because the `[type=submit]` text appears in the raw selector and `/\[type=...(button|submit|reset)/` matched it — even though `:not()` is component-erasing and the predicate sits inside an opaque pseudo-class. 2. `[data-label="[type=submit]"]` was admitted into Buttons because the attribute VALUE spells `[type=submit]` and the un-anchored matcher regex matched the substring inside the value text. The selector target's actual attribute is `data-label`, not `type`. 3. `:not([aria-hidden="true"])` was admitted into Icons for the same reason as (1) — `[aria-hidden="true"]` text inside `:not()` matched the unanchored Icons matcher. Round-6 fix: - `tokenizeCompound` now also collects attribute-selector tokens from each compound. Quoted strings inside the attribute value are respected, so `[data-label="[type=submit]"]` is captured as ONE token (the inner `[type=submit]` is hidden inside the quoted value, not split out). - `:is()` / `:where()` recursion now unions the inner compounds' attribute tokens into the parent compound's attribute list, so `:is(input[type="submit"])` still admits to Buttons. - `:not()` / `:has()` remain opaque (their argument compounds are never tokenized), so `[type="submit"]` inside `:not(...)` never enters the parsed attribute list, and the matcher pipeline never sees it. - `attributeMatchers` regexes are now anchored at token start (`^\[...`) and require the closing `]`, so a long attribute token whose value merely *contains* attribute-looking text can no longer match (substring search is no longer possible once the regex is bound to the full token). - `selectorMatchesTokens` runs `attributeMatchers` against the parsed attribute tokens (per compound) instead of the raw selector string. The early-return path that bypassed the opacity rule is gone. Round-6 fixture matrix (`tests/components-manifest-6250-not-and-attribute-opacity.test.ts`): Negative cases (all newly added — none of these passed before): - `:not(.btn)` does NOT admit to Buttons (round-5, still passes) - `[data-label=".card"]` does NOT admit to Cards (round-5, still passes) - `:not([type="submit"])` does NOT admit to Buttons — NEW - `:not([aria-hidden="true"])` does NOT admit to Icons — NEW - `[data-label="[type=submit]"]` does NOT admit to Buttons — NEW Positive controls (must keep working): - `[type="submit"]` admits to Buttons and contributes `--tone-button-attr` to `Buttons.tokenReferences` - `[aria-hidden="true"]` admits to Icons and contributes `--tone-icon` to `Icons.tokenReferences` Negative token assertions are now written as `r === 'X' || r === '--X'` so the assertion fires regardless of which form the manifest stores (the previous round-5 assertions compared against the unprefixed token name, which silently passed even when the manifest stored the prefixed form). Validation on this head: pnpm --filter @open-design/contracts test -> 295 passed (44 files), was 290 before this round (5 new fixtures) Signed-off-by: xxiaoxiong <2482929840@qq.com>
1 parent 7ea2570 commit 56bf191

2 files changed

Lines changed: 119 additions & 18 deletions

File tree

‎packages/contracts/src/design-systems/components-manifest.ts‎

Lines changed: 59 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ const COMPONENT_GROUPS: ComponentGroupDefinition[] = [
101101
id: 'buttons',
102102
label: 'Buttons and calls to action',
103103
selectorMatchers: [/^(?:\.)?button(?:$|[-_:])/i, /\.btn(?:$|[-_:])/i, /\[type=["']?(?:button|submit|reset)/i],
104-
attributeMatchers: [/\[type=["']?(?:button|submit|reset)/i],
104+
attributeMatchers: [/^\[type=["']?(?:button|submit|reset)["']?\]/i],
105105
classMatchers: [/^btn(?:$|-)/i, /^button(?:$|-)/i, /^cta(?:$|-)/i],
106106
elementMatchers: [/^button$/i],
107107
},
@@ -191,7 +191,7 @@ const COMPONENT_GROUPS: ComponentGroupDefinition[] = [
191191
id: 'icons',
192192
label: 'Icon slots',
193193
selectorMatchers: [/\.icon(?:$|[-_:])/i, /\[aria-hidden=["']true["']\]/i],
194-
attributeMatchers: [/\[aria-hidden=["']true["']\]/i],
194+
attributeMatchers: [/^\[aria-hidden=["']true["']\]/i],
195195
classMatchers: [/^icon(?:$|-)/i],
196196
elementMatchers: [/^svg$/i],
197197
},
@@ -350,6 +350,17 @@ interface CompoundTokens {
350350
element: string | null;
351351
classes: string[];
352352
extraElements: string[];
353+
/**
354+
* Attribule-selector tokens parsed from this compound. Each entry is the
355+
* verbatim text of one attribute selector (e.g. `[type="submit"]`) WITH
356+
* balanced quote/bracket handling — quoted strings inside the attribute
357+
* value are respected so a value like `[type="submit]"]` is scanned as a
358+
* single token. Tokens from `:not(...)` / `:has(...)` are NOT included:
359+
* those pseudos are opaque (their arguments do NOT positively select),
360+
* so their attribute predicates must not cross-attribute the selectors
361+
* into component groups.
362+
*/
363+
attributeSelectors: string[];
353364
}
354365

355366
// Tokenize one compound selector (no combinators) into its element name and
@@ -374,6 +385,7 @@ function tokenizeCompound(compound: string): CompoundTokens {
374385
let element: string | null = null;
375386
const classes: string[] = [];
376387
const extraElements: string[] = [];
388+
const attributeSelectors: string[] = [];
377389
let i = 0;
378390
// leading element name (type selector) — must come first in the compound
379391
const elementMatch = /^([a-zA-Z][a-zA-Z0-9_-]*)/.exec(compound);
@@ -390,6 +402,30 @@ function tokenizeCompound(compound: string): CompoundTokens {
390402
i += classMatch[0].length;
391403
continue;
392404
}
405+
// attribute selector: `[name? op? value? flags?]` — scan balanced bracket
406+
// and respect quoted strings inside the value so an attribute value that
407+
// contains `[...]` itself (e.g. `[data-label="[type=submit]"]`) is captured
408+
// as ONE token, not split.
409+
const attributeMatch = /^\[/.exec(rest);
410+
if (attributeMatch) {
411+
let j = 1; // skip leading '['
412+
let inQuote: string | null = null;
413+
while (j < rest.length) {
414+
const c = rest[j];
415+
if (inQuote) {
416+
if (c === inQuote && rest[j - 1] !== '\\') inQuote = null;
417+
} else if (c === '"' || c === '\'') {
418+
inQuote = c;
419+
} else if (c === ']') {
420+
break;
421+
}
422+
j += 1;
423+
}
424+
const tokenEnd = j >= rest.length ? rest.length : j + 1; // include closing ']'
425+
attributeSelectors.push(rest.slice(0, tokenEnd));
426+
i += tokenEnd;
427+
continue;
428+
}
393429
const preserveMatch = /^:(?:is|where)\s*\(/i.exec(rest);
394430
if (preserveMatch) {
395431
// capture the balanced `(...)` block so nested parens are respected
@@ -432,6 +468,7 @@ function tokenizeCompound(compound: string): CompoundTokens {
432468
if (element == null) element = extra;
433469
else extraElements.push(extra);
434470
}
471+
for (const attr of subTokens.attributeSelectors) attributeSelectors.push(attr);
435472
}
436473
}
437474
i = j + 1; // consume `:...(...)`
@@ -446,7 +483,7 @@ function tokenizeCompound(compound: string): CompoundTokens {
446483
// anything else (one char) we cannot tokenize — bail forward
447484
i += 1;
448485
}
449-
return { element, classes, extraElements };
486+
return { element, classes, extraElements, attributeSelectors };
450487
}
451488

452489
// Decide whether a selector belongs to a component group by examining its
@@ -457,20 +494,16 @@ function tokenizeCompound(compound: string): CompoundTokens {
457494
// regression. The matcher now:
458495
// 1. checks each compound's element token against `elementMatchers`
459496
// 2. checks each compound's class tokens against `classMatchers`
460-
// 3. keeps `selectorMatchers` for attribute selectors and other cases that
461-
// are not expressible as element/class tokens (e.g. `[type=button]`,
462-
// `[aria-hidden="true"]`); these match the full selector string as before.
463-
// Any compound passing any of the three matcher families admits the selector.
497+
// 3. checks each compound's attribute selectors against `attributeMatchers`
498+
// (so `input[type="submit"]` admits the selector to Buttons; previously
499+
// the raw selector was matched but tokenization lost the attribute text)
500+
// 4. keeps `selectorMatchers` for cases that are not expressible as
501+
// element/class/attribute tokens; these match the full selector string.
502+
// Any compound passing any of the four matcher families admits the selector.
464503
function selectorMatchesTokens(selector: string, definition: ComponentGroupDefinition): boolean {
465-
// Attribute predicate matchers run against the raw selector because
466-
// attribute values appear verbatim in the compound (e.g. `[type="button"]`
467-
// contains the class-like text `.button` inside the attribute value);
468-
// tokenizing would lose that context and the predicate must run on the
469-
// unmodified text.
470-
if (definition.attributeMatchers.some((matcher) => matcher.test(selector))) return true;
471504
const compounds = splitCompoundSelectors(selector);
472505
for (const compound of compounds) {
473-
const { element, classes, extraElements } = tokenizeCompound(compound);
506+
const { element, classes, extraElements, attributeSelectors } = tokenizeCompound(compound);
474507
// element token (and any extra element tokens pulled out of :is()/:where())
475508
// joined together so multi-element compounds can match several groups.
476509
const elementCandidates = element ? [element, ...extraElements] : extraElements;
@@ -480,6 +513,18 @@ function selectorMatchesTokens(selector: string, definition: ComponentGroupDefin
480513
for (const className of classes) {
481514
if (definition.classMatchers.some((matcher) => matcher.test(className))) return true;
482515
}
516+
// Attribute predicates run against the parsed attribute-selector tokens
517+
// (e.g. `[type="submit"]`, `[aria-hidden="true"]`) rather than the raw
518+
// compound text, so opacity rules still apply: a `[type="submit"]` that
519+
// sits *inside* `:not(...)` never reaches this list (tokenizeCompound
520+
// leaves `:not()` opaque), and an attribute *value* containing attribute
521+
// text (e.g. `[data-label="[type=submit]"]`) is captured as ONE token
522+
// — the matcher regex tests the outer bracket, not the inner quoted
523+
// bracket — so it does not admit the selector to Buttons just because
524+
// the value happens to spell `[type=submit]`.
525+
for (const attrToken of attributeSelectors) {
526+
if (definition.attributeMatchers.some((matcher) => matcher.test(attrToken))) return true;
527+
}
483528
}
484529
return false;
485530
}

‎packages/contracts/tests/components-manifest-6250-not-and-attribute-opacity.test.ts‎

Lines changed: 60 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ function findGroup(
1515
//
1616
// With the round-3/round-4 implementation, the regex `selectorMatchers` family
1717
// still ran against the raw selector before tokenizeCompound, so:
18-
// - `:not(.btn) { color: var(--tone) }` matched `/\.btn(?:$|[-_:])/i` and
18+
// - `:not(.btn) { color: var(--tone) }` matched `/.btn(?:$|[-_:])/i` and
1919
// admitted this into Buttons.selectors / Buttons.tokenReferences even
2020
// though `:not(.btn)` selects *non*-button elements.
21-
// - `[data-label=".card"] { color: var(--tone) }` matched `/\.card(?:$|[-_:])/i`
21+
// - `[data-label=".card"] { color: var(--tone) }` matched `/.card(?:$|[-_:])/i`
2222
// inside the attribute value and admitted this into Cards even though the
2323
// selector targets an element whose `[data-label]` happens to mention a
2424
// card name.
@@ -28,6 +28,24 @@ function findGroup(
2828
// predicates like `[type=button]` / `[aria-hidden="true"]` / `[role=checkbox]`)
2929
// run against the raw selector; element + class matching happens strictly on
3030
// parsed tokens via tokenizeCompound (which itself keeps `:not()` *opaque*).
31+
//
32+
// Round-6 fixture matrix (PerishCode round-6 blocker):
33+
// attribute predicates themselves must run on PARSED attribute-selector
34+
// tokens, NOT on the raw compound string. Same opacity rule that round-5
35+
// applied to class matchers now applies to attribute matchers:
36+
// - `:not([type=submit]) { color: var(--tone) }` must NOT admit to Buttons
37+
// even though `[type=submit]` text appears in the raw selector — the
38+
// predicate sits inside `:not()` which is component-erasing.
39+
// - `[data-label="[type=submit]"] { color: var(--tone) }` must NOT admit to
40+
// Buttons even though the attribute VALUE spells `[type=submit]` — it is
41+
// a data-label value, not a type attribute. The matcher regex should
42+
// run against the outer attribute token `[data-label="[type=submit]"]`,
43+
// not the inner quoted bracket.
44+
// - `:not([aria-hidden="true"]) { color: var(--tone) }` must NOT admit to
45+
// Icons (same logic as the buttons :not case).
46+
// Positive controls `[type=submit]` and `[aria-hidden="true"]` continue to
47+
// admit selectors to Buttons / Icons respectively so the matcher pipeline
48+
// can still see genuine attribute predicates.
3149

3250
describe('#6250 round-5 — :not() and attribute-value opacity', () => {
3351
const html = `
@@ -36,6 +54,9 @@ describe('#6250 round-5 — :not() and attribute-value opacity', () => {
3654
[data-label=".card"] { color: var(--tone-card) }
3755
[type="submit"] { color: var(--tone-button-attr) }
3856
[aria-hidden="true"] { color: var(--tone-icon) }
57+
:not([type="submit"]) { color: var(--tone-button-not-attr) }
58+
:not([aria-hidden="true"]) { color: var(--tone-icon-not-attr) }
59+
[data-label="[type=submit]"] { color: var(--tone-button-attr-leak) }
3960
</style>
4061
<button>real button</button>
4162
`;
@@ -55,7 +76,7 @@ describe('#6250 round-5 — :not() and attribute-value opacity', () => {
5576
it(':not(.btn) does not contribute Buttons.tokenReferences either', () => {
5677
const buttons = findGroup(manifest, 'buttons');
5778
expect(buttons).toBeDefined();
58-
expect(buttons!.tokenReferences.some((r) => r === 'tone-button')).toBe(false);
79+
expect(buttons!.tokenReferences.some((r) => r === 'tone-button' || r === '--tone-button')).toBe(false);
5980
});
6081

6182
it('[data-label=".card"] is NOT admitted to Cards.selectors', () => {
@@ -68,7 +89,7 @@ describe('#6250 round-5 — :not() and attribute-value opacity', () => {
6889
it('[data-label=".card"] does not contribute Cards.tokenReferences either', () => {
6990
const cards = findGroup(manifest, 'cards');
7091
expect(cards).toBeDefined();
71-
expect(cards!.tokenReferences.some((r) => r === 'tone-card')).toBe(false);
92+
expect(cards!.tokenReferences.some((r) => r === 'tone-card' || r === '--tone-card')).toBe(false);
7293
});
7394

7495
it('[type="submit"] IS still admitted to Buttons via attributeMatchers', () => {
@@ -82,4 +103,39 @@ describe('#6250 round-5 — :not() and attribute-value opacity', () => {
82103
expect(icons).toBeDefined();
83104
expect(icons!.selectors.some((s) => s.includes('[aria-hidden="true"]'))).toBe(true);
84105
});
106+
107+
// ----- Round-6: attribute predicate opacity (PerishCode round-6 blocker) -----
108+
109+
it(':not([type="submit"]) is NOT admitted to Buttons — :not() erases attribute too', () => {
110+
const buttons = findGroup(manifest, 'buttons');
111+
expect(buttons).toBeDefined();
112+
expect(buttons!.selectors.some((s) => s.includes(':not([type="submit"])'))).toBe(false);
113+
expect(buttons!.tokenReferences.some((r) => r === 'tone-button-not-attr' || r === '--tone-button-not-attr')).toBe(false);
114+
});
115+
116+
it(':not([aria-hidden="true"]) is NOT admitted to Icons — :not() erases attribute too', () => {
117+
const icons = findGroup(manifest, 'icons');
118+
expect(icons).toBeDefined();
119+
expect(icons!.selectors.some((s) => s.includes(':not([aria-hidden="true"])'))).toBe(false);
120+
expect(icons!.tokenReferences.some((r) => r === 'tone-icon-not-attr' || r === '--tone-icon-not-attr')).toBe(false);
121+
});
122+
123+
it('[data-label="[type=submit]"] is NOT admitted to Buttons — value text is not a real predicate', () => {
124+
const buttons = findGroup(manifest, 'buttons');
125+
expect(buttons).toBeDefined();
126+
expect(buttons!.selectors.some((s) => s.includes('[data-label="[type=submit]"]'))).toBe(false);
127+
expect(buttons!.tokenReferences.some((r) => r === 'tone-button-attr-leak' || r === '--tone-button-attr-leak')).toBe(false);
128+
});
129+
130+
it('positive control: [type="submit"] still contributes Buttons.tokenReferences', () => {
131+
const buttons = findGroup(manifest, 'buttons');
132+
expect(buttons).toBeDefined();
133+
expect(buttons!.tokenReferences.some((r) => r === '--tone-button-attr')).toBe(true);
134+
});
135+
136+
it('positive control: [aria-hidden="true"] still contributes Icons.tokenReferences', () => {
137+
const icons = findGroup(manifest, 'icons');
138+
expect(icons).toBeDefined();
139+
expect(icons!.tokenReferences.some((r) => r === '--tone-icon')).toBe(true);
140+
});
85141
});

0 commit comments

Comments
 (0)