Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Remove TypeError in reactA11yImgHasAltRule.ts for undefined role attribute #414

Closed
wants to merge 2 commits into from
Closed

Remove TypeError in reactA11yImgHasAltRule.ts for undefined role attribute #414

wants to merge 2 commits into from

Conversation

garrow
Copy link
Contributor

@garrow garrow commented Feb 1, 2018

Fix

Ensure that undefined role attributes on images do not cause TypeErrors in the reactA11yImgHasAltRule rule itself.

This is achieved by coercing a possibly undefined role attribute value to a string.

Fixes #390

Background

We discovered a case where we use a possibly undefined role value for images which have a caption provided. This is both valid React, and an allowable value for that attribute (type is string | undefined).

getStringLiteral

tslint-microsoft-contrib/src/utils/JsxAttribute.ts

Lines 35 to 51 in b5e3cfc

export function getStringLiteral(node: ts.JsxAttribute | ts.JsxSpreadAttribute): string {
if (!isJsxAttribute(node)) {
throw new Error('The node must be a JsxAttribute collected by the AST parser.');
}
const initializer: ts.Expression = node == null ? null : node.initializer;
if (!initializer) { // <tag attribute/>
return '';
} else if (isStringLiteral(initializer)) { // <tag attribute='value' />
return initializer.text.trim();
} else if (isJsxExpression(initializer) && isStringLiteral(initializer.expression)) { // <tag attribute={'value'} />
return (<ts.StringLiteral>initializer.expression).text;
} else if (isJsxExpression(initializer) && !initializer.expression) { // <tag attribute={} />
return '';
} else {
return undefined;

Error Case

import * as React from 'react';

export const ImageWithUndefinedAttributes = ({ caption, src }: { caption?: string; src: string; }) => (
  <img
    alt={caption}
    className="classes"
    role={caption ? undefined : 'presentation'}
    src={src}
  />
);

Workaround

There is a workaround, though is a little unwieldy, destructuring either an empty object, or one providing the role property.

export const ImageWithConditionalAttributes = ({ caption, src }: { caption?: string; src: string; }) => (
  <img
    alt={caption}
    className="classes"
    {... caption ? {} : { role : 'presentation' }}
    src={src}
  />
);

Testing

The test cases added do not actually fail before the fix, but do emit TypeError console errors.

Note that the example below were obtained by modifying the Gruntfile to run a single test dist/src/tests/reactA11yImgHasAltRuleTests.js in order to highlight the case in question.

Before

grunt mochaTest
Running "mochaTest:test" (mochaTest) task

  reactA11yImgHasAlt
    default tests
      should pass
        ✓ when the element name is not img
        ✓ when the img tag name is not lower case
        ✓ when the img element has spread attribute
        ✓ when the img element has empty alt value and presentation role
        ✓ when the img element has non-empty alt value and not presentation role
TypeError: Cannot read property 'toLowerCase' of undefined
  at ImgHasAltWalker.checkJsxOpeningElement (/code/tslint-microsoft-contrib/dist/src/reactA11yImgHasAltRule.js:91:58)
  at ImgHasAltWalker.visitJsxSelfClosingElement (/code/tslint-microsoft-contrib/dist/src/reactA11yImgHasAltRule.js:69:14)
  at ImgHasAltWalker.SyntaxWalker.visitNode (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:413:22)
  at /code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:535:63
  at visitNode (/code/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:12690:24)
  at Object.forEachChild (/code/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:12754:21)
  at ImgHasAltWalker.SyntaxWalker.walkChildren (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:535:12)
  at ImgHasAltWalker.SyntaxWalker.visitVariableDeclaration (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:261:14)
  at ImgHasAltWalker.SyntaxWalker.visitNode (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:515:22)
  at /code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:535:63
  at visitNodes (/code/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:12699:30)
  at Object.forEachChild (/code/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:12884:24)
  at ImgHasAltWalker.SyntaxWalker.walkChildren (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:535:12)
  at ImgHasAltWalker.SyntaxWalker.visitVariableDeclarationList (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:264:14)
  at ImgHasAltWalker.SyntaxWalker.visitNode (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:518:22)
  at /code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:535:63
  at visitNode (/code/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:12690:24)
  at Object.forEachChild (/code/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:12882:21)
  at ImgHasAltWalker.SyntaxWalker.walkChildren (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:535:12)
  at ImgHasAltWalker.SyntaxWalker.visitVariableStatement (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:267:14)
  at ImgHasAltWalker.SyntaxWalker.visitNode (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:521:22)
  at /code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:535:63
  at visitNodes (/code/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:12699:30)
  at Object.forEachChild (/code/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:12877:24)
  at ImgHasAltWalker.SyntaxWalker.walkChildren (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:535:12)
  at ImgHasAltWalker.SyntaxWalker.visitSourceFile (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:228:14)
  at ImgHasAltWalker.SyntaxWalker.visitNode (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:482:22)
  at ImgHasAltWalker.SyntaxWalker.walk (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/walker/syntaxWalker.js:24:14)
  at Rule.AbstractRule.applyWithWalker (/code/tslint-microsoft-contrib/node_modules/tslint/lib/language/rule/abstractRule.js:31:16)
  at Rule.apply (/code/tslint-microsoft-contrib/dist/src/reactA11yImgHasAltRule.js:38:20)
  at Linter.applyRule (/code/tslint-microsoft-contrib/node_modules/tslint/lib/linter.js:177:29)
  at /code/tslint-microsoft-contrib/node_modules/tslint/lib/linter.js:119:85
  at Object.flatMap (/code/tslint-microsoft-contrib/node_modules/tslint/lib/utils.js:151:29)
  at Linter.getAllFailures (/code/tslint-microsoft-contrib/node_modules/tslint/lib/linter.js:119:32)
  at Linter.lint (/code/tslint-microsoft-contrib/node_modules/tslint/lib/linter.js:75:33)
  at runRule (/code/tslint-microsoft-contrib/dist/src/tests/TestHelper.js:73:20)
  at runRuleAndEnforceAssertions (/code/tslint-microsoft-contrib/dist/src/tests/TestHelper.js:94:26)
  at Object.assertNoViolation (/code/tslint-microsoft-contrib/dist/src/tests/TestHelper.js:16:9)
  at Context.<anonymous> (/code/tslint-microsoft-contrib/dist/src/tests/reactA11yImgHasAltRuleTests.js:32:41)
  at callFn (/code/tslint-microsoft-contrib/node_modules/mocha/lib/runnable.js:354:21)
  at Test.Runnable.run (/code/tslint-microsoft-contrib/node_modules/mocha/lib/runnable.js:346:7)
  at Runner.runTest (/code/tslint-microsoft-contrib/node_modules/mocha/lib/runner.js:442:10)
  at /code/tslint-microsoft-contrib/node_modules/mocha/lib/runner.js:560:12
  at next (/code/tslint-microsoft-contrib/node_modules/mocha/lib/runner.js:356:14)
  at /code/tslint-microsoft-contrib/node_modules/mocha/lib/runner.js:366:7
  at next (/code/tslint-microsoft-contrib/node_modules/mocha/lib/runner.js:290:14)
  at Immediate.<anonymous> (/code/tslint-microsoft-contrib/node_modules/mocha/lib/runner.js:334:5)
  at runCallback (timers.js:637:20)
  at tryOnImmediate (timers.js:610:5)
  at processImmediate [as _immediateCallback] (timers.js:582:5)

        ✓ when the img element has non-empty alt value and undefined presentation role (48ms)
        ✓ when the img element has non-empty alt value and presentation role when option is enabled
      should fail
        ✓ when the img element has no alt prop
        ✓ when the img element has empty alt value and not presentation role
        ✓ when the img element has non-empty alt value and presentation role
    custom element tests
      should pass
        ✓ when the element is neither img nor custom element
        ✓ when custom element or img has empty alt value and presentation role
        ✓ when custom element or img has non-empty alt value and not presentation role
      should fail
        ✓ when custom element or img has no alt prop
        ✓ when custom element or img has non-empty alt value and presentation role
        ✓ when custom element or img has empty alt value and not presentation role


  16 passing (184ms)


Done.

After

> grunt mochaTest
Running "mochaTest:test" (mochaTest) task


  reactA11yImgHasAlt
    default tests
      should pass
        ✓ when the element name is not img
        ✓ when the img tag name is not lower case
        ✓ when the img element has spread attribute
        ✓ when the img element has empty alt value and presentation role
        ✓ when the img element has non-empty alt value and not presentation role
        ✓ when the img element has non-empty alt value and undefined presentation role
        ✓ when the img element has non-empty alt value and presentation role when option is enabled
      should fail
        ✓ when the img element has no alt prop
        ✓ when the img element has empty alt value and not presentation role
        ✓ when the img element has non-empty alt value and presentation role
    custom element tests
      should pass
        ✓ when the element is neither img nor custom element
        ✓ when custom element or img has empty alt value and presentation role
        ✓ when custom element or img has non-empty alt value and not presentation role
      should fail
        ✓ when custom element or img has no alt prop
        ✓ when custom element or img has non-empty alt value and presentation role
        ✓ when custom element or img has empty alt value and not presentation role


  16 passing (90ms)


Done.

@msftclas
Copy link

msftclas commented Feb 1, 2018 •
edited
Loading

CLA assistant check
All CLA requirements met.

@garrow
Copy link
Contributor Author

garrow commented Feb 1, 2018

Attempting to fix #390

@HamletDRC HamletDRC closed this in b81ad57 Feb 13, 2018
@HamletDRC
Copy link
Member

Thanks!

@HamletDRC HamletDRC added this to the 5.0.3 milestone Feb 13, 2018
@garrow garrow deleted the bugfix/react-a11y-image-alt-typeerror branch February 13, 2018 21:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.0.3
Development

Successfully merging this pull request may close these issues.

react-a11y-image-button-has-alt toLowerCase error throws
3 participants
@garrow @msftclas @HamletDRC