fix: make topic colors legible across dark, night, and light themes#2050
fix: make topic colors legible across dark, night, and light themes#2050deviationist wants to merge 3 commits into
Conversation
User-chosen topic colors with extreme lightness (e.g. near-black) are unreadable when rendered against the opposite-tone theme background. The color is applied raw to the topic text, the color-line, and the pill tint in SubscriptionsList, with no luminance check. Introduce a `readableColor(hex, theme)` helper that parses the hex, converts to HSL, and clamps the lightness into a theme-appropriate band (L >= 0.6 for dark/night, L <= 0.55 for light) while preserving hue and saturation. The transform is applied at render time only; the stored `sub.color` is never mutated, so editing round-trips cleanly and theme switches re-derive a legible variant on every render. Applied symmetrically to both the web and desktop SubscriptionsList components.
Extends the existing tests/unit/utils/colors.spec.ts with 11 cases
for the new readableColor helper:
- dark/night themes lighten low-lightness inputs to L >= 0.6 and
preserve hue
- already-readable inputs (e.g. the predefined palette cyan) pass
through unchanged
- light theme darkens near-white inputs to L <= 0.55, leaves dark
inputs alone
- empty/non-hex/3-char shorthand inputs are handled gracefully
All cases pass under the existing mocha runner.
|
Hi @ysfscream — sorry to bother you directly, just wanted to flag this one in case you have a moment. This PR is a small dark/night-mode legibility fix for the subscriptions list: user-chosen topic colors with extreme lightness become unreadable against the opposite-tone theme background. I've added a hue-preserving If you'd rather I split the web and desktop changes into two PRs, change the contrast strategy (e.g. iterative WCAG ≥ 3:1 instead of HSL lightness clamping), or adjust anything else, I'm happy to rework it. No urgency — review whenever it fits your schedule. Thanks for maintaining MQTTX! |
There was a problem hiding this comment.
Pull request overview
This PR improves subscription topic color legibility across Light/Dark/Night themes by deriving a theme-appropriate, readable variant of the user-selected topic color at render time (without mutating stored subscription data).
Changes:
- Added a
readableColor(hex, theme)utility (duplicated in bothsrc/andweb/src/) to clamp HSL lightness per theme. - Updated both desktop and web
SubscriptionsList.vueto usereadableColorfor topic text, sidebar color-line, and background tint. - Added unit tests for
readableColorintests/unit/utils/colors.spec.ts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/colors.ts |
Adds readableColor plus hex/RGB/HSL conversion helpers for desktop app rendering. |
web/src/utils/colors.ts |
Mirrors the same readableColor helper for the web app. |
src/components/SubscriptionsList.vue |
Applies readableColor to subscription item styling (text/line/tint) in desktop UI. |
web/src/components/SubscriptionsList.vue |
Applies readableColor to subscription item styling (text/line/tint) in web UI. |
tests/unit/utils/colors.spec.ts |
Adds unit coverage for the new color readability behavior and input handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const readableColor = (hex: string, theme: Theme): string => { | ||
| if (!hex) return hex | ||
| const rgb = hexToRgb(hex) | ||
| if (!rgb) return hex | ||
| const [h, s, l] = rgbToHsl(rgb) | ||
| const minL = theme === 'light' ? 0 : 0.6 | ||
| const maxL = theme === 'light' ? 0.55 : 1 | ||
| const newL = Math.max(minL, Math.min(maxL, l)) | ||
| if (newL === l) return hex | ||
| const [r, g, b] = hslToRgb([h, s, newL]) | ||
| return rgbToHex(r, g, b) |
There was a problem hiding this comment.
Good catch — thanks. Fixed in 124eee2: when the early-return path is hit, the helper now normalizes 3-char shorthand to 6-char so the ${color}1A alpha-suffix idiom always yields valid CSS. 6-char input still passes through unchanged to preserve casing.
| export const readableColor = (hex: string, theme: Theme): string => { | ||
| if (!hex) return hex | ||
| const rgb = hexToRgb(hex) | ||
| if (!rgb) return hex | ||
| const [h, s, l] = rgbToHsl(rgb) | ||
| const minL = theme === 'light' ? 0 : 0.6 | ||
| const maxL = theme === 'light' ? 0.55 : 1 | ||
| const newL = Math.max(minL, Math.min(maxL, l)) | ||
| if (newL === l) return hex | ||
| const [r, g, b] = hslToRgb([h, s, newL]) | ||
| return rgbToHex(r, g, b) |
There was a problem hiding this comment.
Same fix applied to the web copy in 124eee2 — symmetric with the desktop change.
| it('accepts 3-character shorthand hex', () => { | ||
| // #003 expands to #000033 — very dark, should be lightened on dark theme | ||
| const out = readableColor('#003', 'dark' as Theme) | ||
| expect(lightness(out)).to.be.at.least(0.6 - 1e-6) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Added a regression test in 124eee2 — normalizes 3-char shorthand to 6-char even when no clamp is needed — exercising the light-theme short-circuit path where the bug would have triggered. Full suite is 331 passing.
… valid readableColor previously short-circuited and returned the raw input when no lightness clamp was needed. For 3-character shorthand input (e.g. "emqx#3") this leaked back to the call sites, which append an alpha hex suffix (e.g. `${readableColor(...)}1A`) and would have produced invalid CSS like "#0031A". Normalize the early-return path to emit 6-character hex when the input is shorthand, while still passing 6-character input through unchanged to preserve the user's chosen casing. Add a regression test covering the light-theme short-circuit case. Flagged by GitHub Copilot review on PR emqx#2050. The actual call sites (el-color-picker, getRandomColor, defineColors) all emit 6-char hex today, so the bug was latent — this hardens the helper's contract so future callers can't trip it.
|
Hi again @ysfscream — quick update: GitHub Copilot's code review flagged a latent edge case where The PR is ready for your review whenever it fits your schedule. Thanks! |
|
@deviationist Thanks for working on this PR. This is actually an optimization we have wanted to improve for a while, especially around subscription colors in different themes. I’m wondering if we should handle this from a slightly different angle. The core idea I have is: we should prevent the app from generating unreadable colors by default, but we should not silently change or compensate for colors that users explicitly choose themselves. For example, if the random/default color generation produces a very dark color in dark/night theme, that is something the app should avoid. In that case, using a safe palette or limiting the random color range makes sense. But if a user manually selects a very dark or very light color, I think we should preserve that choice exactly. Otherwise the app is effectively changing the user’s selected color during rendering, and the color shown in the subscription list may not match what the user picked in the color picker. So my suggestion would be:
What do you think about adjusting the PR in this direction? |
|
Thanks for the thoughtful response — the WYSIWYG principle is right and I want to align with it. Proposed direction, if it matches your thinking: User's pick is preserved exactly, in every theme:
The actual readability bug is narrower than my current PR addresses. The topic text currently inherits Why compute against the blend rather than Implementation note: the tint alpha ( Two follow-up PRs rather than bundling them in:
This keeps each piece reviewable on its own. If the direction works for you, I'll rework this branch: drop the |
|
Thanks, this direction sounds good to me. Let’s keep this PR focused on the text readability issue: preserve The safe random/default palette and picker hint can be handled in follow-up PRs. |
PR Checklist
If you have any questions, you can refer to the Contributing Guide
What is the current behavior?
In the Subscriptions list, the topic text, the colored side-bar, and the pill background tint are all painted with the user-chosen
sub.colorverbatim. When the user picks a dark color (e.g. dark indigo) and is on the Dark or Night theme, the text is rendered nearly invisible against the dark page background. The same problem exists in reverse — a near-white color picked on the Light theme is unreadable against the white background.Reproduction (web):
#0F003A).Before:
The bug exists symmetrically in
src/components/SubscriptionsList.vue(desktop) andweb/src/components/SubscriptionsList.vue(web).Issue Number
N/A
What is the new behavior?
Introduces a
readableColor(hex, theme)helper insrc/utils/colors.tsandweb/src/utils/colors.tsthat:L >= 0.6(dark colors get lightened).L <= 0.55(near-white colors get darkened).The helper is applied at render time only to the topic text, the color-line, and the pill tint. The stored
sub.coloris never mutated, so:SubscriptionsList.vueline ~642 in both apps).After the fix on the same
#0F003Atopic in Night mode, contrast goes from ~1.05:1 to ~2.62:1 and the topic name is clearly readable.After:
Does this PR introduce a breaking change?
Specific Instructions
yarn electron:serve— the desktop change is a 1:1 mirror of the web change (same template, same imports, same class field exposing the helper to the template).readableColorintests/unit/utils/colors.spec.tscovering all three themes, hue preservation, the predefined palette pass-through, and edge cases (empty / non-hex / 3-char shorthand). Fullyarn test:unitsuite passes — 330 tests green.web/Cypress e2e suites are unmodifiedvue-clitemplates with no real coverage, so they were not run.Other information
The lightness clamp was chosen over a strict WCAG-iterative approach because it produces more predictable, hue-preserving output for the typical topic-color palette. If maintainers prefer a true WCAG ≥ 3:1 guarantee, the helper can be extended to iterate against a per-theme background luminance — happy to take direction.
On code duplication: the new
readableColorhelper and its hex/HSL conversion functions are duplicated verbatim betweensrc/utils/colors.tsandweb/src/utils/colors.ts. This mirrors the pre-existing pattern —defineColorsandgetRandomColorwere already duplicated across the two trees before this PR, and the repo has no shared package or workspace setup. Consolidating into a shared module would require new tsconfig path mappings, build/test wiring, and is a structural change well beyond a UI bug fix, so I've kept the duplication consistent with the existing convention. Happy to open a follow-up PR proposing ashared/package if maintainers think it's worth it.