Skip to content

Commit

Permalink
Enable jsx-a11y eslint rules (#2232)
Browse files Browse the repository at this point in the history
## Summary:
This work is part of the implementation of [ADR#781 Enabling more lint rules for accessibility](https://khanacademy.atlassian.net/wiki/x/IoBVyg)

These changes include:
- Updating `@khanacademy/eslint-config` to the latest version to include new jsx-a11y configuration and settings
- Updating `esling-plugin-jsx-a11y` to the latest version so it is up to date
- Addressed existing errors by adding `eslint-disable-next-line`
  - View the PR commits for a breakdown of errors by rule
  - Used `npx [email protected] .` to disable lint errors per rule
- Provided custom component mapping for jsx-a11y in the eslint config since Perseus has custom components 

New potential a11y issues can be prevented with these lint rules and existing errors can be addressed over time by the team! The lint error messages have a link to the [rule docs](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/tree/main/docs/rules) for more details!

A breakdown of the existing jsx-a11y errors:

<img width="1019" alt="perseus-linting-errors-before" src="https://github.com/user-attachments/assets/8191e551-4147-43cb-bbcf-79b0e5684c26" />

### Implementation Plan:
- Khan/wonder-stuff#1114 Set up eslint a11y config and enable aphrodite-add-style-variable-name 
- Use this new config in WB, perseus, webapp and address lint errors from the aphrodite-add-style-variable-name rule
  - Khan/wonder-blocks#2459
  - #2193
  - Khan/webapp#29212
- Khan/wonder-stuff#1118 Update the eslint-config/a11y config with the config based on the accessibility linting rules ADR
- (includes this PR) Use the updated config in projects and address existing errors by disabling the rules per line.

Issue: FEI-1133

## Test plan:
- Run `yarn lint` and make sure there are no linting errors

Author: beaesguerra

Reviewers: catandthemachines, beaesguerra, #frontend-infra-web

Required Reviewers:

Approved By: catandthemachines

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #2232
  • Loading branch information
beaesguerra authored Feb 13, 2025
1 parent 3ec6ec1 commit dc99898
Show file tree
Hide file tree
Showing 34 changed files with 783 additions and 60 deletions.
7 changes: 7 additions & 0 deletions .changeset/four-pans-sneeze.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@khanacademy/math-input": patch
"@khanacademy/perseus": patch
"@khanacademy/perseus-editor": patch
---

Tooling: Enabled jsx-a11y lint rules and disabled existing errors that were found
13 changes: 13 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,19 @@ module.exports = {
react: {
version: "detect",
},
"jsx-a11y": {
// Map perseus components to their corresponding DOM elements. This
// helps eslint-plugin-jsx-a11y understand what the underlying
// elements are. For more details, see
// https://github.com/jsx-eslint/eslint-plugin-jsx-a11y?tab=readme-ov-file#component-mapping
components: {
NumericInput: "input",
ScrolllessNumberTextField: "input",
BlurInput: "input",
TextInput: "input",
NumberInput: "input",
},
},
},
env: {
"cypress/globals": true,
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"@cypress/code-coverage": "^3.12.24",
"@cypress/react": "^8.0.0",
"@jest/globals": "^29.7.0",
"@khanacademy/eslint-config": "^5.1.0",
"@khanacademy/eslint-config": "^5.2.0",
"@khanacademy/eslint-plugin": "^3.1.1",
"@khanacademy/mathjax-renderer": "^2.1.1",
"@khanacademy/wonder-blocks-button": "7.1.1",
Expand Down Expand Up @@ -84,7 +84,7 @@
"eslint-plugin-import": "^2.29.1",
"eslint-plugin-jest": "28.9.0",
"eslint-plugin-jsdoc": "^48.2.1",
"eslint-plugin-jsx-a11y": "^6.7.1",
"eslint-plugin-jsx-a11y": "^6.10.2",
"eslint-plugin-monorepo": "^0.3.2",
"eslint-plugin-prettier": "^5.1.3",
"eslint-plugin-promise": "^6.1.1",
Expand Down
2 changes: 2 additions & 0 deletions packages/math-input/src/components/input/math-input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -992,9 +992,11 @@ class MathInput extends React.Component<Props, State> {
>
{/* NOTE(charlie): This is used purely to namespace the styles in
overrides.css. */}
{/* eslint-disable-next-line jsx-a11y/no-static-element-interactions -- TODO(LEMS-2871): Address a11y error */}
<div
className="keypad-input"
// @ts-expect-error - TS2322 - Type 'string' is not assignable to type 'number | undefined'.
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex -- TODO(LEMS-2871): Address a11y error
tabIndex={"0"}
ref={(node) => {
this.inputRef = node;
Expand Down
1 change: 1 addition & 0 deletions packages/math-input/src/fake-react-native-web/view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ class View extends React.Component<Props> {
(this.props.extraClassName ? ` ${this.props.extraClassName}` : "");

return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error
<div
className={className}
style={this.props.dynamicStyle}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/components/graph-settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,7 @@ class GraphSettings extends React.Component<Props, State> {
</div>
)}
<div className="perseus-widget-row">
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>Markings: </label>
<ButtonGroup
value={this.props.markings}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/components/sortable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ class SortableItem extends React.Component<ItemProps> {
}

return (
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions -- TODO(LEMS-2871): Address a11y error
<li
draggable={this.props.draggable}
className={[dragState, css(styles.sortableListItem)].join(" ")}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/diffs/text-diff.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ class TextDiff extends React.Component<any, any> {
</div>
{_.map([BEFORE, AFTER], (side, index) => {
return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error
<div
className={className + " " + side}
key={index}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/diffs/widget-diff.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class CollapsedRow extends React.Component<any> {
render(): React.ReactNode {
const self = this;
return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error
<div onClick={self.props.onClick} style={{clear: "both"}}>
{_.map([BEFORE, AFTER], function (side) {
return (
Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/widgets/cs-program-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ class CSProgramEditor extends React.Component<any> {
}}
/>
<br />
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>
Settings:
<PairsEditor
Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/widgets/grapher-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ class GrapherEditor extends React.Component<Props> {
onChange={this.change("graph")}
/>
<div className="perseus-widget-row">
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>Available functions: </label>
<MultiButtonGroup
allowEmpty={false}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/widgets/iframe-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ class IframeEditor extends React.Component<IframeEditorProps> {
/>
</label>
<br />
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>
Settings:
<PairsEditor
Expand Down
4 changes: 4 additions & 0 deletions packages/perseus-editor/src/widgets/image-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,10 @@ class ImageEditor extends React.Component<Props> {
<div className="image-settings">
{!Util.isLabeledSVG(backgroundImage.url) && (
<div>
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>
<div>Preview:</div>
{/* eslint-disable-next-line jsx-a11y/img-redundant-alt -- TODO(LEMS-2871): Address a11y error */}
<img
alt="Editor preview of image"
src={backgroundImage.url}
Expand All @@ -281,6 +283,7 @@ class ImageEditor extends React.Component<Props> {
</div>

<div>
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>
<div>
Alt text:
Expand All @@ -303,6 +306,7 @@ class ImageEditor extends React.Component<Props> {
</label>
</div>
<div>
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>
<div>Caption:</div>
<Editor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ const DraggableGripIcon = () => (
* A button link to add a new answer.
*/
const AddAnswer = ({onClick}: AddAnswerProps): React.ReactElement => (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events, jsx-a11y/anchor-is-valid -- TODO(LEMS-2871): Address a11y error
<Link
className={css(styles.addAnswer, editorStyles.addAnswer)}
onClick={onClick}
Expand All @@ -111,6 +112,7 @@ const Answer = ({
onRemove,
}: AnswerProps): React.ReactElement => (
<li className={css(styles.answer)}>
{/* eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events, jsx-a11y/anchor-is-valid -- TODO(LEMS-2871): Address a11y error, TODO(LEMS-2871): Address a11y error */}
<Link onClick={onRemove}>
<Icon icon={removeIcon} size={24} color="#D92916" />
</Link>
Expand All @@ -125,6 +127,7 @@ const Answer = ({

<div className={css(styles.spacer)} />

{/* eslint-disable-next-line jsx-a11y/anchor-is-valid -- TODO(LEMS-2871): Address a11y error */}
<Link
style={[styles.disabled]}
title="Answer reordering is not implemented."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ export default class QuestionMarkers extends React.Component<Props> {
maxHeight: imageHeight,
}}
>
{/* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions -- TODO(LEMS-2871): Address a11y error */}
<img
alt=""
className={css(styles.image)}
Expand Down
3 changes: 3 additions & 0 deletions packages/perseus-editor/src/widgets/numeric-input-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ class NumericInputEditor extends React.Component<Props, State> {
const suggestedAnswerTypes = (i: any) => (
<>
<div className="perseus-widget-row">
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>Possible answer formats&ensp;</label>
<InfoTip>
<p>
Expand Down Expand Up @@ -482,6 +483,7 @@ class NumericInputEditor extends React.Component<Props, State> {
const labelText = (
<>
<div className="perseus-widget-row">
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>Aria label</label>
<InfoTip>
<p>
Expand Down Expand Up @@ -588,6 +590,7 @@ class NumericInputEditor extends React.Component<Props, State> {
(answer.maxError ? " with-max-error" : "")
}
>
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control -- TODO(LEMS-2871): Address a11y error */}
<label>User input:</label>
<NumberInput
value={answer.value}
Expand Down
2 changes: 2 additions & 0 deletions packages/perseus/src/components/__tests__/zoomable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ describe("Zoomable", () => {
const onClickHandler = jest.fn();
renderAndWaitToSettle(
<Zoomable>
{/* eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error */}
<span onClick={onClickHandler}>Some zoomable text</span>
</Zoomable>,
);
Expand All @@ -274,6 +275,7 @@ describe("Zoomable", () => {
const onClickHandler = jest.fn();
rerender(
<Zoomable>
{/* eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error */}
<span onClick={onClickHandler}>Some zoomable text</span>
</Zoomable>,
);
Expand Down
3 changes: 3 additions & 0 deletions packages/perseus/src/components/graph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -415,15 +415,18 @@ class Graph extends React.Component<Props> {
}

return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error
<div
className="graphie-container blank-background"
style={{
width: this.props.box[0],
height: this.props.box[1],
}}
// @ts-expect-error - TS2339 - Property 'onMouseOut' does not exist on type 'Graph'.
// eslint-disable-next-line jsx-a11y/mouse-events-have-key-events -- TODO(LEMS-2871): Address a11y error
onMouseOut={this.onMouseOut}
// @ts-expect-error - TS2339 - Property 'onMouseOver' does not exist on type 'Graph'.
// eslint-disable-next-line jsx-a11y/mouse-events-have-key-events -- TODO(LEMS-2871): Address a11y error
onMouseOver={this.onMouseOver}
// @ts-expect-error - TS2339 - Property 'onClick' does not exist on type 'Graph'.
onClick={this.onClick}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ class HighlightTooltip extends React.PureComponent<Props> {
// TooltipContent wouldn't let me overwrite
// user-select and onClick
const content: any = (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error
<div
className={css(styles.tooltipLabel)}
onClick={this.props.onClick}
Expand All @@ -111,6 +112,7 @@ class HighlightTooltip extends React.PureComponent<Props> {
} as const;

return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions -- TODO(LEMS-2871): Address a11y error
<div
style={style}
onMouseEnter={this.props.onMouseEnter}
Expand Down
2 changes: 2 additions & 0 deletions packages/perseus/src/components/image-loader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ class ImageLoader extends React.Component<Props, State> {
const staticUrl = getDependencies().staticUrl;

return (
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions -- TODO(LEMS-2871): Address a11y error
<img
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex -- TODO(LEMS-2871): Address a11y error
tabIndex={0}
src={staticUrl(src)}
onKeyUp={onKeyUp}
Expand Down
2 changes: 2 additions & 0 deletions packages/perseus/src/components/math-input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ class InnerMathInput extends React.Component<InnerProps, State> {
this.props.hasError && styles.wrapperError,
]}
>
{/* eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error */}
<div
style={{
display: "flex",
Expand All @@ -330,6 +331,7 @@ class InnerMathInput extends React.Component<InnerProps, State> {
});
}}
>
{/* eslint-disable-next-line jsx-a11y/no-static-element-interactions -- TODO(LEMS-2871): Address a11y error */}
<span
className={className}
ref={(ref) => (this.__mathFieldWrapperRef = ref)}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/components/sortable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ class Draggable extends React.Component<DraggableProps, DraggableState> {
style.margin = this.props.margin;
}
return (
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions -- TODO(LEMS-2871): Address a11y error
<li
className={className}
style={style}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/components/zoomable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ class Zoomable extends React.Component<Props, State> {
} as const;

return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- TODO(LEMS-2871): Address a11y error
<span
onClick={this.handleClick}
onClickCapture={this.handleClickIfZoomed}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/perseus-api.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ export const ApiOptions = {
Link: (
props: any,
): React.ReactElement<React.ComponentProps<"a">> => {
// eslint-disable-next-line jsx-a11y/anchor-has-content -- TODO(LEMS-2871): Address a11y error
return <a {...props} />;
},
},
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/categorizer/categorizer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ export class Categorizer
}
key={catNum}
>
{/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/interactive-supports-focus -- TODO(LEMS-2871): Address a11y error */}
<div
role="button"
aria-label={catName}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/cs-program/cs-program.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ class CSProgram extends React.Component<Props> implements Widget {
styleContainer && styles.container,
)}
>
{/* eslint-disable-next-line jsx-a11y/iframe-has-title -- TODO(LEMS-2871): Address a11y error */}
<iframe
sandbox={sandboxOptions}
src={url}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/expression/expression.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ export class Expression
{this.props.visibleLabel}
</LabelSmall>
)}
{/* eslint-disable-next-line jsx-a11y/no-static-element-interactions -- TODO(LEMS-2871): Address a11y error */}
<div
className={className}
onBlur={() =>
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/iframe/iframe.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ class Iframe extends React.Component<Props> implements Widget {
// creator "went wild".
// http://www.html5rocks.com/en/tutorials/security/sandboxed-iframes/
return (
// eslint-disable-next-line jsx-a11y/iframe-has-title -- TODO(LEMS-2871): Address a11y error
<iframe
sandbox={sandboxProperties}
style={style}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/mock-widgets/mock-widget.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ class MockWidgetComponent extends React.Component<Props> implements Widget {
render(): React.ReactNode {
return (
<View style={styles.widgetContainer}>
{/* eslint-disable-next-line jsx-a11y/no-redundant-roles -- TODO(LEMS-2871): Address a11y error */}
<TextField
ref={(ref) => (this.inputRef = ref)}
aria-label="Mock Widget"
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/orderer/orderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ class Card extends React.Component<CardProps, CardState> {
const onMouseDown = this.props.animating ? $.noop : this.onMouseDown;

return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions -- TODO(LEMS-2871): Address a11y error
<div
className={"card-wrap"}
style={style}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class PythonProgram extends React.Component<Props> implements Widget {
// http://www.html5rocks.com/en/tutorials/security/sandboxed-iframes/
return (
<View style={styles.container}>
{/* eslint-disable-next-line jsx-a11y/iframe-has-title -- TODO(LEMS-2871): Address a11y error */}
<iframe
sandbox={sandboxOptions}
src={url}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/radio/base-radio.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ const BaseRadio = function ({
// somehow break something happening inside a choice's
// child Renderers, by changing when we mount/unmount?
return (
// eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions -- TODO(LEMS-2871): Address a11y error
<li
key={i}
ref={(e) => (listElem = e)}
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/video/video.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ class Video extends React.Component<Props> implements Widget {
{this.context.strings.videoWrapper}
</View>

{/* eslint-disable-next-line jsx-a11y/iframe-has-title -- TODO(LEMS-2871): Address a11y error */}
<iframe
// TODO(joshuan): Consider not using iframes when we're
// loading this from webapp. This iframe is problematic
Expand Down
Loading

0 comments on commit dc99898

Please sign in to comment.