Uh oh!
There was an error while loading. Please reload this page.
fix(design-system): report radius custom properties under their real name - #1537
Merged
Conversation
…name The CSS scanner matched the property name as a bare substring, so `--os-track-border-radius: …` in base.css was scanned as though it were a plain `border-radius` declaration. Both such properties currently hold whitelisted tokens, so nothing failed — but the moment one held a raw px the offender would have read renderer CSS: border-radius: 4px; naming a declaration that does not exist anywhere in the stylesheet, and sending whoever has to fix it looking for the wrong thing. Keeping these in scope is deliberate, not merely tolerated. `.os-theme-maka` is OverlayScrollbars' theming API: the library's own CSS consumes those properties and renders a real scrollbar corner. A radius that reaches the screen is governed no matter which declaration spells it. So the fix is to capture the whole property name rather than to anchor the match and drop the coverage. The lookbehind is load-bearing: without it a prefixed property also matches at its inner `border-radius` and is reported twice. `\b` cannot do this job — `-` is a non-word character, so a word boundary already exists between `-` and `border`. Found while reviewing #1520; deliberately left out of that PR and #1535 as pre-existing and orthogonal. Verified: lint, format:check, typecheck, check-dead-css, and 2891 main-process tests. The new assertion fails against the old regex.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the last item from the #1520 review — the scanner's substring match on the property name.
The defect
RADIUS_DECL_REmatchedborder-radiusas a bare substring, so this inbase.css:was scanned as if it were a plain
border-radiusdeclaration. Both hold whitelisted tokens today, so nothing failed. But had either held a raw px, the offender would have read:— naming a declaration that exists nowhere in the stylesheet.
Why the fix is not to exclude custom properties
My first instinct was to anchor the match so only real declarations qualify. Looking at what these actually are changed that:
.os-theme-makais OverlayScrollbars' theming API — the library's own CSS consumes those properties and renders a real scrollbar corner.A radius that reaches the screen should be governed regardless of which declaration spells it. Anchoring would have dropped live coverage to fix a cosmetic bug. So the fix captures the whole property name instead, turning accidental coverage into intentional coverage:
--os-track-border-radius: 4px;border-radius: 4px;❌--os-track-border-radius: 4px;✅border-radius: 10px;The lookbehind is load-bearing
Without
(?<![-\w]), a prefixed property also matches at its innerborder-radiusand gets reported twice. A\bcannot do this job:-is a non-word character, so a word boundary already exists between-andborder— the naive anchor is a no-op here.Tests
One case added, covering all four halves: the custom property is reported, it is named truthfully, the real
base.cssspelling still passes, longhand custom properties are covered, and one declaration yields exactly one match. Verified the naming assertion fails against the old regex.Gate
lint · format:check · typecheck · check-dead-css — clean, plus 2891 main-process tests passing.