-
Notifications
You must be signed in to change notification settings - Fork 504
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
fix(css): transparent images in dark mode issue #9863
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooo crud, yup, let's get this one merged. Thanks @OnkarRuikar!
Damn, I don't have privs to merge on this repo either. Can you handle this one @caugner, or maybe @Rumyra? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but let's add comments and change the color slightly.
Co-authored-by: Claas Augner <495429+caugner@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you! 🎉
Summary
The PR #9250 caused a regression. It changed background color of all the images from
white
totransparent
. Now all the transparent images inmdn/content
are facing issues in dark mode: black lines, black text, and black boxes are appearing invisible.Problem
Set page in dark mode.
Before the regression: https://web.archive.org/web/20230601124043/https://developer.mozilla.org/en-US/docs/Web/Performance/Animation_performance_and_frame_rate#the_rendering_waterfall
After the regression: https://developer.mozilla.org/en-US/docs/Web/Performance/Animation_performance_and_frame_rate#the_rendering_waterfall
Before regression:
After regression:
Solution
It is not feasible to make all the images compatible with both white and black backgrounds. Also, after the regression it is not easy to identify all the images that are affected by transparent background. We can't prevent future contributors from committing transparent illustrations.
It's better to keep the image background white. To solve original issue of alt text, set
color: black
on images.Screenshots
Before
After
Alt text scenario
How did you test this change?
In local browser after doing
yarn dev
.