Allow :is() and :where() selectors#789
Conversation
1a78461 to
77e6a17
Compare
bf9aeeb to
a92102d
Compare
1e4af27 to
15263f7
Compare
|
Have successfully moved the whole updated functionality to the |
|
The build and linter are failing. You can run tests locally or by actions on your fork and opening a PR against the fork. Please feel free to request a review once the basic build/tests/linter are passing. If you encounter issues testing it locally let me know. |
Ok Sure. |
|
All builds and tests pass locally except @lumino/datagrid:test, which fails due to known text rendering differences in headless Chromium. |
|
I think it is ready for the review. |
|
Requesting a review @krassowski . |
krassowski
left a comment
There was a problem hiding this comment.
This PR should not be modifying 23 files. Can you please revert the unrelated changes to yaml and html files please?
I ran prettier on every file and fixed the error in the previous commit, I think it is a result of running prettier on all files. |
|
|
|
I’m having a bit of trouble understanding how to implement this change. |
e990ec6 to
18d052e
Compare
|
@krassowski |
|
Requesting a review. |
krassowski
left a comment
There was a problem hiding this comment.
Just some minor things to finish here
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
|
Hi @krassowski , have fixed the small changes. Hope now its good to be merged |
There was a problem hiding this comment.
Pull request overview
This PR aims to allow Selectors Level 4 functional pseudo-classes :is() and :where() in Lumino selector validation by rejecting only top-level comma-separated selector lists, while permitting commas inside those pseudo-classes.
Changes:
- Add
Selector.hasTopLevelComma()to detect commas that separate selector lists (intended to ignore commas inside:is()/:where()). - Update selector validation in context menu items and command keybinding options to use
Selector.hasTopLevelComma()instead ofindexOf(','). - Add unit tests for
hasTopLevelComma().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/domutils/src/selector.ts | Introduces hasTopLevelComma() (via stripIsWhere) to allow commas inside :is()/:where(). |
| packages/domutils/tests/src/selector.spec.ts | Adds tests asserting commas inside :is()/:where() are allowed but top-level commas are rejected. |
| packages/widgets/src/contextmenu.ts | Switches context menu selector validation to reject only top-level commas. |
| packages/commands/src/index.ts | Switches keybinding selector validation to reject only top-level commas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Returns true if the selector contains a top-level comma. | ||
| */ | ||
| export function hasTopLevelComma(selector: string): boolean { | ||
| return stripIsWhere(selector).indexOf(',') !== -1; | ||
| } |
There was a problem hiding this comment.
hasTopLevelComma() now allows selectors like .a:is(.b,.c), but Selector.calculateSpecificity()/Private.calculateSingle() still split the selector at the first comma (selector.split(',', 1)[0]). That will cut commas inside :is()/:where() and produce incorrect specificity values, which affects ordering logic in context menus and keybinding resolution. Please update specificity parsing to ignore commas inside :is()/:where() (or implement their Selectors Level 4 specificity rules) so the new allowed selectors don’t break precedence decisions.
packages/domutils/src/selector.ts
Outdated
| return selector.replace( | ||
| /:(is|where)\(([^()]|\([^()]*\))*\)/g, | ||
| '' | ||
| ); |
There was a problem hiding this comment.
stripIsWhere() uses a regex that only handles limited parenthesis nesting. Valid selectors can nest functional pseudos (e.g. :is(:not(:is(.b,.c)))), and in those cases the regex won’t strip the outer :is(...), causing commas inside nested selectors to be treated as “top-level” and rejected. Consider replacing the regex with a small scanner that tracks parentheses depth and whether the current depth is inside an :is( / :where( function.
| return selector.replace( | |
| /:(is|where)\(([^()]|\([^()]*\))*\)/g, | |
| '' | |
| ); | |
| let result = ''; | |
| const len = selector.length; | |
| let i = 0; | |
| while (i < len) { | |
| const ch = selector[i]; | |
| if ( | |
| ch === ':' && | |
| (selector.startsWith('is(', i + 1) || | |
| selector.startsWith('where(', i + 1)) | |
| ) { | |
| // Determine which function name we matched. | |
| const isIs = selector.startsWith('is(', i + 1); | |
| const nameLen = isIs ? 2 : 5; // 'is' or 'where' | |
| const openParenIndex = i + 1 + nameLen; | |
| // Sanity check: ensure there is an opening parenthesis. | |
| if (openParenIndex >= len || selector[openParenIndex] !== '(') { | |
| // Fallback: treat ':' as a normal character. | |
| result += ch; | |
| i++; | |
| continue; | |
| } | |
| // Skip over the entire :is(...) or :where(...) construct, | |
| // correctly handling nested parentheses. | |
| let depth = 1; | |
| let j = openParenIndex + 1; | |
| while (j < len && depth > 0) { | |
| const cj = selector[j]; | |
| if (cj === '(') { | |
| depth++; | |
| } else if (cj === ')') { | |
| depth--; | |
| } | |
| j++; | |
| } | |
| // Set i to the position after the closing parenthesis (or end of string). | |
| i = j; | |
| continue; | |
| } | |
| // Normal character, keep it. | |
| result += ch; | |
| i++; | |
| } | |
| return result; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR Changes
This PR allows the use of
:is()and:where()functional pseudo-classesin Lumino selector validation.
Historically, selectors containing commas were rejected because
comma-separated selector lists do not have a single specificity.
However,
:is()and:where()are defined by Selectors Level 4as single selectors, with well-defined specificity rules.
This change:
:is()and:where()Fixes
:is()and:where()#685Labels
enhancement