[diffs/edit] fix marker popover text contrast - #1061
Closed
necolas wants to merge 1 commit into
Closed
Conversation
`contrast-color()` takes one argument. The marker popover passed two, which makes the declaration invalid at computed-value time. `color` then inherited the editor foreground, so the popover showed near-white text on the yellow warning fill. Chrome 151 measured 2.72:1 before the fix and 5.71:1 or better after it, across every severity and both themes. Playwright bundles an old Chromium without `contrast-color()`, so the existing e2e contrast test runs the fallback branch only. Add a static test that checks the argument count in the shipped CSS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mdo
reviewed
Aug 6, 2026
| ignores the `--diffs-marker-contrast` declaration above. */ | ||
| @supports (color: contrast-color(red)) { | ||
| color: contrast-color(var(--diffs-marker-bg), var(--diffs-bg)); | ||
| color: contrast-color(var(--diffs-marker-bg)); |
Contributor
There was a problem hiding this comment.
To keep the intent here, this was meant to be a fallback. Can probably update the code comment to remove mention of two colors.
Suggested change
| color: contrast-color(var(--diffs-marker-bg)); | |
| color: contrast-color(var(--diffs-marker-bg, var(--diffs-bg))); |
Contributor
Author
|
Closing in favor of #1062 |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
contrast-color()takes one argument. The marker popover ineditor.csspassed two:The arguments contain
var(), so the browser cannot check the grammar at parse time and accepts the declaration. After substitution the value is invalid, so the declaration becomes invalid at computed-value time.coloris an inherited property, so it then resolves to the inherited value. It does not fall back to the earliercolor: var(--diffs-marker-contrast)declaration, because the cascade already chose a winner.The popover therefore showed the near-white editor foreground on the severity fill.
The fix passes one argument.
contrast-color()still adapts to a theme-supplied fill (editorWarning.foregroundand the similar tokens), which the hard-coded per-severity fallback cannot do.Motivation & Context
A user reported poor contrast on the editor lint tooltip in the diffs docs app. The defect is in
@pierre/diffs, not in the docs app. The docs app usesworkspace:*, so it picks the fix up on the next build. Published consumers on1.3.4need a version bump.Measured in Chrome 151 against the real stylesheet, with the existing e2e contrast test:
rgb(250, 250, 250)onrgb(0, 159, 255)All four severities in both themes now clear WCAG AA.
contrast-color()picks white for the light-theme hint (dark grey fill) and black elsewhere.CI could not catch this. Playwright 1.51.1 bundles Chromium 134, which predates
contrast-color(), so the existing contrast test only ever measured the@supportsfallback branch — the branch that was already correct.cssContrastColorArity.test.tscloses that gap. It parses the shipped CSS with balanced-paren scanning and asserts one top-level argument percontrast-color()call. It runs in Bun with no browser, so it holds whatever browser CI uses. Confirmed red/green: it fails on the original CSS withcontrast-color(var(--diffs-marker-bg), var(--diffs-bg)) must take exactly one argument.Type of changes
first discussed with the dev team and they should be aware that this PR is
being opened
You must have first discussed with the dev team and they should be aware
that this PR is being opened
Checklist
contributing guidelines
moon run root:lint)moon run root:format)moonx diffs:test— 1504 pass, 0 fail)Also ran
moonx diffs:typecheck(clean) andmarkers.pw.tsin system Chrome 151 (5 passed).How was AI used in generating this PR
Claude Code (Opus 5) did the whole change: it traced the root cause, wrote the CSS fix and the regression test, and drafted this description. It verified the diagnosis in a real browser rather than by reasoning alone — it built a standalone harness in Chrome 151 to confirm that
CSS.supports('color', 'contrast-color(red, blue)')isfalsewhile the one-argument form istrue, then reproduced and fixed the failure end to end through the package's own e2e fixture.Related issues
None. Reported by a user against the diffs docs app.
Notes for the reviewer
Two open points I did not act on:
@pierre/diffsversion bump for published consumers. Your history keeps those as separatechore: bumpcommits, so I left it out.