Allow empty string as Image alt

Author: rbitting

README.md

@@ -126,7 +126,7 @@ Any use of third-party trademarks or logos are subject to those third-party's po
 | [emptyswatch-needs-labelling](docs/rules/emptyswatch-needs-labelling.md)                                               | Accessibility: EmptySwatch must have an accessible name via aria-label, Tooltip, aria-labelledby, etc..                                                | ✅  |    |    |
 | [field-needs-labelling](docs/rules/field-needs-labelling.md)                                                           | Accessibility: Field must have label                                                                                                                   | ✅  |    |    |
 | [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                                      | ✅  |    |    |
-| [image-needs-alt](docs/rules/image-needs-alt.md)                                                                       | Accessibility: Image must have alt attribute                                                                                                           | ✅  |    |    |
+| [image-needs-alt](docs/rules/image-needs-alt.md)                                                                       | Accessibility: Image must have alt attribute with a meaningful description of the image                                                                | ✅  |    |    |
 | [imageswatch-needs-labelling](docs/rules/imageswatch-needs-labelling.md)                                               | Accessibility: ImageSwatch must have an accessible name via aria-label, Tooltip, aria-labelledby, etc..                                                | ✅  |    |    |
 | [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                                         | ✅  |    |    |
 | [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.              | ✅  |    | 🔧 |

docs/rules/image-needs-alt.md

@@ -1,4 +1,4 @@
-# Accessibility: Image must have alt attribute (`@microsoft/fluentui-jsx-a11y/image-needs-alt`)
+# Accessibility: Image must have alt attribute with a meaningful description of the image (`@microsoft/fluentui-jsx-a11y/image-needs-alt`)
 
 💼 This rule is enabled in the ✅ `recommended` config.
 

lib/rules/image-needs-alt.ts

@@ -1,48 +1,27 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-const elementType = require("jsx-ast-utils").elementType;
-import { ESLintUtils, TSESTree } from "@typescript-eslint/utils";
-import { hasNonEmptyProp } from "../util/hasNonEmptyProp";
+import { ESLintUtils } from "@typescript-eslint/utils";
+import { makeLabeledControlRule } from "../util/ruleFactory";
 
 //------------------------------------------------------------------------------
 // Rule Definition
 //------------------------------------------------------------------------------
 
-const rule = ESLintUtils.RuleCreator.withoutDocs({
-    meta: {
-        type: "problem",
-        docs: {
-            description: "Accessibility: Image must have alt attribute",
-            recommended: "error"
-        },
-        messages: {
-            imageNeedsAlt: "Accessibility: Image must have alt attribute with a meaningful description of the image"
-        },
-        schema: []
-    },
-    defaultOptions: [], // no options needed
-    create(context) {
-        return {
-            // Listen for variable declarations
-            JSXOpeningElement(node: TSESTree.JSXOpeningElement) {
-                // No error if the element is not an Image
-                if (elementType(node) !== "Image") {
-                    return;
-                }
-
-                // No error if alt prop exists and is non-empty
-                if (hasNonEmptyProp(node.attributes, "alt")) {
-                    return;
-                }
-
-                context.report({
-                    node,
-                    messageId: "imageNeedsAlt"
-                });
-            }
-        };
-    }
-});
+const rule = ESLintUtils.RuleCreator.withoutDocs(
+    makeLabeledControlRule({
+        component: "Image",
+        messageId: "imageNeedsAlt",
+        description: "Accessibility: Image must have alt attribute with a meaningful description of the image",
+        requiredProps: ["alt"],
+        allowFieldParent: false,
+        allowHtmlFor: false,
+        allowLabelledBy: false,
+        allowWrappingLabel: false,
+        allowTooltipParent: false,
+        allowDescribedBy: false,
+        allowLabeledChild: false
+    })
+);
 
 export default rule;

lib/rules/swatchpicker-needs-labelling.ts

@@ -13,7 +13,7 @@ export default ESLintUtils.RuleCreator.withoutDocs(
         component: "SwatchPicker",
         messageId: "noUnlabeledSwatchPicker",
         description: "Accessibility: SwatchPicker must have an accessible name via aria-label, aria-labelledby, Field component, etc..",
-        labelProps: ["aria-label"],
+        requiredNonEmptyProps: ["aria-label"],
         allowFieldParent: true,
         allowHtmlFor: false,
         allowLabelledBy: true,

lib/util/hasDefinedProp.ts

@@ -0,0 +1,43 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+import { TSESTree } from "@typescript-eslint/utils";
+import { JSXOpeningElement } from "estree-jsx";
+import { hasProp, getPropValue, getProp } from "jsx-ast-utils";
+
+/**
+ * Determines if the property exists and has a non-nullish value.
+ * @param attributes The attributes on the visited node
+ * @param name The name of the prop to check
+ * @returns Whether the specified prop exists and is not null or undefined
+ * @example
+ * // <img src="image.png" />
+ * hasDefinedProp(attributes, 'src') // true
+ * // <img src="" />
+ * hasDefinedProp(attributes, 'src') // true
+ * // <img src={null} />
+ * hasDefinedProp(attributes, 'src') // false
+ * // <img src={undefined} />
+ * hasDefinedProp(attributes, 'src') // false
+ * // <img src={1} />
+ * hasDefinedProp(attributes, 'src') // false
+ * // <img src={true} />
+ * hasDefinedProp(attributes, 'src') // false
+ * // <img />
+ * hasDefinedProp(attributes, 'src') // false
+ */
+const hasDefinedProp = (attributes: TSESTree.JSXOpeningElement["attributes"], name: string): boolean => {
+    if (!hasProp(attributes as JSXOpeningElement["attributes"], name)) {
+        return false;
+    }
+
+    const prop = getProp(attributes as JSXOpeningElement["attributes"], name);
+
+    // Safely get the value of the prop, handling potential undefined or null values
+    const propValue = prop ? getPropValue(prop) : undefined;
+
+    // Return true if the prop value is not null or undefined
+    return propValue !== null && propValue !== undefined;
+};
+
+export { hasDefinedProp };

lib/util/ruleFactory.ts

@@ -14,26 +14,43 @@ import { elementType } from "jsx-ast-utils";
 import { JSXOpeningElement } from "estree-jsx";
 import { hasToolTipParent } from "./hasTooltipParent";
 import { hasLabeledChild } from "./hasLabeledChild";
+import { hasDefinedProp } from "./hasDefinedProp";
 import { hasTextContentChild } from "./hasTextContentChild";
 
+/**
+ * Configuration options for a rule created via the `ruleFactory`
+ */
 export type LabeledControlConfig = {
+    /** The name of the component that the rule applies to. @example 'Image', /Image|Icon/ */
     component: string | RegExp;
+    /** The unique id of the problem message. @example 'itemNeedsLabel' */
     messageId: string;
+    /** A short description of the rule. */
     description: string;
-    labelProps: string[]; // e.g. ["aria-label", "title", "label"]
-    allowFieldParent: boolean; // Accept a parent <Field label="..."> wrapper as providing the label.
-    allowHtmlFor: boolean; // Accept <label htmlFor="..."> association.
-    allowLabelledBy: boolean; // Accept aria-labelledby association.
-    allowWrappingLabel: boolean; // Accept being wrapped in a <label> element.
-    allowTooltipParent: boolean; // Accept a parent <Tooltip content="..."> wrapper as providing the label.
+    /** Properties that are required to have string values. @example ["alt"] */
+    requiredProps?: string[];
+    /** Properties that are required to be defined and not empty. @example ["aria-label", "title", "label"] */
+    requiredNonEmptyProps?: string[];
+    /** Accept a parent `<Field label="...">` wrapper as providing the label. */
+    allowFieldParent: boolean;
+    /** Accept `<label htmlFor="...">` association. */
+    allowHtmlFor: boolean;
+    /** Accept aria-labelledby association. */
+    allowLabelledBy: boolean;
+    /** Accept being wrapped in a `<label>` element. */
+    allowWrappingLabel: boolean;
+    /** Accept a parent `<Tooltip content="...">` wrapper as providing the label. */
+    allowTooltipParent: boolean;
     /**
      * Accept aria-describedby as a labeling strategy.
      * NOTE: This is discouraged for *primary* labeling; prefer text/aria-label/labelledby.
      * Keep this off unless a specific component (e.g., Icon-only buttons) intentionally uses it.
      */
     allowDescribedBy: boolean;
-    allowLabeledChild: boolean; // Accept labeled child elements to provide the label e.g. <Button><img alt="..." /></Button>
-    allowTextContentChild?: boolean; // Accept text children to provide the label e.g. <Button>Click me</Button>
+    /** Treat labeled child content (img `alt`, svg `title`, `aria-label` on `role="img"`) as the name */
+    allowLabeledChild: boolean;
+    /** Accept text children to provide the label e.g. <Button>Click me</Button> */
+    allowTextContentChild?: boolean;
 };
 
 /**
@@ -58,16 +75,22 @@ export function hasAccessibleLabel(
     context: TSESLint.RuleContext<string, []>,
     config: LabeledControlConfig
 ): boolean {
-    const allowFieldParent = !!config.allowFieldParent;
-    const allowWrappingLabel = !!config.allowWrappingLabel;
-    const allowHtmlFor = !!config.allowHtmlFor;
-    const allowLabelledBy = !!config.allowLabelledBy;
-    const allowTooltipParent = !!config.allowTooltipParent;
-    const allowDescribedBy = !!config.allowDescribedBy;
-    const allowLabeledChild = !!config.allowLabeledChild;
+    const {
+        allowFieldParent,
+        allowWrappingLabel,
+        allowHtmlFor,
+        allowLabelledBy,
+        allowTooltipParent,
+        allowDescribedBy,
+        allowLabeledChild,
+        requiredNonEmptyProps,
+        requiredProps
+    } = config;
     const allowTextContentChild = !!config.allowTextContentChild;
+
     if (allowFieldParent && hasFieldParent(context)) return true;
-    if (config.labelProps?.some(p => hasNonEmptyProp(opening.attributes, p))) return true;
+    if (requiredProps?.some(p => hasDefinedProp(opening.attributes, p))) return true;
+    if (requiredNonEmptyProps?.some(p => hasNonEmptyProp(opening.attributes, p))) return true;
     if (allowWrappingLabel && isInsideLabelTag(context)) return true;
     if (allowHtmlFor && hasAssociatedLabelViaHtmlFor(opening, context)) return true;
     if (allowLabelledBy && hasAssociatedLabelViaAriaLabelledBy(opening, context)) return true;

package.json

@@ -42,6 +42,8 @@
         "lint:eslint-docs": "npm-run-all \"update:eslint-docs -- --check\"",
         "lint:js": "eslint .",
         "test": "jest",
+        "test:branch": "npm run test -- -o",
+        "test:watch": "npm run test -- --watch",
         "lint:docs": "markdownlint **/*.md",
         "update:eslint-docs": "eslint-doc-generator",
         "fix:md": "npm run lint:docs -- --fix",

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

@@ -16,19 +16,16 @@ ruleTester.run("image-needs-alt", rule as unknown as Rule.RuleModule, {
         // Valid string test
         '<Image src="image.png" alt="Description of image" />',
         // Valid expression test
-        '<Image src="image.png" alt={altText} />'
+        '<Image src="image.png" alt={altText} />',
+        // Decorative image with empty alt
+        '<Image src="image.png" alt="" />'
     ],
     invalid: [
         {
             // No alt attribute
             code: '<Image src="image.png" />',
             errors: [{ messageId: "imageNeedsAlt" }]
         },
-        {
-            // Empty alt attribute
-            code: '<Image src="image.png" alt="" />',
-            errors: [{ messageId: "imageNeedsAlt" }]
-        },
         {
             // Null alt attribute
             code: '<Image src="image.png" alt={null} />',

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

@@ -0,0 +1,61 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+import { TSESTree } from "@typescript-eslint/utils";
+import { getProp, getPropValue, hasProp } from "jsx-ast-utils";
+import { hasDefinedProp } from "../../../../lib/util/hasDefinedProp";
+
+// Mocking getProp, getPropValue, and hasProp
+jest.mock("jsx-ast-utils", () => ({
+    hasProp: jest.fn(),
+    getProp: jest.fn(),
+    getPropValue: jest.fn()
+}));
+
+describe("hasDefinedProp", () => {
+    const attributes: TSESTree.JSXOpeningElement["attributes"] = [];
+    const propName = "testProp";
+
+    beforeEach(() => {
+        jest.clearAllMocks();
+    });
+
+    it("should return false if the property does not exist", () => {
+        (hasProp as jest.Mock).mockReturnValue(false);
+        const result = hasDefinedProp(attributes, propName);
+        expect(result).toBe(false);
+    });
+
+    it("should return false if the property is falsy", () => {
+        (hasProp as jest.Mock).mockReturnValue(true);
+        (getProp as jest.Mock).mockReturnValue(null);
+        const result = hasDefinedProp(attributes, propName);
+        expect(result).toBe(false);
+    });
+
+    it("should return false if the property value is undefined", () => {
+        (hasProp as jest.Mock).mockReturnValue(true);
+        (getProp as jest.Mock).mockReturnValue({});
+        (getPropValue as jest.Mock).mockReturnValue(undefined);
+        const result = hasDefinedProp(attributes, propName);
+        expect(result).toBe(false);
+    });
+
+    it("should return false if the property value is null", () => {
+        (hasProp as jest.Mock).mockReturnValue(true);
+        (getProp as jest.Mock).mockReturnValue({});
+        (getPropValue as jest.Mock).mockReturnValue(null);
+        const result = hasDefinedProp(attributes, propName);
+        expect(result).toBe(false);
+    });
+
+    ["non-empty string", "", 1, 0, true, false, [], {}].forEach(value => {
+        it(`should return true if the property value is: ${JSON.stringify(value)}`, () => {
+            (hasProp as jest.Mock).mockReturnValue(true);
+            (getProp as jest.Mock).mockReturnValue({});
+            (getPropValue as jest.Mock).mockReturnValue(value);
+            const result = hasDefinedProp(attributes, propName);
+            expect(result).toBe(true);
+        });
+    });
+});