feat: adapt low-contrast inline colors to surface#4069
feat: adapt low-contrast inline colors to surface#4069FredrikWallstrom wants to merge 3 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces a WCAG-based color contrast adaptation utility that automatically strips or adjusts inline text colors failing a 3:1 contrast threshold. The utility is integrated into limel-markdown and limel-email-viewer rendering pipelines, with comprehensive unit tests, e2e tests, and a demonstration example component. ChangesColor Contrast Adaptation Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 unit tests (beta)
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.
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
2 participants
Summary by CodeRabbit
New Features
limel-markdownnow supports an optionaladaptColorContrastprop that automatically adjusts text colors to meet WCAG contrast requirements for improved readability.limel-email-viewernow applies color contrast adaptation during rendering.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: