Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add test cases
  • Loading branch information
khiga8 committed Oct 6, 2023
commit 488fbb4ccd85c13a7e099916e4e08f63b320716a
35 changes: 35 additions & 0 deletions docs/rules/GH003-no-empty-string-alt.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# GH003 No empty string alt

## Rule details

This rule is _off_ by default and is only applicable for GitHub rendered markdown.

Currently, all images on github.com are automatically wrapped in an anchor tag.

As a result, images that are intentionally marked as decorative (via `alt=""``) end up as a link without an accessible name. This is confusing for assistive technology users.

This rule can be enabled to enforce that the alt attribute is always set to descriptive text.

This rule should be removed once this behavior is updated on GitHub's UI.

## Examples

### Incorrect 👎

```md
![""](cat.png)
Copy link
Member

@iansan5653 iansan5653 Oct 6, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! You're right -- thank you for catching that! I'll stick with HTML for this rule!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

```

```html
<img src="cat.png" alt="">
```

### Correct 👍

```md
![Mona Lisa](mona.png)
```

```html
<img src="mona.png" alt="Mona Lisa, the Octocat">
```
84 changes: 84 additions & 0 deletions test/no-empty-string-alt.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
const noEmptyStringAltRule = require("../src/rules/no-empty-string-alt");
const runTest = require("./utils/run-test").runTest;

describe("GH003: No Empty String Alt", () => {
describe("successes", () => {
test("inline", async () => {
const strings = [
"![Chart with a single root node reading 'Example'](https://user-images.githubusercontent.com/abcdef.png)",
];

const results = await runTest(strings, noEmptyStringAltRule);

for (const result of results) {
expect(result).not.toBeDefined();
}
});
test("html image", async () => {
const strings = [
'<img alt="A helpful description" src="https://user-images.githubusercontent.com/abcdef.png">',
];

const results = await runTest(strings, noEmptyStringAltRule);

for (const result of results) {
expect(result).not.toBeDefined();
}
});
});
describe("failures", () => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Done in 8f80d4d.

test("markdown example", async () => {
const strings = [
"![''](https://user-images.githubusercontent.com/abcdef.png)",
'![""](https://user-images.githubusercontent.com/abcdef.png)',
];

const results = await runTest(strings, noEmptyStringAltRule);

const failedRules = results
.map((result) => result.ruleNames)
.flat()
.filter((name) => !name.includes("GH"));

expect(failedRules).toHaveLength(2);
for (const rule of failedRules) {
expect(rule).toBe("no-empty-string-alt");
}
});

test("HTML example", async () => {
const strings = [
'<img alt="" src="https://user-images.githubusercontent.com/abcdef.png">',
"<img alt='' src='https://user-images.githubusercontent.com/abcdef.png'>",
];

const results = await runTest(strings, noEmptyStringAltRule);

const failedRules = results
.map((result) => result.ruleNames)
.flat()
.filter((name) => !name.includes("GH"));

expect(failedRules).toHaveLength(2);
for (const rule of failedRules) {
expect(rule).toBe("no-empty-string-alt");
}
});

test("error message", async () => {
const strings = [
"![''](https://user-images.githubusercontent.com/abcdef.png)",
'<img alt="" src="https://user-images.githubusercontent.com/abcdef.png">',
];

const results = await runTest(strings, noEmptyStringAltRule);

expect(results[0].ruleDescription).toMatch(
"Please provide an alternative text for the image.",
);
expect(results[1].ruleDescription).toMatch(
"Please provide an alternative text for the image.",
);
});
});
});
6 changes: 5 additions & 1 deletion test/usage.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ describe("usage", () => {
describe("default export", () => {
test("custom rules on default export", () => {
const rules = githubMarkdownLint;
expect(rules).toHaveLength(2);
expect(rules).toHaveLength(3);

expect(rules[0].names).toEqual(["GH001", "no-default-alt-text"]);
expect(rules[1].names).toEqual(["GH002", "no-generic-link-text"]);
expect(rules[2].names).toEqual(["GH003", "no-empty-string-alt"]);
});
});
describe("init method", () => {
Expand All @@ -17,6 +20,7 @@ describe("usage", () => {
"no-space-in-links": false,
"single-h1": true,
"no-emphasis-as-header": true,
"no-empty-string-alt": false,
"heading-increment": true,
"no-generic-link-text": true,
"ul-style": {
Expand Down