feat: adapt low-contrast inline colors to surface#4069
Conversation
📝 WalkthroughWalkthroughAdds a WCAG-based adaptColorContrast utility, integrates it into limel-markdown (new reflected prop and post-render invocation) and limel-email-viewer (post-render ref call), plus example, unit, and e2e tests. ChangesColor Contrast Adaptation Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4069/ |
FredrikWallstrom
force-pushed
the
feat/contrast-aware-rendering
branch
from
99bdc60 to
645cdac
Compare
| /** | ||
| * Adapt rendered inline `color:` declarations to the surrounding | ||
| * surface. After each render the component walks the rendered DOM and | ||
| * removes any inline `color` whose contrast against the resolved | ||
| * background falls below WCAG 3:1, letting the surface's themed text | ||
| * color inherit through. Brand colors that already meet contrast are | ||
| * left alone. | ||
| * | ||
| * Default `false` so the component remains a neutral renderer; turn | ||
| * this on for surfaces that render externally-authored content | ||
| * (e.g. imported email bodies) where the host application's theme | ||
| * drives the surrounding text color. | ||
| */ | ||
| @Prop({ reflect: true }) | ||
| public adaptColorContrast = false; |
There was a problem hiding this comment.
Doing cleanup after every render sounds rather inefficient to me. Why can't it be done as pre-processing by the component providing the bad input in the first place? 🙂
There was a problem hiding this comment.
Believe me, me and @Kiarokh has been discussing this back and forth 😄
See https://github.com/Lundalogik/lime-crm-building-blocks/pull/1423, and #4066
Pre-processing the "real field value" doesn't feel right too me since we will mutate users content which will then be lost on round-trips. I rather see a visual solution.
We would love to hear your thoughts how to solve this.
There was a problem hiding this comment.
OK. I'm going to dig into your arguments, and see if there are really no better solutions. Please give me over the weekend before you merge this. This component unfortunately has a history of being the go to place for people to try to cram patches to fix their broken email HTML, and I really want that to stop. But if this is truly the best solution in this particular case, I will approve it.
There was a problem hiding this comment.
OK, I've looked at you earlier discussion. It seems like this is a fairly reasonable trade-off.
Claude helped me summarise your conversations, and traced back the first "email band-aid" to be my commit 43e89bf (feat(markdown): support certain whitelisted inline css styles, fix for Lundalogik/limepkg-email#590). It told me the fact that you put the email-shaped feature in there in the first place is worth naming. Otherwise the precedent argument lands as "stop doing what I started." 😄
But yes, "stop doing what I started" is exactly what I'm saying 😆
So, consider this approved on a conceptual level. I'm not setting an approved-stamp, since I have neither reviewed nor tested the code.
Thank you for holding off on merging this until I had a chance to look into it a bit 🙏
FredrikWallstrom
force-pushed
the
feat/contrast-aware-rendering
branch
2 times, most recently
from
5aa3831 to
74defa2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/email-viewer/email-viewer.tsx`:
- Around line 94-100: componentDidRender currently calls
adaptColorContrast(this.bodyElement) even if this.bodyElement may be detached;
update componentDidRender (and optionally where renderBodyHtml sets the ref) to
guard by checking that this.bodyElement exists and is connected
(this.bodyElement && this.bodyElement.isConnected) before calling
adaptColorContrast, and ensure renderBodyHtml's ref clears this.bodyElement when
the body div is not rendered so the reference cannot become stale.
In `@src/components/markdown/markdown.tsx`:
- Around line 96-110: The adaptColorContrast prop is not watched so toggling it
doesn't re-run the DOM processing; add a watcher for adaptColorContrast (similar
to the existing `@Watch` handlers for value, whitelist, removeEmptyParagraphs)
that calls the same DOM reprocessing used by textChanged() — e.g., invoke
adaptColorContrast(this.rootElement) (or reuse textChanged() / the
morphChildren-based re-render path) so true→false restores original inline
colors and false→true strips low-contrast colors; ensure the watcher mirrors
behavior used for whitelist/removeEmptyParagraphs and uses morphChildren to
re-inject original markup when needed.
- Around line 96-108: Update the JSDoc for the adaptColorContrast prop/method to
clarify it runs only after the component's textChanged() update cycle (i.e.,
after value/whitelist/removeEmptyParagraphs and the adaptColorContrast
invocation), not on arbitrary host re-paints or external theme changes; locate
the existing comment block above adaptColorContrast (and references to
textChanged()) and replace the sentence "After each render the component walks
the rendered DOM…" with a one-line note explicitly stating it runs only after
textChanged() cycles so consumers don't expect it to respond to host theme
repaints.
In `@src/util/adapt-color-contrast.ts`:
- Line 2: STRIPPED_COLOR_ATTR is declared but never read, so either remove the
constant and the code that sets this attribute on stripped elements to avoid DOM
bloat, or keep it and add a short comment above STRIPPED_COLOR_ATTR explaining
its intended purpose (e.g., for QA/debugging or future restore path) and why it
is left intentionally write-only; update the code that assigns the attribute to
reference the comment and ensure no other code relies on it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4609123-5c7b-4eb9-89a8-b38ff463f917
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (6)
src/components/email-viewer/email-viewer.tsxsrc/components/markdown/examples/markdown-adapt-color-contrast.tsxsrc/components/markdown/markdown.tsxsrc/util/adapt-color-contrast.e2e.tssrc/util/adapt-color-contrast.spec.tssrc/util/adapt-color-contrast.ts
FredrikWallstrom
force-pushed
the
feat/contrast-aware-rendering
branch
from
915746d to
812fb8f
Compare
|
|
||
| public componentDidRender() { | ||
| if (this.bodyElement?.isConnected) { | ||
| adaptColorContrast(this.bodyElement); |
There was a problem hiding this comment.
Does this have to run on every re-render, or could we check so that it only runs for example when innerHTML is updated? (Or maybe it's never re-rendered apart from when the body changes? 🤔)
There was a problem hiding this comment.
We could skip to run this on every re-render by memoizing the produced innerHtml string. However, that means holding a full extra copy of the body HTML in memory, not just a pointer to the input.
Which approach do we think is best? Holding a copy of the innerHTML in-memory, or run adaption on each re-render?
(The reason we'd need to memoize is that applyRemoteImagesPolicy returns a fresh string on every call when remote images are on, so Stencil keeps re-setting innerHTML between renders, which re-parses the HTML and silently wipes our adaptation.)
There was a problem hiding this comment.
Alright, let's keep it as it is :)
FredrikWallstrom
force-pushed
the
feat/contrast-aware-rendering
branch
from
812fb8f to
faae813
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/util/adapt-color-contrast.ts`:
- Line 156: The alpha parsing in parseColor currently does const a = match[4]
=== undefined ? 1 : Number.parseFloat(match[4]) which can yield NaN or invalid
values; change it to explicitly parse and validate the alpha: if match[4] is
present, parse with Number.parseFloat into a variable, check
Number.isFinite(parsedA), then clamp it to the 0–1 range (e.g. Math.min(1,
Math.max(0, parsedA))); if it's not finite or invalid, fall back to a sensible
default (e.g. 1) rather than using NaN. This ensures downstream functions
compositeOver and contrastRatio receive a valid alpha.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2153e34f-25f4-48f8-9666-16fe2ab553c1
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (6)
src/components/email-viewer/email-viewer.tsxsrc/components/markdown/examples/markdown-adapt-color-contrast.tsxsrc/components/markdown/markdown.tsxsrc/util/adapt-color-contrast.e2e.tssrc/util/adapt-color-contrast.spec.tssrc/util/adapt-color-contrast.ts
| if (!match) { | ||
| return null; | ||
| } | ||
| const a = match[4] === undefined ? 1 : Number.parseFloat(match[4]); |
There was a problem hiding this comment.
Validate parsed alpha to prevent NaN or invalid values.
The regex [\d.]+ for the alpha channel can match invalid strings such as "." (yields NaN) or "1.2.3" (parseFloat stops at the second dot, yielding 1.2). Since parseColor is exported and may be called by external consumers, invalid alpha values can propagate through compositeOver and contrastRatio, causing incorrect contrast decisions.
🛡️ Proposed fix
const a = match[4] === undefined ? 1 : Number.parseFloat(match[4]);
+ if (Number.isNaN(a)) {
+ return null;
+ }
return {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const a = match[4] === undefined ? 1 : Number.parseFloat(match[4]); | |
| const a = match[4] === undefined ? 1 : Number.parseFloat(match[4]); | |
| if (Number.isNaN(a)) { | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/adapt-color-contrast.ts` at line 156, The alpha parsing in
parseColor currently does const a = match[4] === undefined ? 1 :
Number.parseFloat(match[4]) which can yield NaN or invalid values; change it to
explicitly parse and validate the alpha: if match[4] is present, parse with
Number.parseFloat into a variable, check Number.isFinite(parsedA), then clamp it
to the 0–1 range (e.g. Math.min(1, Math.max(0, parsedA))); if it's not finite or
invalid, fall back to a sensible default (e.g. 1) rather than using NaN. This
ensures downstream functions compositeOver and contrastRatio receive a valid
alpha.
|
🎉 This PR is included in version 39.24.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Summary by CodeRabbit
New Features
Documentation
Tests
Fixes Lundalogik/crm-client#621
Fixes Lundalogik/crm-client#966
When externally-authored HTML lands inside CRM (imported email bodies, in particular), the sender's inline
color:declarations can collide with the host theme — black-on-dark, white-on-white, etc. — leaving content unreadable.This adds a small
adaptColorContrastutility that walks a rendered subtree, computes WCAG contrast for each element with an inlinecolor, and removes declarations below a 3:1 ratio so the surface's themed text color inherits through. Brand colors that already meet contrast pass through untouched.Three atomic commits:
feat(util): add adaptColorContrast utility— pure DOM walk + WCAG luminance / contrast / sRGB compositing, plus spec and e2e tests. Lives insrc/util/since it's not specific to any one component.feat(markdown): add opt-in adaptColorContrast prop— newadaptColorContrastboolean prop on<limel-markdown>, defaultfalse. Off by default keeps the component a neutral renderer; consumers that mix authored content with themed surfaces (the activity feed) opt in.feat(email-viewer): adapt color contrast in rendered body— runs unconditionally after each render in<limel-email-viewer>, since it always renders externally-authored content.Replaces the closed PR #4060 (which had the same logic but ran unconditionally on every
<limel-markdown>render) and supersedes the limbb wrapper approach inLundalogik/lime-crm-building-blocks#1423(closed). Direction agreed offline with @Kiarokh — the algorithm lives in lime-elements, opt-in for<limel-markdown>, default-on for<limel-email-viewer>.Review:
Test plan:
.emlwhose body has inlinecolor: blacktext in CRM dark mode via the file viewer. Verify the text is readable (inherits the surface's themed color) rather than rendering black-on-dark..emlwhose body has inlinecolor: whitetext in CRM light mode. Verify the text falls back to a readable color rather than being invisible..emlwhose body uses brand colors (red, blue, green) on a normal CRM surface in both light and dark mode. Brand colors should still be visible.<limel-markdown>withadapt-color-contrastset, confirm the same behaviour for inline-colored markdown content.<limel-markdown>withoutadapt-color-contrast, confirm rendering is unchanged from main.Browsers tested:
Windows:
Linux:
macOS:
Mobile: