Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement minimum contrast ratio #2563

Merged
merged 35 commits into from
Nov 15, 2019
Merged

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Nov 12, 2019

This feature tries to ensure a minimum contrast ratio by increasing or reducing the luminance of cell foreground colors.

Fixes #1514

minimumContrastRatio: 1

Screen Shot 2019-11-11 at 11 02 51 PM

minimumContrastRatio: 7

Screen Shot 2019-11-11 at 11 05 47 PM

It helps fix an accessibility issue I have with some themes where ls colors in particular clash and make something very unreadable like this:

Screen Shot 2019-11-11 at 11 15 22 PM

Fixed with minimumContrastRatio: 4.5

Screen Shot 2019-11-11 at 11 16 02 PM

Care was taken with performance with the default skipping the code path all together, avoiding object creation and after the cache is done this should have very minimal impact on performance. This also includes a big reorganization of the DOM and WebGL glyph rendering code. Note that this is not supported on the canvas renderer as non-cached glyphs would be required which are too slow.

TODO:

  • Cache resolved minimum contrast ratio calculations
  • Fix tests (they broke because I tweaked the luminance algorithm)
  • Do a once over of the code in case I missed something

@Tyriar Tyriar added this to the 4.3.0 milestone Nov 12, 2019
@Tyriar Tyriar self-assigned this Nov 12, 2019
@Tyriar
Copy link
Member Author

Tyriar commented Nov 12, 2019

1:

Screen Shot 2019-11-12 at 9 44 59 AM

15:

Screen Shot 2019-11-12 at 9 45 10 AM

@Tyriar Tyriar force-pushed the 322_minimum_contrast branch from ea80f14 to 20b0a95 Compare November 12, 2019 19:06
Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

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

Wow thats alot of changes. I really have no clue about your calculation in Color.ts, maybe you could comment there abit on the formulas and constants used.
Made a few code comments below.

@Tyriar Tyriar merged commit 12e9dce into xtermjs:master Nov 15, 2019
@Tyriar Tyriar deleted the 322_minimum_contrast branch November 15, 2019 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce a minimum contrast level
2 participants