Skip to content

Commit ea3da3f

Browse files
rbittingrbitting
committed
Allow empty string as Image alt
1 parent db486dc commit ea3da3f

File tree

10 files changed

+186
-67
lines changed

10 files changed

+186
-67
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ Any use of third-party trademarks or logos are subject to those third-party's po
124124
| [dropdown-needs-labelling](docs/rules/dropdown-needs-labelling.md) | Accessibility: Dropdown menu must have an id and it needs to be linked via htmlFor of a Label | ✅ | | |
125125
| [field-needs-labelling](docs/rules/field-needs-labelling.md) | Accessibility: Field must have label | ✅ | | |
126126
| [image-button-missing-aria](docs/rules/image-button-missing-aria.md) | Accessibility: Image buttons must have accessible labelling: title, aria-label, aria-labelledby, aria-describedby | ✅ | | |
127-
| [image-needs-alt](docs/rules/image-needs-alt.md) | Accessibility: Image must have alt attribute | ✅ | | |
127+
| [image-needs-alt](docs/rules/image-needs-alt.md) | Accessibility: Image must have alt attribute with a meaningful description of the image | ✅ | | |
128128
| [input-components-require-accessible-name](docs/rules/input-components-require-accessible-name.md) | Accessibility: Input fields must have accessible labelling: aria-label, aria-labelledby or an associated label | ✅ | | |
129129
| [link-missing-labelling](docs/rules/link-missing-labelling.md) | Accessibility: Image links must have an accessible name. Add either text content, labelling to the image or labelling to the link itself. | ✅ | | 🔧 |
130130
| [menu-item-needs-labelling](docs/rules/menu-item-needs-labelling.md) | Accessibility: MenuItem without label must have an accessible and visual label: aria-labelledby | ✅ | | |

‎docs/rules/image-needs-alt.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Accessibility: Image must have alt attribute (`@microsoft/fluentui-jsx-a11y/image-needs-alt`)
1+
# Accessibility: Image must have alt attribute with a meaningful description of the image (`@microsoft/fluentui-jsx-a11y/image-needs-alt`)
22

33
💼 This rule is enabled in the ✅ `recommended` config.
44

‎lib/rules/image-needs-alt.ts‎

Lines changed: 17 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,48 +1,27 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33

4-
const elementType = require("jsx-ast-utils").elementType;
5-
import { ESLintUtils, TSESTree } from "@typescript-eslint/utils";
6-
import { hasNonEmptyProp } from "../util/hasNonEmptyProp";
4+
import { ESLintUtils } from "@typescript-eslint/utils";
5+
import { makeLabeledControlRule } from "../util/ruleFactory";
76

87
//------------------------------------------------------------------------------
98
// Rule Definition
109
//------------------------------------------------------------------------------
1110

12-
const rule = ESLintUtils.RuleCreator.withoutDocs({
13-
meta: {
14-
type: "problem",
15-
docs: {
16-
description: "Accessibility: Image must have alt attribute",
17-
recommended: "error"
18-
},
19-
messages: {
20-
imageNeedsAlt: "Accessibility: Image must have alt attribute with a meaningful description of the image"
21-
},
22-
schema: []
23-
},
24-
defaultOptions: [], // no options needed
25-
create(context) {
26-
return {
27-
// Listen for variable declarations
28-
JSXOpeningElement(node: TSESTree.JSXOpeningElement) {
29-
// No error if the element is not an Image
30-
if (elementType(node) !== "Image") {
31-
return;
32-
}
33-
34-
// No error if alt prop exists and is non-empty
35-
if (hasNonEmptyProp(node.attributes, "alt")) {
36-
return;
37-
}
38-
39-
context.report({
40-
node,
41-
messageId: "imageNeedsAlt"
42-
});
43-
}
44-
};
45-
}
46-
});
11+
const rule = ESLintUtils.RuleCreator.withoutDocs(
12+
makeLabeledControlRule({
13+
component: "Image",
14+
messageId: "imageNeedsAlt",
15+
description: "Accessibility: Image must have alt attribute with a meaningful description of the image",
16+
requiredProps: ["alt"],
17+
allowFieldParent: false,
18+
allowHtmlFor: false,
19+
allowLabelledBy: false,
20+
allowWrappingLabel: false,
21+
allowTooltipParent: false,
22+
allowDescribedBy: false,
23+
allowLabeledChild: false
24+
})
25+
);
4726

4827
export default rule;

‎lib/rules/swatchpicker-needs-labelling.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ export default ESLintUtils.RuleCreator.withoutDocs(
1313
component: "SwatchPicker",
1414
messageId: "noUnlabeledSwatchPicker",
1515
description: "Accessibility: SwatchPicker must have an accessible name via aria-label, aria-labelledby, Field component, etc..",
16-
labelProps: ["aria-label"],
16+
requiredNonEmptyProps: ["aria-label"],
1717
allowFieldParent: true,
1818
allowHtmlFor: false,
1919
allowLabelledBy: true,

‎lib/util/hasDefinedProp.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
import { TSESTree } from "@typescript-eslint/utils";
5+
import { JSXOpeningElement } from "estree-jsx";
6+
import { hasProp, getPropValue, getProp } from "jsx-ast-utils";
7+
8+
/**
9+
* Determines if the property exists and has a non-nullish value.
10+
* @param attributes The attributes on the visited node
11+
* @param name The name of the prop to check
12+
* @returns Whether the specified prop exists and is not null or undefined
13+
* @example
14+
* // <img src="image.png" />
15+
* hasDefinedProp(attributes, 'src') // true
16+
* // <img src="" />
17+
* hasDefinedProp(attributes, 'src') // true
18+
* // <img src={null} />
19+
* hasDefinedProp(attributes, 'src') // false
20+
* // <img src={undefined} />
21+
* hasDefinedProp(attributes, 'src') // false
22+
* // <img src={1} />
23+
* hasDefinedProp(attributes, 'src') // false
24+
* // <img src={true} />
25+
* hasDefinedProp(attributes, 'src') // false
26+
* // <img />
27+
* hasDefinedProp(attributes, 'src') // false
28+
*/
29+
const hasDefinedProp = (attributes: TSESTree.JSXOpeningElement["attributes"], name: string): boolean => {
30+
if (!hasProp(attributes as JSXOpeningElement["attributes"], name)) {
31+
return false;
32+
}
33+
34+
const prop = getProp(attributes as JSXOpeningElement["attributes"], name);
35+
36+
// Safely get the value of the prop, handling potential undefined or null values
37+
const propValue = prop ? getPropValue(prop) : undefined;
38+
39+
// Return true if the prop value is not null or undefined
40+
return propValue !== null && propValue !== undefined;
41+
};
42+
43+
export { hasDefinedProp };

‎lib/util/ruleFactory.ts‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,39 @@ import { elementType } from "jsx-ast-utils";
1414
import { JSXOpeningElement } from "estree-jsx";
1515
import { hasToolTipParent } from "./hasTooltipParent";
1616
import { hasLabeledChild } from "./hasLabeledChild";
17+
import { hasDefinedProp } from "./hasDefinedProp";
1718

19+
/**
20+
* Configuration options for a rule created via the `ruleFactory`
21+
*/
1822
export type LabeledControlConfig = {
23+
/** The name of the component that the rule applies to. @example 'Image', /Image|Icon/ */
1924
component: string | RegExp;
25+
/** The unique id of the problem message. @example 'itemNeedsLabel' */
2026
messageId: string;
27+
/** A short description of the rule. */
2128
description: string;
22-
labelProps: string[]; // e.g. ["aria-label", "title", "label"]
23-
/** Accept a parent <Field label="..."> wrapper as providing the label. */
24-
allowFieldParent: boolean; // default false
25-
allowHtmlFor: boolean /** Accept <label htmlFor="..."> association. */;
26-
allowLabelledBy: boolean /** Accept aria-labelledby association. */;
27-
allowWrappingLabel: boolean /** Accept being wrapped in a <label> element. */;
28-
allowTooltipParent: boolean /** Accept a parent <Tooltip content="..."> wrapper as providing the label. */;
29+
/** Properties that are required to have string values. @example ["alt"] */
30+
requiredProps?: string[];
31+
/** Properties that are required to be defined and not empty. @example ["aria-label", "title", "label"] */
32+
requiredNonEmptyProps?: string[];
33+
/** Accept a parent `<Field label="...">` wrapper as providing the label. */
34+
allowFieldParent: boolean;
35+
/** Accept `<label htmlFor="...">` association. */
36+
allowHtmlFor: boolean;
37+
/** Accept aria-labelledby association. */
38+
allowLabelledBy: boolean;
39+
/** Accept being wrapped in a `<label>` element. */
40+
allowWrappingLabel: boolean;
41+
/** Accept a parent `<Tooltip content="...">` wrapper as providing the label. */
42+
allowTooltipParent: boolean;
2943
/**
3044
* Accept aria-describedby as a labeling strategy.
3145
* NOTE: This is discouraged for *primary* labeling; prefer text/aria-label/labelledby.
3246
* Keep this off unless a specific component (e.g., Icon-only buttons) intentionally uses it.
3347
*/
3448
allowDescribedBy: boolean;
35-
// NEW: treat labeled child content (img alt, svg title, aria-label on role="img") as the name
49+
/** Treat labeled child content (img `alt`, svg `title`, `aria-label` on `role="img"`) as the name */
3650
allowLabeledChild: boolean;
3751
};
3852

@@ -53,16 +67,21 @@ export type LabeledControlConfig = {
5367
* This checks for presence of an accessible *name* only; not contrast or UX.
5468
*/
5569
export function hasAccessibleLabel(node: TSESTree.JSXOpeningElement, context: any, config: LabeledControlConfig): boolean {
56-
const allowFieldParent = !!config.allowFieldParent;
57-
const allowWrappingLabel = !!config.allowWrappingLabel;
58-
const allowHtmlFor = !!config.allowHtmlFor;
59-
const allowLabelledBy = !!config.allowLabelledBy;
60-
const allowTooltipParent = !!config.allowTooltipParent;
61-
const allowDescribedBy = !!config.allowDescribedBy;
62-
const allowLabeledChild = !!config.allowLabeledChild;
70+
const {
71+
allowFieldParent,
72+
allowWrappingLabel,
73+
allowHtmlFor,
74+
allowLabelledBy,
75+
allowTooltipParent,
76+
allowDescribedBy,
77+
allowLabeledChild,
78+
requiredProps,
79+
requiredNonEmptyProps
80+
} = config;
6381

6482
if (allowFieldParent && hasFieldParent(context)) return true;
65-
if (config.labelProps?.some(p => hasNonEmptyProp(node.attributes, p))) return true;
83+
if (requiredProps?.some(p => hasDefinedProp(node.attributes, p))) return true;
84+
if (requiredNonEmptyProps?.some(p => hasNonEmptyProp(node.attributes, p))) return true;
6685
if (allowWrappingLabel && isInsideLabelTag(context)) return true;
6786
if (allowHtmlFor && hasAssociatedLabelViaHtmlFor(node, context)) return true;
6887
if (allowLabelledBy && hasAssociatedLabelViaAriaLabelledBy(node, context)) return true;

‎package.json‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@
4242
"lint:eslint-docs": "npm-run-all \"update:eslint-docs -- --check\"",
4343
"lint:js": "eslint .",
4444
"test": "jest",
45+
"test:branch": "npm run test -- -o",
46+
"test:watch": "npm run test -- --watch",
4547
"lint:docs": "markdownlint **/*.md",
4648
"update:eslint-docs": "eslint-doc-generator",
4749
"fix:md": "npm run lint:docs -- --fix",

‎tests/lib/rules/image-needs-alt.test.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,16 @@ ruleTester.run("image-needs-alt", rule as unknown as Rule.RuleModule, {
1616
// Valid string test
1717
'<Image src="image.png" alt="Description of image" />',
1818
// Valid expression test
19-
'<Image src="image.png" alt={altText} />'
19+
'<Image src="image.png" alt={altText} />',
20+
// Decorative image with empty alt
21+
'<Image src="image.png" alt="" />'
2022
],
2123
invalid: [
2224
{
2325
// No alt attribute
2426
code: '<Image src="image.png" />',
2527
errors: [{ messageId: "imageNeedsAlt" }]
2628
},
27-
{
28-
// Empty alt attribute
29-
code: '<Image src="image.png" alt="" />',
30-
errors: [{ messageId: "imageNeedsAlt" }]
31-
},
3229
{
3330
// Null alt attribute
3431
code: '<Image src="image.png" alt={null} />',

‎tests/lib/rules/utils/hasDefinedProp.test.ts‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
import { TSESTree } from "@typescript-eslint/utils";
5+
import { getProp, getPropValue, hasProp } from "jsx-ast-utils";
6+
import { hasDefinedProp } from "../../../../lib/util/hasDefinedProp";
7+
8+
// Mocking getProp, getPropValue, and hasProp
9+
jest.mock("jsx-ast-utils", () => ({
10+
hasProp: jest.fn(),
11+
getProp: jest.fn(),
12+
getPropValue: jest.fn()
13+
}));
14+
15+
describe("hasDefinedProp", () => {
16+
const attributes: TSESTree.JSXOpeningElement["attributes"] = [];
17+
const propName = "testProp";
18+
19+
beforeEach(() => {
20+
jest.clearAllMocks();
21+
});
22+
23+
it("should return false if the property does not exist", () => {
24+
(hasProp as jest.Mock).mockReturnValue(false);
25+
const result = hasDefinedProp(attributes, propName);
26+
expect(result).toBe(false);
27+
});
28+
29+
it("should return false if the property is falsy", () => {
30+
(hasProp as jest.Mock).mockReturnValue(true);
31+
(getProp as jest.Mock).mockReturnValue(null);
32+
const result = hasDefinedProp(attributes, propName);
33+
expect(result).toBe(false);
34+
});
35+
36+
it("should return false if the property value is undefined", () => {
37+
(hasProp as jest.Mock).mockReturnValue(true);
38+
(getProp as jest.Mock).mockReturnValue({});
39+
(getPropValue as jest.Mock).mockReturnValue(undefined);
40+
const result = hasDefinedProp(attributes, propName);
41+
expect(result).toBe(false);
42+
});
43+
44+
it("should return false if the property value is null", () => {
45+
(hasProp as jest.Mock).mockReturnValue(true);
46+
(getProp as jest.Mock).mockReturnValue({});
47+
(getPropValue as jest.Mock).mockReturnValue(null);
48+
const result = hasDefinedProp(attributes, propName);
49+
expect(result).toBe(false);
50+
});
51+
52+
["non-empty string", "", 1, 0, true, false, [], {}].forEach(value => {
53+
it(`should return true if the property value is: ${JSON.stringify(value)}`, () => {
54+
(hasProp as jest.Mock).mockReturnValue(true);
55+
(getProp as jest.Mock).mockReturnValue({});
56+
(getPropValue as jest.Mock).mockReturnValue(value);
57+
const result = hasDefinedProp(attributes, propName);
58+
expect(result).toBe(true);
59+
});
60+
});
61+
});

‎tests/lib/rules/utils/ruleFactory.test.ts‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33

44
import { AST_NODE_TYPES } from "@typescript-eslint/utils";
55

6+
jest.mock("../../../../lib/util/hasDefinedProp", () => ({
7+
hasDefinedProp: jest.fn()
8+
}));
69
jest.mock("../../../../lib/util/hasNonEmptyProp", () => ({
710
hasNonEmptyProp: jest.fn()
811
}));
@@ -22,6 +25,7 @@ jest.mock("../../../../lib/util/hasTooltipParent", () => ({
2225
hasToolTipParent: jest.fn()
2326
}));
2427

28+
import { hasDefinedProp } from "../../../../lib/util/hasDefinedProp";
2529
import { hasNonEmptyProp } from "../../../../lib/util/hasNonEmptyProp";
2630
import {
2731
hasAssociatedLabelViaAriaLabelledBy,
@@ -40,6 +44,7 @@ import { hasToolTipParent } from "../../../../lib/util/hasTooltipParent";
4044

4145
// Helper: reset all mocks to a default "false" stance
4246
const resetAllMocksToFalse = () => {
47+
(hasDefinedProp as jest.Mock).mockReset().mockReturnValue(false);
4348
(hasNonEmptyProp as jest.Mock).mockReset().mockReturnValue(false);
4449
(hasAssociatedLabelViaAriaLabelledBy as jest.Mock).mockReset().mockReturnValue(false);
4550
(hasAssociatedLabelViaAriaDescribedby as jest.Mock).mockReset().mockReturnValue(false);
@@ -72,9 +77,10 @@ function makeOpeningElement(name: string, attributes: Array<Partial<TSESTree.JSX
7277
/* -------------------------------------------------------------------------- */
7378

7479
describe("hasAccessibleLabel (unit)", () => {
75-
const cfg: LabeledControlConfig = {
80+
const cfg: Required<LabeledControlConfig> = {
7681
component: "RadioGroup",
77-
labelProps: ["label", "aria-label"],
82+
requiredProps: ["alt"],
83+
requiredNonEmptyProps: ["label", "aria-label"],
7884
allowFieldParent: true,
7985
allowHtmlFor: true,
8086
allowLabelledBy: true,
@@ -115,6 +121,17 @@ describe("hasAccessibleLabel (unit)", () => {
115121
expect(hasAccessibleLabel(node, {}, cfg)).toBe(true);
116122
});
117123

124+
test("true when a label prop is defined via hasDefinedProp", () => {
125+
(hasDefinedProp as jest.Mock).mockImplementation((attrs, name) => (name === "alt" ? true : false));
126+
const node = makeOpeningElement("RadioGroup", [
127+
{
128+
type: AST_NODE_TYPES.JSXAttribute,
129+
name: { type: AST_NODE_TYPES.JSXIdentifier, name: "alt", range: [0, 0], loc: {} as any }
130+
}
131+
]);
132+
expect(hasAccessibleLabel(node, {}, cfg)).toBe(true);
133+
});
134+
118135
test("true when allowWrappingLabel and isInsideLabelTag(ctx) === true", () => {
119136
(isInsideLabelTag as jest.Mock).mockReturnValue(true);
120137
const node = makeOpeningElement("RadioGroup");
@@ -147,9 +164,10 @@ describe("hasAccessibleLabel (unit)", () => {
147164
const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6, ecmaFeatures: { jsx: true } } });
148165

149166
describe("makeLabeledControlRule (RuleTester integration)", () => {
150-
const baseCfg: LabeledControlConfig = {
167+
const baseCfg: Required<LabeledControlConfig> = {
151168
component: "RadioGroup",
152-
labelProps: ["label", "aria-label"],
169+
requiredProps: ["alt"],
170+
requiredNonEmptyProps: ["label", "aria-label"],
153171
allowFieldParent: true,
154172
allowHtmlFor: true,
155173
allowLabelledBy: true,

0 commit comments

Comments
 (0)