Introduce a no-empty-alt-text rule
#85
Conversation
| ### Incorrect 👎 | ||
|
|
||
| ```md | ||
|  |
There was a problem hiding this comment.
I'm not sure about this example. The alt text in the Markdown form is never quoted like an HTML property, so this would result in an image tag like <img alt='""' src="cat.png" />. While an alt text of "" is not useful, it's not empty either. The only way to create a truly empty alt tag in Markdown is with the HTML form alt="", so maybe we should only focus on HTML for this rule.
There was a problem hiding this comment.
I think a useful-alt-text rule might be an interesting avenue to explore, where we ban things like "screenshot", "image", "example", '""', etc. But I think that's a separate rule from empty text.
There was a problem hiding this comment.
Ah! You're right -- thank you for catching that! I'll stick with HTML for this rule!
There was a problem hiding this comment.
useful-alt-text
The no-default-alt-text currently flags alternative text like, image, Image, Screen Shot 2022-06-26 at 7 41 30 PM though it's not comprehensive by any means. We could potentially expand on that?
There was a problem hiding this comment.
Yeah, I think we definitely could expand on that! It might be interesting to see if we can run a query to figure out what the most common alt texts are across GitHub -- I bet the top 20 or so are all things we'd probably want to lint against.
src/rules/no-empty-string-alt.js
Outdated
| const inlineImages = params.parsers.markdownit.tokens.filter( | ||
| (token) => | ||
| token.type === "inline" && | ||
| token.children.some((child) => child.type === "image"), | ||
| ); |
There was a problem hiding this comment.
Per other comment - we probably should limit this rule to just HTML tags
There was a problem hiding this comment.
Removed markdown syntax in 00fc5dd!
test/no-empty-string-alt.test.js
Outdated
| expect(results).toHaveLength(0); | ||
| }); | ||
| }); | ||
| describe("failures", () => { |
There was a problem hiding this comment.
Not super important, but it might also be useful to have a test for two failures in one line, like:
<img src="cat.png" alt="" /> <img src="dog.png" alt="" />I think we do handle this correctly with the g flag and iterating over matches per line, but it probably would still be good to test it.
There was a problem hiding this comment.
Good idea! Done in 8f80d4d.
src/rules/index.js
Outdated
| rules: [ | ||
| require("./no-default-alt-text"), | ||
| require("./no-generic-link-text"), | ||
| require("./no-empty-string-alt"), |
There was a problem hiding this comment.
🤔 Maybe we should call this one no-empty-alt or no-empty-alt-text?
There was a problem hiding this comment.
I like no-empty-alt-text!
Co-authored-by: Ian Sanders <iansan5653@github.com>
Co-authored-by: Ian Sanders <iansan5653@github.com>
This reverts commit 5b17abf. Reverting because we are using this index.
khiga8
changed the title
no-empty-string-alt ruleno-empty-alt-text rule
Co-authored-by: Ian Sanders <iansan5653@github.com>
Co-authored-by: Ian Sanders <iansan5653@github.com>
discussing over Slack! |
This PR introduces a new rule,
no-empty-alt-textwhich nudges people to add alt text when they set an image to decorative. Settingalt=""to mark decorative images is usually acceptable, but currently leads to unexpected behaviors, specifically on GitHub.There is currently a known issue on GitHub where all images are rendered inside of an anchor tag. As a result of this, decorative images result in links without an accessible name. Therefore, it's a good idea to always set alt text on markdown images on GitHub. However, this rule isn't really a long term solution and this issue is best fixed in GitHub in the long-term.
This rule should be removed in the future when we stops wrapping images in anchor tags.
We plan to enable this in github/accessibility-alt-text-bot#33.