Skip to content

[Bug]: Paint CodeEditor from the theme instead of CodeMirror's light default - #4025

Open
fashxp wants to merge 8 commits into
2026.2from
fix/code-editor-theme-aware
Open

[Bug]: Paint CodeEditor from the theme instead of CodeMirror's light default#4025
fashxp wants to merge 8 commits into
2026.2from
fix/code-editor-theme-aware

Conversation

@fashxp

@fashxp fashxp commented Aug 25, 2026

Copy link
Copy Markdown
Member

Why

CodeMirror defaults to its own light colour set, which paints an opaque white surface
while the text colour is inherited from the Studio theme. On a dark theme that leaves
every editor white on white — measured on pimcore-dark:

.cm-editor  background-color  rgb(255, 255, 255)
.cm-editor  color             rgba(255, 255, 255, 0.85)   <- invisible

What

  • Chrome comes from a CodeMirror theme built from the tokens, not a stylesheet.
    EditorView.baseTheme registers at Prec.lowest and mounts first, so a normal-precedence
    theme wins ties by construction instead of by hand-counted specificity.
  • Syntax colours take One Dark's highlight style only, never the whole oneDark bundle
    (which carries chrome that would win over the tokens). { dark } on the theme is what
    makes CodeMirror's own base themes pick their dark variants.
  • Darkness is derived from the surface colour (isDarkSurface(token.colorBgContainer)),
    not from a theme id, so a theme registered by a bundle works too.
  • Also fixes the three editors that bypass CodeEditorTextEditor (which backs the
    asset text editor and is SDK-public), the email-log text preview and the translation HTML
    preview. They keep rendering ReactCodeMirror directly because CodeEditor overwrites
    className, which carries their sizing.

Token choices and the reasoning behind them are in comments at the call sites.

Backward compatibility

code-editor.styles.tsx is untouched, so every rule that applied before still applies.
A caller passing its own theme keeps it and keeps control of the visuals — the token
theme is not appended on that path. One DOM detail: with no theme supplied the wrapper
class goes from cm-theme-light to cm-theme-none; nothing in this repo styles cm-theme*.

Verification

Driven through a running Studio in both themes, on the core robots.txt editor and the
asset text editor (no bundle involved). Contrast measured on the rendered DOM:

pimcore-dark studio-default-light
text on surface 12.25:1 16.56:1
selection band vs surface 1.60:1 (stock 1.26) 1.89:1 (stock 1.44)
text on selection 8.01:1 9.51:1

Search panel chrome confirmed token-driven. Dark syntax colours 5.6–9.9:1 on the surface.

Not driven end to end: the .cm-searchMatch fills — the panel opens and its chrome is
confirmed, but match state would not register from a synthetic input event.

Gates: tsc --noEmit clean · eslint clean · color.test.ts 7 passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CWjetJ3XSrzdSZpYFG3N57

CodeMirror defaults to its light colour set, which paints an opaque white surface
while the text colour is inherited from the theme, so on a dark theme the editor
was white on white. The chrome now comes from the tokens, and the dark syntax set
is selected when the surface is dark since CSS cannot reach those colours.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings August 25, 2026 16:42
@fashxp fashxp added this to the 2026.2.8 milestone Aug 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates CodeEditor to integrate CodeMirror with Studio theme colors and syntax highlighting.

Changes:

  • Derives CodeMirror syntax theme from surface luminance.
  • Applies Studio tokens to editor chrome.
  • Adds color parsing and luminance tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
assets/js/src/core/utils/color.ts Adds dark-surface detection.
assets/js/src/core/utils/color.test.ts Tests color parsing and classification.
assets/js/src/core/components/code-editor/code-editor.tsx Selects a theme from the surface color.
assets/js/src/core/components/code-editor/code-editor.styles.tsx Themes CodeMirror chrome with Studio tokens.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread assets/js/src/core/components/code-editor/code-editor.tsx Outdated
Comment thread assets/js/src/core/components/code-editor/code-editor.styles.tsx Outdated
Comment thread assets/js/src/core/utils/color.ts Outdated
fashxp and others added 4 commits August 25, 2026 16:47
Addresses the review on #4025. `theme="dark"` installed the whole oneDark bundle,
whose chrome won over the tokens for search matches, brackets, fold placeholders
and tooltips; the token rules also overrode a caller's own theme extension, and
a translucent surface colour was read as if it were opaque.

Co-Authored-By: Claude <noreply@anthropic.com>
A stylesheet has to out-specify selectors it does not own, and it lost: the
focused selection stayed at CodeMirror's #233, 1.26:1 on pimcore-dark. A theme
registers above the base themes, so ties go to it by construction.

The same hook now also themes the three editors that render CodeMirror directly,
including the asset text editor, which had the identical defect.

Co-Authored-By: Claude <noreply@anthropic.com>
@fashxp

fashxp commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Reviewed and acted on in 95a5331. Everything below was reproduced before changing anything.

The blocker was real and I reproduced it. On pimcore-dark, focused selection measured rgb(34,51,51) on rgb(30,30,36) — 1.26:1, invisible. My round-2 dark: true is what activated &dark.cm-focused > .cm-scroller > .cm-selectionLayer .cm-selectionBackground (five classes) against a four-class stylesheet rule. So round 2 fixed the chrome leak and introduced something worse.

Taken the structural recommendation. The chrome is now EditorView.theme(...) built from tokens, not a CSS class. code-editor.styles.tsx is back to untouched by this PR. Confirmed the reasoning: EditorView.baseTheme wraps in Prec.lowest (view:8679) and StyleModule.mount(root, styleModules.concat(baseTheme$1).reverse(), …) (:8227) mounts it first, so ties go to a normal-precedence theme. @codemirror/search's rules are EditorView.baseTheme too, so the same holds there. The focused-selection selector is still restated at matching depth — precedence settles ties, not specificity.

The two dead rules are gone. Confirmed .cm-content caret-color computes rgba(0,0,0,0) and .cm-line ::selection likewise, from hideNativeSelection's transparent !important. Dropped both, and the comment that claimed the caret rule was load-bearing. Also dropped .cm-lineNumbers, which measured 0.45 alpha — the emotion rule in code-editor.styles wins at equal depth, so a theme rule there is dead too.

The three siblings are covered. TextEditor, the email-log text preview and the translation HTML preview now take the same hook. They keep rendering ReactCodeMirror directly rather than moving to CodeEditor, because CodeEditor overwrites className and theirs carries their sizing. Verified E2E on a text/plain asset in the asset text editor: surface rgb(30,30,36), text rgba(255,255,255,.85), selection rgba(255,255,255,.15) — previously white on white.

Threshold moved to 0.179 (where contrast against white equals contrast against black) with a test that pins it, since the suite would have passed with anything in (0.0722, 0.7152).

Search-match salience: the two now use different hues rather than two steps of one ramp, so "a hit" and "the current hit" are distinguishable. Panel chrome is now token-driven and verified live — panel colorBgElevated, field colorBgContainer, buttons colorFillQuaternary, all with colorText — including the .cm-button / .cm-textfield greys you flagged.

On the selection colour, your framing led somewhere useful. A band has to move away from the surface, and the direction differs per theme, so no single token works: colorPrimary is itself dark (1.1:1 on pimcore-dark) and on light drops text to 2.39:1. Measured result — colorFill on dark, colorPrimaryBorder on light:

dark light
band vs surface 1.60:1 (stock 1.26) 1.89:1 (stock 1.44)
text on band 8.01:1 9.51:1

Two points I'm not acting on, with reasons. The cm-theme-lightcm-theme-none class change is now named in the BC section; nothing in the repo styles cm-theme*. And @codemirror/language stays undeclared, matching helpers.ts and detect-language.tsx in the same directory — worth fixing, but as its own change rather than smuggled into a bugfix.

Corrections to the review. Two things it got slightly wrong: the PR body said 4 tests where jest reported 5 (stale, now 6), and the dark-syntax figures were indeed measured through the agent-bundle wrapper on One Dark's own #282c34, not colorBgContainer — the numbers in the body are now measured on this code path. Both fair hits.

Still not driven end to end: the .cm-searchMatch fills themselves. The panel opens and its chrome is confirmed, but I could not get match state to register from a synthetic input event, so those two rules rest on selector depth. Flagged in the PR body rather than claimed.

fashxp and others added 3 commits August 25, 2026 19:30
One expression describing every separator the syntax allows reached a complexity
of 24. Matching the wrapper and splitting the inside is simpler and covers the
same forms, but accepts non-numeric channels, so those are now rejected rather
than read as black — and therefore as dark.

Co-Authored-By: Claude <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@fashxp fashxp removed this from the 2026.2.8 milestone Aug 26, 2026
@fashxp
fashxp requested a review from vin0401 August 26, 2026 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants