Skip to content

Conversation

@eslteacher902010
Copy link
Contributor

Fixes: [processing/p5.js-website] [Content] Insufficient text-to-background contrast (Issue #886)

Added

/* Force 'function' keyword to have better contrast /
code span[style
="#D73A49"] {
color: #000 !important;
}

which seems to have fixed the contrast issue with the word 'function' in the tutorial part of the website.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM

I also noticed that the contrast between the annotations and the background color needs to be adjusted.

截圖 2025-07-28 晚上9 58 25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll take a look at that and get back to you soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @coseeian — I realized there was a small improvement I probably should have included in the function color fix. Using *= like this:

code span[style*="#D73A49"] {
color: #000 !important;
}
makes the selector more flexible. I noticed a couple of the function keywords were still showing up pinkish which made me want to try this fix.

That said, if some text is supposed to stay that color, feel free to let me know — this might not be the right adjustment in that case.

I also tweaked the annotation styling to make it a bit darker — I assumed you were referring to the // circle in the center..... (in your picture above)

BTW, you’ll see both changes reflected in the Files Changed tab.

Let me know if you need anything else. Thanks!!

@ksen0
Copy link
Member

ksen0 commented Sep 23, 2025

Hi @eslteacher902010, sorry for the long delay in reviewing this. Please let me know if you'd like to finish this issue based on the feedback below, or if it should be reassigned - totally ok either way, thanks so much for your work already. You can confirm either way within the next ~7 days (though you don't have to finish in that time, of course)

The challenge was that the approach in your PR doesn't quite resolve all instances of insufficient contrast in a maintainable way - rather than overriding individual colors, it would be better to use (or create) a new theme for the AnnotatedCode blocks. So I didn't want to respond here until I had a better understanding myself of what the better approach would be (thanks to @davepagurek who explained the below to me, as I was not sure how to proceed in reviewing this issue)

@davepagurek suggested to look for available themes for these two libraries, CodeMirror and shiki.

So the PR for this issue should, instead of using CSS to override specific elements, switch the theme used in the code blocks to a theme that has sufficient text-to-background contrast. If no such theme exists, then this PR should rather add such a theme and use it - though making new themes might need a bit more research and iteration.

What do you think?

@eslteacher902010
Copy link
Contributor Author

Hi @ksen0, thanks so much for the thoughtful review and for pointing me to the right files. I kind of suspected my original approach was more of a patch than a maintainable fix, so it makes sense that a theme-based solution would be better long-term.

My schedule probably only allows me a few hours to work on this, but I’d like to take a stab at updating it with the theme approach you and @davepagurek suggested (starting with existing CodeMirror/shiki options). If it looks like it’ll take more than I can realistically handle, I’ll let you know so it can be reassigned.

Appreciate the guidance!

@ksen0
Copy link
Member

ksen0 commented Sep 23, 2025

Thanks for the quick ping @eslteacher902010, that sounds good! Please don't hesitate to follow up here or on Discord, I'll be happy to help as well as I can. Especially if a new theme turns out to be necessary.

@eslteacher902010
Copy link
Contributor Author

Thanks, will definitely do that! :)

@eslteacher902010
Copy link
Contributor Author

@ksen0 @davepagurek Hey thanks again for the guidance. I tried switching to a high-contrast GitHub theme, but it looks like the current Shiki version (1.1.7) doesn’t include it — so the site falls back to the default and the pink “function” is still there. Would you be open to me upgrading Shiki to a newer version so we can use github-light-high-contrast directly? That way we’d meet the accessibility need without a CSS patch. If there's a solution I'm missing, let me know. Thanks.

@ksen0
Copy link
Member

ksen0 commented Sep 26, 2025

@eslteacher902010 Thanks for the update and the research! Yes, please try to upgrade; if the tests pass with the new version, then no problem. If there are any issues with upgrading, I'd be happy to help too. Just to confirm, the github-light-high-contrast theme does have sufficient contrast?

@eslteacher902010
Copy link
Contributor Author

eslteacher902010 commented Sep 27, 2025

@eslteacher902010 Thanks for the update and the research! Yes, please try to upgrade; if the tests pass with the new version, then no problem. If there are any issues with upgrading, I'd be happy to help too. Just to confirm, the github-light-high-contrast theme does have sufficient contrast?

Hi @ksen0, I’ve upgraded Shiki and set the theme to github-light-high-contrast. Functionally this works — Shiki is outputting the expected colors — but the CSS rules (maybe .code-box) overrides are muting those changes in the rendered site so I'm unable to see the change but I can confirm it's happening by console logging.

I do think GitHub Light High Contrast is a decent choice from seeing previews of it. Would you prefer I push this update as-is, or adjust/remove background rules so the new theme is fully visible? I’m also mindful of not getting too far into the weeds here, so happy to follow whatever approach you and @davepagurek think is best. Thanks!

@eslteacher902010
Copy link
Contributor Author

@eslteacher902010 Thanks for the update and the research! Yes, please try to upgrade; if the tests pass with the new version, then no problem. If there are any issues with upgrading, I'd be happy to help too. Just to confirm, the github-light-high-contrast theme does have sufficient contrast?

Hi @ksen0, I’ve upgraded Shiki and set the theme to github-light-high-contrast. Functionally this works — Shiki is outputting the expected colors — but the CSS rules (maybe .code-box) overrides are muting those changes in the rendered site so I'm unable to see the change but I can confirm it's happening by console logging.

I do think GitHub Light High Contrast is a decent choice from seeing previews of it. Would you prefer I push this update as-is, or adjust/remove background rules so the new theme is fully visible? I’m also mindful of not getting too far into the weeds here, so happy to follow whatever approach you and @davepagurek think is best. Thanks!

@eslteacher902010
Copy link
Contributor Author

fyi...Just pushed the Shiki upgrade and applied the GitHub Light High Contrast theme to both annotated and editable code blocks. Figured it might be helpful for you to see it. Thanks again for the helpful guidance!

@ksen0
Copy link
Member

ksen0 commented Oct 13, 2025

Hi @eslteacher902010 , if you're still working on this:

I do think GitHub Light High Contrast is a decent choice from seeing previews of it. Would you prefer I push this update as-is, or adjust/remove background rules so the new theme is fully visible?

I think removing the rules to make the theme visible makes sense, but if it is not practically feasible, then maybe a different approach overall can be found. But that feels reasonable.

The last commit wasn't building - do you need any support with that?

@eslteacher902010
Copy link
Contributor Author

Thanks, @ksen0! I checked the build logs — the failure was due to CodeMirror not exporting githubLightHighContrast from @uiw/codemirror-theme-github. That theme only exists for Shiki (didn't occur to me at the time), so I’ll switch to githubLight for that part I think because it should be similar and I think should work.

Sorry for the delay — I’ll push the fix and reopen the PR within 48 hours. Not sure how I closed it (butterfingers, I guess!). I’ll also see if tweaking the CSS helps reveal the theme. If it’s still not working, I’m happy to hand it off to @NalinDalal or anyone else.

@eslteacher902010
Copy link
Contributor Author

So what I did was switch CodeMirror to use github-light instead of github-light-high-contrast, since the latter wasn’t supported and was causing the code blocks not to render correctly.

I then adjusted the global SCSS overrides, prioritizing Shiki’s styles so they actually show through. Even after that, I noticed some pages were still lacking contrast, which I think comes down to github-light in CodeMirror not providing quite enough contrast.

So I added a small tweak in global.scss to improve contrast — again, not really a patch or permanent override, just a tweak.

I think it looks better now for sure.
I’d merge this, but I’ll leave it up to you and your judgment.

@ksen0
Copy link
Member

ksen0 commented Oct 23, 2025

Hi @eslteacher902010 , sorry it took a while to review this. After npm install, npm run build produces:

 generating static routes 
Named export 'githubLight' not found. The requested module '@uiw/codemirror-theme-github' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@uiw/codemirror-theme-github';
const { githubLight } = pkg;

And npm run dev results in:

image

If actually my issue is not building correctly, please let me know? Otherwise, the theme import would be good to fix.
Another contributor has expressed interest in taking it on, cold you tag them please if you don't have capacity to finish this? But it's up to you. (Or if you don't ping here in a week, they can pick up where you started.)

Thanks for all your work on this already!

@eslteacher902010
Copy link
Contributor Author

Thanks for the note, @ksen0. I double-checked the imports, and it turns out the ESM import (import { githubLight } from '@uiw/codemirror-theme-github') works fine in this Astro/Vite setup.

The earlier build issue seemed to be a Vite caching thing — once I cleared it and rebuilt, everything seemed to compile cleanly.

I just pushed that version. Appreciate you taking the time to check and follow up!

@NalinDalal feel free to take a look whenever you get a chance.

Comment on lines 567 to 568
margin-top: var(--spacing-sm);
margin-bottom: var(--spacing-sm);
Copy link
Collaborator

@coseeian coseeian Oct 23, 2025

Choose a reason for hiding this comment

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

Though I'm still trying to build a more holistic understanding of this PR (especially regarding accessibility), I would like to bring up a potential minor UX regression I noticed here first:

The addition of margin-top: var(--spaceing-sm) to the .cm-editor causes the copy & reset buttons to visually overflow their parent container within the CodeEmbed component.

Screenshots below for reference.

Before

https://p5js.org/tutorials/setting-up-your-environment/

Image

After

After I built this PR at my local: http://localhost:4321/tutorials/setting-up-your-environment/

Image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Verified that the issue has been fixed.
#908 (comment)

@eslteacher902010
Copy link
Contributor Author

fixed?

Hopefully, the most recent commit — where I removed the extra var() calls in global.scss — resolves the issue. Sorry I missed that earlier.

FYI--the first time the build failed because I had forgotten I had the absolute path in the file.

I’ve agreed to let @NalinDalal take a look at any other issues that might come up.

@coseeian
Copy link
Collaborator

Thank you @eslteacher902010. I've built the latest commit d422492 locally and confirmed that the issue with the copy & reset buttons overflowing their parent container in the CodeEmbed component is fixed.

@coseeian
Copy link
Collaborator

coseeian commented Oct 25, 2025

I spent some time investigating the issue mentioned here today. For future reference, here is what I found:

... I’ve upgraded Shiki and set the theme to github-light-high-contrast. Functionally this works — Shiki is outputting the expected colors — but the CSS rules (maybe .code-box) overrides are muting those changes in the rendered site so I'm unable to see the change but I can confirm it's happening by console logging.

The site appears to be running two different versions of Shiki simultaneously:

% npm ls shiki
p5.js-website@0.0.1
├─┬ astro@4.5.6
│ ├─┬ @astrojs/markdown-remark@4.3.0
│ │ └── shiki@1.29.2 deduped
│ └── shiki@1.29.2
└── shiki@3.13.0

The newer v3.13.0 (as updated in this PR) is used by the AnnotatedCode component, but the older v1.29.2 is still used by Astro for syntax highlighting (configured via astro.config.mjs) in components like CodeBlockWrapper.

I think this is why the new github-light-high-contrast theme is not being consistently applied across all code blocks after the Shiki upgrade.

@NalinDalal
Copy link
Contributor

hi, following after 2 days, was busy with something else, so is the issue fixed, and package needs to be made regular across whole repo?
kindly explain the future steps, are they need to be authored by @eslteacher902010 ?

@eslteacher902010
Copy link
Contributor Author

Thanks for the detailed breakdown, @coseeian — that makes a lot of sense about the two Shiki versions. Before I (or @NalinDalal ) jump in, do you have a preferred approach for unifying them? For example, would you lean toward upgrading Astro’s internal Shiki version or pinning everything to 1.29.2 for consistency?

@NalinDalal
Copy link
Contributor

hi @eslteacher902010 , i think we should upgrade.

@coseeian
Copy link
Collaborator

coseeian commented Oct 30, 2025

I really wish I could agree, but it looks like astro@v4 is still using shiki@v1. The shiki@v3 update doesn't seem to be included until astro@v5.

https://unpkg.com/astro@4.16.19/package.json

Upgrading either package is challenging due to breaking changes. It would also introduce modifications unrelated to the original text/background contrast issue in code blocks.

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.

4 participants