feat: support high contrast mode via C toggle key

[ellvix]

Pull Request

Description

Adds basic high contrast functionality, including multiple level support in settings (defaults to 2 levels (black and white)).

Related Issues

Addresses #361

Changes Made

Created a toggle function triggered on 'c'. All colored objects in the standard svg structure are analyzed and converted to their closest grayscale color according to the number of specified contrast levels. EG, for black and white, convert everything to either black or white.

A few exceptions: Many charts use a color very close to white, so we define a custom "is it actually white, or just close" so the chart items and backgrounds don't end up being the same color in high contrast mode.

Additional Notes

Still a minor issue with some heatmap charts, where the numeric text on the cell can be the same as the background.

Still todo:

• Convert to the full available color range, rather than just the closest color.
• Move from highlightService to displayService.

[Copilot]

Pull Request Overview

This PR adds high contrast mode functionality to the MAIDR system, triggered by the 'c' key toggle. The feature analyzes SVG elements and converts their colors to grayscale levels (defaulting to black and white) based on configurable contrast settings.

• Adds a new "High Contrast Levels" setting to control the number of contrast levels (defaults to 2 for black/white)
• Implements color-to-grayscale conversion with special handling for near-white colors to prevent background/foreground conflicts
• Integrates the toggle functionality into the existing keybinding system across all modes

Reviewed Changes

Copilot reviewed 13 out of 16 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/ui/pages/Settings.tsx	Adds High Contrast Levels numeric input field to settings UI
src/type/settings.ts	Extends GeneralSettings interface with highContrastLevels property
src/state/viewModel/settingsViewModel.ts	Sets default value of 2 for highContrastLevels in initial state
src/service/settings.ts	Sets default value of 2 for highContrastLevels in settings service
src/service/keybinding.ts	Adds 'c' key mapping for TOGGLE_HIGH_CONTRAST across all modes
src/service/highlight.ts	Implements core high contrast functionality with color conversion logic
src/service/display.ts	Minor formatting changes to constructor parameters
src/model/context.ts	Changes instructionContext from private to public for high contrast access
src/controller.ts	Formatting improvements and adds settingsService to command context
src/command/toggle.ts	Adds ToggleHighContrast command class
src/command/factory.ts	Integrates ToggleHighContrast into command factory
src/command/command.ts	Adds settingsService to CommandContext interface
examples/lineplot.html	Adds CSS rules for high contrast mode styling

Comments suppressed due to low confidence (1)

src/service/highlight.ts:120

• This method contains DOM manipulation logic that should be handled by the UI layer. According to the architectural guidelines, the Service layer should emit events for ViewModels to consume rather than directly manipulating the DOM.

      svg.removeAttribute('style');

[Copilot]

The numeric input for High Contrast Levels lacks validation constraints. Consider adding min/max values (e.g., min=2, max=255) to prevent invalid configurations.

[github-actions]

⚠️ Changes detected in examples or UI directory

This PR contains changes to files in the examples/ or src/ui/ directories. Before merging, please ensure:

1. Run the e2e tests locally:

   npm run e2e

2. Verify that all examples still work as expected

3. Update any relevant documentation if needed

If you need help running the tests, please refer to our testing documentation.

[github-actions]

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

1. Run the unit tests locally:

   npm run test

2. Update tests if necessary

[ellvix]

Issues still pending:

• Announcement "High contrast mode enabled." (or disabled) needed
• Multi layer / figure / etc charts break and don't show some of the items. Cause is that the items are nearly white and get grouped with the background and assigned the same (black) color. This is handled for similar cases like bar, and just needs the same treatment for these multi chart types.
• Stacked, candle, and other charts that have multiple colors for different types currently use the same high contrast color for all (white). Recommended solution: get access to selectors and use them to determine what should be shown, and then spread their colors across the full grayscale spectrum (depending on number of levels user has available).

[github-actions]

⚠️ Changes detected in examples or UI directory

This PR contains changes to files in the examples/ or src/ui/ directories. Before merging, please ensure:

1. Run the e2e tests locally:

   npm run e2e

2. Verify that all examples still work as expected

3. Update any relevant documentation if needed

If you need help running the tests, please refer to our testing documentation.

[github-actions]

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

1. Run the unit tests locally:

   npm run test

2. Update tests if necessary

[nk1408]

@jooyoungseo @ellvix , High contrast mode toggle works fine outside the L scope. However, I see this error on console when I toggle high contrast mode

factory.ts:267 Uncaught Error: Invalid command name: MOVE_TO_INDEX
create	@	factory.ts:267
(anonymous)	@	keybinding.ts:281

[github-actions]

⚠️ Changes detected in examples or UI directory

This PR contains changes to files in the examples/ or src/ui/ directories. Before merging, please ensure:

1. Run the e2e tests locally:

   npm run e2e

2. Verify that all examples still work as expected

3. Update any relevant documentation if needed

If you need help running the tests, please refer to our testing documentation.

[github-actions]

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

1. Run the unit tests locally:

   npm run test

2. Update tests if necessary

[claude]

Pull Request Review: High Contrast Mode Feature

Overview

This PR adds a comprehensive high contrast accessibility feature with keyboard toggle (C key), configurable contrast levels, and pattern support for differentiation. The implementation is substantial and well-architected overall.

Code Quality & Architecture

✅ Strengths

1. Well-structured service pattern: The HighContrastService and PatternService follow good separation of concerns
2. Lifecycle management: Proper initialization/suspension logic via initializeHighContrast() and suspendHighContrast() handles focus/blur states correctly
3. Comprehensive documentation: Excellent inline comments explaining complex logic (e.g., constructor behavior, DOM state management)
4. Settings integration: Properly uses the observer pattern for reactive settings changes
5. Color manipulation: Sophisticated color interpolation and luminance-based algorithms

⚠️ Issues & Concerns

1. Missing Test Coverage (High Priority)

• Issue: No tests found for the new 981-line highContrast.ts or 482-line pattern.ts
• Risk: Complex color algorithms, DOM manipulation, and state management are untested
• Recommendation: Add unit tests for:
  • Color interpolation functions (interpolateColors, parseColorToRgb, rgbToHex)
  • Color distance calculations (findClosestColor, spreadColorsAcrossLuminanceSpectrum)
  • Pattern generation and application
  • State management (capture/restore cycles)
  • Edge cases (invalid colors, missing elements, etc.)

2. Performance Concerns (Medium Priority)

highContrast.ts:368-376 - Inefficient element capture:

private getOriginalColorInfo(): ElementColorInfo[] | null {
  const svg = this.displayService.plot;
  if (!svg) return null;
  const svgElements = svg.querySelectorAll('*'); // Captures ALL elements
  
  for (let i = 0; i < svgElements.length; i++) { // Could be thousands

• Issue: querySelectorAll('*') captures every SVG element, including hidden ones, then iterates with inefficient conditions
• Impact: On complex charts with many elements, this could cause noticeable lag
• Recommendation:
  • Use more specific selectors (e.g., [fill], [stroke])
  • Consider querySelectorAll('[fill]:not([visibility="hidden"]), [stroke]:not([visibility="hidden"])')
  • Filter earlier rather than checking visibility === 'hidden' for each element

highContrast.ts:560-565 - Repeated canvas context creation:

private toColorStep(colorInfo: ElementColorInfo): string {
  const ctx = document.createElement('canvas').getContext('2d'); // Created for EACH color

• Issue: Creates a new canvas context for every color conversion (called once per element)
• Recommendation: Create a single canvas context and reuse it:

private canvasContext: CanvasRenderingContext2D | null = null;

private getCanvasContext(): CanvasRenderingContext2D | null {
  if (!this.canvasContext) {
    this.canvasContext = document.createElement('canvas').getContext('2d');
  }
  return this.canvasContext;
}

3. Potential Memory Leaks (Medium Priority)

highContrast.ts:50, 338 - Cached data not cleared on dispose:

private originalColorInfo: ElementColorInfo[] | null = null;
private traceElementsCache: Set<SVGElement> | null = null;

• Issue: dispose() doesn't explicitly clear these caches, potentially holding references to DOM elements
• Recommendation: Add to dispose():

this.originalColorInfo = null;
this.traceElementsCache = null;

4. Code Duplication (Low Priority)

Multiple instances of canvas-based color normalization:

• normalizeColor() (line 764)
• parseColorToRgb() (line 732)
• Inline canvas usage in toColorStep() (line 560)

Recommendation: Consolidate into a single reusable utility.

5. Magic Numbers (Low Priority)

highContrast.ts:392-394:

if (complexPath) {
  isComplexPath = complexPath.length > 120; // Why 120?
}

• Issue: Unexplained threshold
• Recommendation: Extract to named constant with explanation:

private static readonly COMPLEX_PATH_THRESHOLD = 120; // Paths longer than this are likely foreground elements, not backgrounds

highContrast.ts:583, 655:

const nearWhiteScale = 0.1; // Why 0.1?
const nearWhiteThreshold = 255 * (1 - nearWhiteScale);

• Recommendation: Extract and document:

private static readonly NEAR_WHITE_SCALE = 0.1; // 10% tolerance for "near white" detection

6. Error Handling (Medium Priority)

Missing null checks:

• pattern.ts:238-245 - normalizeColor() returns original color if canvas context fails, but this could propagate invalid colors
• highContrast.ts:736-743 - parseColorToRgb() returns null on failure, but callers don't always check

Recommendation: Add defensive checks or throw descriptive errors for invalid color inputs.

Potential Bugs

1. Race Condition Risk (Medium)

controller.ts:270-277:

public initializeHighContrast(): void {
  this.highContrastService.initializeHighContrast();
}

Called immediately after Controller construction, but high contrast service subscribes to settings changes in its constructor. If settings change during initialization, there could be a race condition.

Recommendation: Ensure the service is fully initialized before applying high contrast.

2. DOM Mutation After Capture (High)

highContrast.ts:116, 152-159:

this.captureOriginalColors(); // Captures in constructor

private captureOriginalColors(): void {
  const bodyStyle = window.getComputedStyle(document.body);
  this.defaultBackgroundColor = bodyStyle.backgroundColor;
  this.defaultForegroundColor = bodyStyle.color;
  this.originalColorInfo = this.getOriginalColorInfo();
}

Issue: If other scripts or interactions modify SVG colors after capture but before high contrast is applied, the "original" colors will be wrong.

Mitigation: The PR description notes this is handled carefully (first Controller is disposed), but consider adding validation or re-capture logic if DOM state changes are detected.

3. Heatmap Text Issue (Known Issue)

PR description mentions: "Still a minor issue with some heatmap charts, where the numeric text on the cell can be the same as the background."

Recommendation: Track this as a separate issue and add a TODO comment in the code where this edge case should be handled.

Security Concerns

1. DOM Injection (Low Risk)

pattern.ts:514-542 - Using innerHTML:

filter.innerHTML = filterHTML;

While the content is hardcoded (not user input), consider using explicit DOM methods for consistency with security best practices.

2. No Input Validation (Low)

Settings allow arbitrary color values without validation. While browsers handle invalid colors gracefully, explicit validation would be more robust.

Documentation & User Experience

✅ Strengths

1. README.md properly updated with keyboard shortcut documentation
2. Help menu updated
3. Settings UI includes all high contrast options
4. Notification on toggle ("High Contrast Mode on/off")

Suggestions

1. Add JSDoc @example tags to complex functions showing expected inputs/outputs
2. Consider adding a visual indicator in the UI when high contrast mode is active (not just keyboard toggle)
3. Document the "near white" heuristic more clearly - why is this needed for bar/histogram charts specifically?

Recommendations Summary

Must Fix Before Merge

1. ✅ Add test coverage for core functionality (especially color algorithms)
2. ⚠️ Fix memory leaks in dispose()
3. ⚠️ Address performance issues (canvas context reuse, element selector optimization)

Should Fix

1. Add null checks and error handling for color parsing
2. Extract magic numbers to named constants
3. Document edge cases (heatmap text issue)

Nice to Have

1. Consolidate color utility functions
2. Add @example JSDoc tags
3. Consider visual indicator for active high contrast mode

Overall Assessment

This is a well-designed feature with thoughtful architecture and lifecycle management. The main concerns are:

• Lack of test coverage for complex algorithms
• Performance optimizations needed for large charts
• Minor cleanup for maintainability

The code demonstrates good understanding of the codebase patterns and accessibility needs. With the recommended fixes, this will be a solid addition to the project.

Recommendation: Request changes for test coverage and performance fixes before merging.

---

Great work on this substantial feature! 🎉 The attention to lifecycle management and settings integration is particularly impressive.

[jooyoungseo]

@nk1408 Could you please cross the finish line by fixing Claude suggestions?

[github-actions]

⚠️ Changes detected in examples or UI directory

This PR contains changes to files in the examples/ or src/ui/ directories. Before merging, please ensure:

1. Run the e2e tests locally:

   npm run e2e

2. Verify that all examples still work as expected

3. Update any relevant documentation if needed

If you need help running the tests, please refer to our testing documentation.

[github-actions]

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

1. Run the unit tests locally:

   npm run test

2. Update tests if necessary

[nk1408]

@jooyoungseo , addressed the comments and merging it

[claude]

Pull Request Review - High Contrast Mode Feature

Summary

This PR adds a comprehensive high contrast accessibility feature triggered by the 'C' key. The implementation includes support for multiple contrast levels, pattern overlays for differentiation, and automatic color transformation. Overall, this is a well-architected and thoughtful addition to the accessibility features.

---

✅ Strengths

Architecture & Design

• Excellent service separation: HighContrastService and PatternService are well-separated with clear responsibilities
• Clean observer pattern: Properly uses the settings service's onChange event pattern for reactive updates
• Smart lifecycle management: Captures original colors on construction, applies on focus-in, and restores on blur - this prevents state corruption
• DOM validation: The validateCapturedElements() and recaptureIfNeeded() methods protect against stale DOM references (lines 202-223 in highContrast.ts)
• Comprehensive constants: HighContrastConstants object provides well-documented magic numbers with proper ITU-R BT.709 standard references

Code Quality

• Strong TypeScript usage: Proper interfaces (ElementColorInfo, PatternConfig), enums (HighContrastSettings), and type safety throughout
• Good documentation: JSDoc comments explain complex behavior, especially the constructor's capture-before-apply strategy (lines 149-162)
• Defensive programming: Handles edge cases like missing canvas context, invalid colors, and empty color arrays
• Reusable pattern system: The 5 distinct pattern types (diagonal-stripes, dots, square-grid, horizontal-lines, diamonds) provide excellent visual differentiation

---

⚠️ Issues & Concerns

1. Performance - DOM Querying (Medium Priority)

Location: highContrast.ts:435-507

const svgElements = svg.querySelectorAll('*');
for (let i = 0; i < svgElements.length; i++) {
  const el = svgElements[i];
  // ... color extraction logic
}

Issue: On complex charts with hundreds of elements, this could cause noticeable lag when toggling high contrast mode. The function iterates through every SVG element and performs regex matching on style attributes.

Recommendation: Consider implementing incremental/batched processing or caching strategies for large SVGs. Alternatively, use requestIdleCallback for non-blocking updates.

2. Memory Leak Risk - Canvas Elements (High Priority)

Location: Multiple locations (e.g., lines 627, 815, 842, 239 in highContrast.ts)

const ctx = document.createElement('canvas').getContext('2d');
if (!ctx) return value;
ctx.fillStyle = '#000';
ctx.fillStyle = value.trim();
let hex = ctx.fillStyle;

Issue: Canvas elements are created repeatedly for color normalization but never explicitly cleaned up. While GC should handle this, it can cause memory pressure during rapid toggling.

Recommendation: Create a single shared canvas context as a private class member and reuse it:

private canvasCtx: CanvasRenderingContext2D | null = null;

private getCanvasContext(): CanvasRenderingContext2D | null {
  if (!this.canvasCtx) {
    this.canvasCtx = document.createElement('canvas').getContext('2d');
  }
  return this.canvasCtx;
}

3. Bug - Array Mutation in findClosestColor (High Priority)

Location: highContrast.ts:726-728

if (cantBeBackground) {
  const backgroundIndex = colorArray.indexOf(this.highContrastDarkColor);
  if (backgroundIndex !== -1) {
    colorArray.splice(backgroundIndex, 1); // MUTATES INPUT ARRAY
  }
}

Issue: This mutates the input colorArray, which comes from this.colorEquivalents. Since colorEquivalents is a getter that creates a new array each time (line 118-123), this might be okay, but it's fragile and could break if the implementation changes.

Recommendation: Clone before mutating:

if (cantBeBackground) {
  const filteredColors = [...colorArray];
  const backgroundIndex = filteredColors.indexOf(this.highContrastDarkColor);
  if (backgroundIndex !== -1) {
    filteredColors.splice(backgroundIndex, 1);
  }
  colorArray = filteredColors;
}

4. Code Smell - Duplicated Service Dependencies (Low Priority)

Location: controller.ts:164-234

The same service/viewModel dependencies are passed three times identically to KeybindingService, Mousebindingservice, and CommandExecutor. This violates DRY principles and makes maintenance harder.

Recommendation: Extract to a shared dependency interface or consider dependency injection patterns.

5. Inconsistent Error Handling (Medium Priority)

Location: highContrast.ts:689

if (colorArray.length === 0) {
  throw new Error('Color array cannot be empty');
}

vs. Lines 628-629, 816-817, etc.:

if (!ctx) return value;  // Silent fallback

Issue: Some functions throw errors while others silently fall back. This inconsistency makes debugging difficult.

Recommendation: Establish consistent error handling - either log warnings with console.warn() or throw consistently.

6. Magic Number - Pattern Count (Low Priority)

Location: pattern.ts:154-161

The pattern assignment order is hardcoded but affects which data series get which pattern. This should be documented or configurable.

7. Missing Input Validation (Medium Priority)

Location: highContrast.ts:779-808 in interpolateColors()

const numColors = Math.max(2, Math.floor(count));

No validation for negative count or for invalid color strings. If parseColorToRgb() returns null, it silently returns [startColor, endColor] which might mask configuration errors.

---

🔒 Security Considerations

Status: ✅ No significant security concerns

• No user input is directly injected into HTML
• SVG filter IDs are safely constructed
• Pattern IDs use timestamp + random string to prevent collisions
• No use of eval() or innerHTML with user data

---

🧪 Test Coverage

Status: ⚠️ Missing E2E tests for this feature

Checked existing test files:

• e2e_tests/specs/*.spec.ts - No tests for high contrast mode

Recommendation: Add E2E tests covering:

1. Toggle high contrast mode with 'C' key
2. Verify color transformations are applied
3. Test pattern application on stacked/dodged bar charts
4. Verify restoration of original colors when toggling off
5. Test persistence of setting across blur/focus cycles
6. Test edge cases: charts with transparent elements, charts with near-white colors

Example test structure:

test('should toggle high contrast mode with C key', async ({ page }) => {
  await page.goto('/examples/barplot.html');
  await page.locator('#chart').click(); // Focus
  
  // Capture original colors
  const originalFill = await page.locator('rect.bar').first().getAttribute('fill');
  
  // Toggle high contrast
  await page.keyboard.press('c');
  const highContrastFill = await page.locator('rect.bar').first().getAttribute('fill');
  
  expect(highContrastFill).not.toBe(originalFill);
  
  // Toggle back
  await page.keyboard.press('c');
  const restoredFill = await page.locator('rect.bar').first().getAttribute('fill');
  
  expect(restoredFill).toBe(originalFill);
});

---

📊 Performance Considerations

Current Implementation

• Color capture: O(n) where n = SVG elements - done once
• Color application: O(n) on every toggle
• Pattern generation: O(1) per pattern type (cached)

Potential Optimizations

1. Debounce rapid toggles: Prevent performance issues if users rapidly toggle
2. Lazy pattern creation: Only create patterns for chart types that need them
3. Cache computed luminance values: The RGB→HSL conversions are expensive

---

📋 Minor Suggestions

1. README.md:409 - Typo fixed ("Hover on the datapoint to" → "Hover on the datapoint to move") ✅

2. README.md:422 - Good catch fixing "Toggle Text Mode" duplication to "Toggle Review Mode" ✅

3. Settings UI - The PR adds high contrast settings to Settings.tsx but the diff was too large to fully review. Ensure the UI properly reflects the 2+ contrast levels.

4. Consider adding keyboard shortcut documentation in the help modal for the 'C' key (appears to be already handled in help.ts:1)

5. Line chart glow filter - The multiple blur layers (20, 10, 5 at lines 582-600) seem reasonable but might benefit from user testing with screen magnification tools

---

🎯 Known Issues (from PR description)

The author notes:

"Still a minor issue with some heatmap charts, where the numeric text on the cell can be the same as the background."

Analysis: This is likely related to the hasParentWithStringInID(colorInfo.element, 'text') check at line 621. The text color is forced to highContrastLightColor but cell backgrounds might also be light.

Suggestion: Add additional logic to ensure text contrast ratio meets WCAG AA standards (4.5:1 for normal text). Consider using the getMostContrastingColor() method (lines 1062-1075) for text elements.

---

✅ Checklist for Merge

• Code follows TypeScript best practices
• Architecture is sound and maintainable
• Documentation is adequate
• Add E2E tests (blocking)
• Fix array mutation bug in findClosestColor (blocking)
• Address canvas memory leak (recommended before merge)
• Add performance optimization for large SVGs (can be follow-up)
• Fix heatmap text contrast issue (noted as known issue, can be follow-up)

---

🎉 Overall Assessment

This is a high-quality implementation of an important accessibility feature. The architecture is sound, the code is well-documented, and the feature set is comprehensive. The main blockers are:

1. Adding E2E test coverage
2. Fixing the array mutation bug
3. Addressing the canvas memory leak concern

Once these are addressed, this will be an excellent addition to the project.

Recommendation: Request changes for the blocking issues, then approve for merge.

---

Great work on this feature! The attention to accessibility details (proper luminance calculations, multiple pattern types, text glow filters) shows strong domain knowledge. 🚀

[github-actions]

⚠️ Changes detected in examples or UI directory

This PR contains changes to files in the examples/ or src/ui/ directories. Before merging, please ensure:

1. Run the e2e tests locally:

   npm run e2e

2. Verify that all examples still work as expected

3. Update any relevant documentation if needed

If you need help running the tests, please refer to our testing documentation.

[github-actions]

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

1. Run the unit tests locally:

   npm run test

2. Update tests if necessary

[claude]

Pull Request Review: High Contrast Mode Feature

Thank you for this comprehensive accessibility feature! This is a significant enhancement that will greatly benefit users with visual impairments. Below is my detailed review:

---

✅ Strengths

Architecture & Design

• Excellent service separation: The HighContrastService and PatternService are well-structured and follow single responsibility principle
• Smart lifecycle management: The capture/apply/restore pattern with initializeHighContrast() and suspendHighContrast() is elegant and prevents memory leaks
• DOM validation: The validateCapturedElements() and recaptureIfNeeded() methods prevent stale DOM references
• Observer pattern: Proper use of settings observers for reactive updates

Code Quality

• Named constants: Good use of HighContrastConstants enum to eliminate magic numbers
• Comprehensive documentation: JSDoc comments are thorough and helpful
• Color calculations: Proper use of ITU-R BT.709 luminance coefficients for perceptually accurate color conversions
• Performance optimization: Canvas context reuse reduces GC pressure

Accessibility

• Pattern fills: The addition of SVG patterns for stacked/dodged bars is a great touch for additional differentiation
• Text legibility: Glow shadow filters ensure text remains readable in high contrast mode
• ARIA announcements: Proper notifications when toggling the mode

---

Issues & Recommendations

1. Memory Leak Risk

Location: src/service/highContrast.ts:98-109

The sharedCanvasCtx is never cleaned up in the dispose() method. Add this.sharedCanvasCtx = null to cleanup.

2. Chart-Specific Logic in Generic Service

Locations: Lines 335-339, 348-355, 390-397, 674-683

The service contains hardcoded chart type checks. This violates the open/closed principle and creates tight coupling. Consider refactoring to a strategy pattern.

3. Error Handling Gaps

Location: src/service/highContrast.ts:704

The findClosestColor() method throws an error for empty color arrays, but this could crash the application. Return inputColor instead of throwing.

4. Missing Input Validation

The highContrastLevels input accepts any number value without validation (no min 2 / max checks).

5. Potential Performance Issue

getOriginalColorInfo() queries ALL SVG elements on every capture. For complex charts with thousands of elements, this could be slow.

6. Color Mutation Bug

Location: src/service/highContrast.ts:738-741

This mutates the colorArray parameter via splice(). Since colorArray comes from this.colorEquivalents getter, this means the computed getter returns different values after the first call! Create a copy of the array before mutating.

7. Race Condition in Settings Observer

Location: src/service/highContrast.ts:157-161

The observer only checks if high contrast mode changed. However, if highContrastLevels, highContrastLightColor, or highContrastDarkColor change while the mode is ON, the display will not update.

8. Incomplete Heatmap Fix

PR description acknowledges: Still a minor issue with some heatmap charts, where the numeric text on the cell can be the same as the background.

---

Testing Concerns

Missing Test Coverage:

• No unit tests for the new services
• No e2e tests for the keyboard shortcut (C key)
• No tests for settings persistence
• No tests for the pattern generation algorithms

---

Security Review

No Critical Issues Found - No XSS vulnerabilities, injection risks, or sensitive data exposure detected. Proper sanitization of user color inputs via canvas normalization.

---

Summary

Overall Assessment: This is a high-quality, well-architected feature with excellent attention to accessibility. The core implementation is solid, but there are several bugs and architectural concerns that should be addressed before merging.

Must Fix Before Merge:

• Fix color array mutation bug (item 6)
• Add input validation for highContrastLevels (item 4)
• Fix settings observer to handle all high contrast settings (item 7)
• Add canvas context cleanup to dispose() (item 1)

Should Fix Before Merge:

• Improve error handling (item 3)
• Add basic test coverage for core functionality

Can Address in Follow-up:

• Refactor chart-specific logic to strategy pattern (item 2)
• Optimize element querying for large charts (item 5)
• Complete heatmap text fix (item 8)

Great work on this feature! The attention to detail in color theory and accessibility is commendable. Please address the critical bugs, and this will be ready to ship.

[xabilitylab]

🎉 This PR is included in version 3.45.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀