diff --git a/.changeset/lovely-bugs-divide.md b/.changeset/lovely-bugs-divide.md new file mode 100644 index 00000000000..df74d575dbf --- /dev/null +++ b/.changeset/lovely-bugs-divide.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus": minor +--- + +Remove unused code and usages of findDOMNode diff --git a/packages/perseus/src/__tests__/renderer-api.test.tsx b/packages/perseus/src/__tests__/renderer-api.test.tsx index daa0667cfe1..3aa71af73cf 100644 --- a/packages/perseus/src/__tests__/renderer-api.test.tsx +++ b/packages/perseus/src/__tests__/renderer-api.test.tsx @@ -1,9 +1,7 @@ import {describe, beforeEach, it} from "@jest/globals"; import {act, render, screen} from "@testing-library/react"; import {userEvent as userEventLib} from "@testing-library/user-event"; -import $ from "jquery"; import * as React from "react"; -import ReactDOM from "react-dom"; import _ from "underscore"; import {testDependencies} from "../../../../testing/test-dependencies"; @@ -52,34 +50,6 @@ describe("Perseus API", function () { }); }); - describe("getDOMNodeForPath", function () { - it("should find one DOM node per ", function () { - const {renderer} = renderQuestion(mockWidget2Item.question); - const inputPaths = renderer.getInputPaths(); - - const allInputs = screen.queryAllByRole("textbox"); - - expect(inputPaths).toHaveLength(allInputs.length); - }); - - it("should find the right DOM nodes for the s", function () { - const {renderer} = renderQuestion(mockWidget2Item.question); - const inputPaths = renderer.getInputPaths(); - - const allInputs = screen.queryAllByRole("textbox"); - - inputPaths.forEach((inputPath, i) => { - // @ts-expect-error - TS2769 - No overload matches this call. - const $node = $(renderer.getDOMNodeForPath(inputPath)); - // @ts-expect-error - TS2769 - No overload matches this call. - const $input = $(ReactDOM.findDOMNode(allInputs[i])); - // @ts-expect-error - TS2339 - Property 'closest' does not exist on type 'JQueryStatic'. - // eslint-disable-next-line testing-library/no-node-access - expect($input.closest($node).length).toBeTruthy(); - }); - }); - }); - describe("CSS ClassNames", function () { describe("perseus-focused", function () { it("should be on a mock-widget exactly when focused", async function () { diff --git a/packages/perseus/src/__tests__/renderer.test.tsx b/packages/perseus/src/__tests__/renderer.test.tsx index 429aa41eafc..7ec2a6e1ef0 100644 --- a/packages/perseus/src/__tests__/renderer.test.tsx +++ b/packages/perseus/src/__tests__/renderer.test.tsx @@ -4,7 +4,7 @@ import { generateTestPerseusItem, splitPerseusItem, } from "@khanacademy/perseus-core"; -import {act, screen, waitFor, within} from "@testing-library/react"; +import {act, screen, waitFor} from "@testing-library/react"; import {userEvent as userEventLib} from "@testing-library/user-event"; import * as React from "react"; @@ -1478,34 +1478,6 @@ describe("renderer", () => { }); }); - describe("getDOMNodeForPath", () => { - it("should return the DOM node for the widget at requested FocusPath", () => { - // Arrange - const {renderer} = renderQuestion(question1); - - // Act - const node = renderer.getDOMNodeForPath(["dropdown 1"]); - - // Assert - // @ts-expect-error - TS2345 - Argument of type 'Element | Text | null | undefined' is not assignable to parameter of type 'HTMLElement'. - expect(within(node).queryAllByRole("combobox")).toHaveLength(1); - }); - - it("should return the widget's getDOMNodeForPath() result for the widget at requested FocusPath", () => { - // Arrange - const {renderer} = renderQuestion(definitionItem); - const widget2DOMNode = ; - const [widget2] = renderer.findWidgets("definition 2"); - widget2.getDOMNodeForPath = jest.fn(() => widget2DOMNode); - - // Act - const node = renderer.getDOMNodeForPath(["definition 2"]); - - // Assert - expect(node).toBe(widget2DOMNode); - }); - }); - describe("getInputPaths", () => { it("should return all input paths for all rendererd widgets", () => { // Arrange diff --git a/packages/perseus/src/__tests__/server-item-renderer.test.tsx b/packages/perseus/src/__tests__/server-item-renderer.test.tsx index 6fa3d8f16bc..ae4ac198c89 100644 --- a/packages/perseus/src/__tests__/server-item-renderer.test.tsx +++ b/packages/perseus/src/__tests__/server-item-renderer.test.tsx @@ -1,5 +1,5 @@ import {RenderStateRoot} from "@khanacademy/wonder-blocks-core"; -import {within, render, screen, act} from "@testing-library/react"; +import {render, screen, act} from "@testing-library/react"; import {userEvent as userEventLib} from "@testing-library/user-event"; import * as React from "react"; @@ -108,18 +108,6 @@ describe("server item renderer", () => { }); }); - it("should return the DOM node for the requested focus path", async () => { - // Arrange - const {renderer} = renderQuestion(itemWithMockWidget); - - // Act - const node = renderer.getDOMNodeForPath(["mock-widget 1"]); - - // Assert - // @ts-expect-error - TS2345 - Argument of type 'Element | Text | null | undefined' is not assignable to parameter of type 'HTMLElement'. - expect(await within(node).findAllByRole("textbox")).toHaveLength(1); - }); - it("should return the number of hints available", () => { // Arrange const {renderer} = renderQuestion({ @@ -304,7 +292,6 @@ describe("server item renderer", () => { ["mock-widget 1"], null, 0, - expect.any(Object), ); }); @@ -349,7 +336,6 @@ describe("server item renderer", () => { ["numeric-input 1"], null, 250, - expect.any(Object), ); }); @@ -373,7 +359,6 @@ describe("server item renderer", () => { null, ["mock-widget 1"], 0, - null, ); }); @@ -419,7 +404,6 @@ describe("server item renderer", () => { null, ["numeric-input 1"], 0, - null, ); }); @@ -441,7 +425,6 @@ describe("server item renderer", () => { ["mock-widget 1"], null, 0, - expect.any(Object), ); }); }); diff --git a/packages/perseus/src/article-renderer.tsx b/packages/perseus/src/article-renderer.tsx index 30d9197bf5f..9b93967e911 100644 --- a/packages/perseus/src/article-renderer.tsx +++ b/packages/perseus/src/article-renderer.tsx @@ -94,7 +94,6 @@ class ArticleRenderer // paths, so as to check whether the focused path represents an // input. let didFocusInput = false; - let focusedInput; if (this._currentFocus) { const [sectionIndex, ...focusPath] = this._currentFocus; @@ -105,10 +104,6 @@ class ArticleRenderer didFocusInput = inputPaths.some((inputPath) => { return Util.inputPathsEqual(inputPath, focusPath); }); - focusedInput = - this.sectionRenderers[sectionIndex].getDOMNodeForPath( - focusPath, - ); } const {onFocusChange} = this.props.apiOptions; @@ -123,12 +118,7 @@ class ArticleRenderer ? keypadDomNode.getBoundingClientRect().height : 0; - onFocusChange( - this._currentFocus, - prevFocusPath, - keypadHeight, - didFocusInput ? focusedInput : null, - ); + onFocusChange(this._currentFocus, prevFocusPath, keypadHeight); }, 0); } diff --git a/packages/perseus/src/components/simple-keypad-input.tsx b/packages/perseus/src/components/simple-keypad-input.tsx index 3b109a04c93..073cb9e55b2 100644 --- a/packages/perseus/src/components/simple-keypad-input.tsx +++ b/packages/perseus/src/components/simple-keypad-input.tsx @@ -15,78 +15,78 @@ import * as React from "react"; import type {Focusable} from "../types"; -export default class SimpleKeypadInput - extends React.Component - implements Focusable -{ - static contextType = KeypadContext; - declare context: React.ContextType; - _isMounted = false; - inputRef = React.createRef(); +type SimpleKeypadInputProps = { + keypadElement?: any; + onFocus: () => void; + onBlur: () => void; + onChange: (value: string, callback: any) => void; + value?: string | number | null; + ariaLabel?: string; + style?: React.CSSProperties; +}; - componentDidMount() { - // TODO(scottgrant): This is a hack to remove the deprecated call to - // this.isMounted() but is still considered an anti-pattern. - this._isMounted = true; - } +const SimpleKeypadInput = React.forwardRef( + function SimpleKeypadInput(props, ref) { + const keypadInputRef = React.useRef(null); + const context = React.useContext(KeypadContext); - componentWillUnmount() { - this._isMounted = false; - } + // Use imperative handle to expose the DOM node properties and custom methods + // required for consuming Perseus widgets to handle focus/blur events. + React.useImperativeHandle(ref, () => { + const keypadInstance = keypadInputRef.current; + if (!keypadInstance) { + return null; + } - focus() { - // The inputRef is a ref to a MathInput, which - // also controls the keypad state during focus events. - this.inputRef.current?.focus(this.context.setKeypadActive); - } + // Get the actual DOM node from the KeypadInput (MathInput) component's + // internal inputRef. + const inputElement = keypadInstance.inputRef; - blur() { - this.inputRef.current?.blur(); - } + if (!inputElement) { + return null; + } - getValue(): string | number { - return this.props.value; - } + // Return the DOM node with focus/blur methods attached. + return { + ...inputElement, + focus: () => { + keypadInstance.focus(context.setKeypadActive); + }, + blur: () => { + keypadInstance.blur(); + }, + getValue: () => { + return props.value; + }, + }; + }); - render(): React.ReactNode { - const _this = this; - // Intercept the `onFocus` prop, as we need to configure the keypad - // before continuing with the default focus logic for Perseus inputs. - // Intercept the `value` prop so as to map `null` to the empty string, - // as the `KeypadInput` does not support `null` values. - const {keypadElement, onFocus, value, ...rest} = _this.props; + const {keypadElement, onFocus, value, ...rest} = props; return ( - // @ts-expect-error - TS2769 - No overload matches this call. { if (keypadElement) { - keypadElement.configure( - { - keypadType: "FRACTION", - }, - () => { - if (_this._isMounted) { - onFocus?.(); - } - }, - ); - } else { - onFocus?.(); + keypadElement.configure({ + keypadType: "FRACTION", + }); } + onFocus?.(); }} value={value == null ? "" : "" + value} + ariaLabel={props.ariaLabel || ""} {...rest} /> ); - } -} + }, +); -// @ts-expect-error - TS2339 - Property 'propTypes' does not exist on type 'typeof SimpleKeypadInput'. SimpleKeypadInput.propTypes = { keypadElement: keypadElementPropType, onFocus: PropTypes.func, value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]), }; + +export default SimpleKeypadInput; diff --git a/packages/perseus/src/renderer.tsx b/packages/perseus/src/renderer.tsx index b2f450c3cbd..fe2dbb32069 100644 --- a/packages/perseus/src/renderer.tsx +++ b/packages/perseus/src/renderer.tsx @@ -17,9 +17,7 @@ import { } from "@khanacademy/perseus-score"; import {entries} from "@khanacademy/wonder-stuff-core"; import classNames from "classnames"; -import $ from "jquery"; import * as React from "react"; -import ReactDOM from "react-dom"; import _ from "underscore"; import AssetContext from "./asset-context"; @@ -311,9 +309,7 @@ class Renderer componentDidMount() { this._isMounted = true; - // figure out why we're passing an empty object - // @ts-expect-error - TS2345 - Argument of type '{}' is not assignable to parameter of type 'Props'. - this.handleRender({}); + this.props.onRender(); this._currentFocus = null; this.props.initializeUserInput?.( @@ -423,7 +419,6 @@ class Renderer } componentDidUpdate(prevProps: Props, prevState: State) { - this.handleRender(prevProps); // We even do this if we did reuse the markdown because // we might need to update the widget props on this render, // even though we have the same widgets. @@ -1421,29 +1416,6 @@ class Renderer ); }; - handleRender: (prevProps: Props) => void = (prevProps: Props) => { - const onRender = this.props.onRender; - const oldOnRender = prevProps.onRender; - - // In the common case of no callback specified, avoid this work. - if (onRender !== noopOnRender || oldOnRender !== noopOnRender) { - // @ts-expect-error - TS2769 - No overload matches this call. | TS2339 - Property 'find' does not exist on type 'JQueryStatic'. - const $images = $(ReactDOM.findDOMNode(this)).find("img"); - - // Fire callback on image load... - if (oldOnRender !== noopOnRender) { - $images.off("load", oldOnRender); - } - - if (onRender !== noopOnRender) { - $images.on("load", onRender); - } - } - - // ...as well as right now (non-image, non-TeX or image from cache) - onRender(); - }; - // Sets the current focus path // If the new focus path is not a prefix of the old focus path, // we send an onChangeFocus event back to our parent. @@ -1519,26 +1491,6 @@ class Renderer } }; - getDOMNodeForPath: (path: FocusPath) => Element | Text | null | undefined = - (path: FocusPath) => { - // @ts-expect-error - TS2345 - Argument of type 'FocusPath' is not assignable to parameter of type 'List'. - const widgetId = _.first(path); - // @ts-expect-error - TS2345 - Argument of type 'FocusPath' is not assignable to parameter of type 'List'. - const interWidgetPath = _.rest(path); - - // Widget handles parsing of the interWidgetPath. If the path is empty - // beyond the widgetID, as a special case we just return the widget's - // DOM node. - const widget = this.getWidgetInstance(widgetId); - if (widget?.getDOMNodeForPath) { - return widget.getDOMNodeForPath(interWidgetPath); - } - if (interWidgetPath.length === 0) { - // @ts-expect-error - TS2345 - Argument of type 'Widget | null | undefined' is not assignable to parameter of type 'ReactInstance | null | undefined'. - return ReactDOM.findDOMNode(widget); - } - }; - getInputPaths: () => ReadonlyArray = () => { const inputPaths: Array = []; this.widgetIds.forEach((widgetId: string) => { diff --git a/packages/perseus/src/server-item-renderer.tsx b/packages/perseus/src/server-item-renderer.tsx index 87422d83a07..2076e7ba4dc 100644 --- a/packages/perseus/src/server-item-renderer.tsx +++ b/packages/perseus/src/server-item-renderer.tsx @@ -243,14 +243,7 @@ export class ServerItemRenderer : 0; // Then call the callback - onFocusChange( - this._currentFocus, - prevFocus, - keypadHeight, - // @ts-expect-error [FEI-5003] - TS2345 - Argument of type 'false | Element | Text | null | undefined' is not assignable to parameter of type 'HTMLElement | undefined'. - didFocusInput && - this.questionRenderer.getDOMNodeForPath(newFocus), - ); + onFocusChange(this._currentFocus, prevFocus, keypadHeight); }, 0); } @@ -304,10 +297,6 @@ export class ServerItemRenderer return this.questionRenderer.blurPath(path); } - getDOMNodeForPath(path: FocusPath): Element | Text | null | undefined { - return this.questionRenderer.getDOMNodeForPath(path); - } - getInputPaths(): ReadonlyArray { const questionAreaInputPaths = this.questionRenderer.getInputPaths(); return questionAreaInputPaths; diff --git a/packages/perseus/src/types.ts b/packages/perseus/src/types.ts index b79cfc09118..78e160aaa72 100644 --- a/packages/perseus/src/types.ts +++ b/packages/perseus/src/types.ts @@ -71,7 +71,6 @@ export interface Widget { path: FocusPath; } | boolean; - getDOMNodeForPath?: (path: FocusPath) => Element | Text | null; /** * Returns widget state that can be passed back to `restoreSerializedState` @@ -179,7 +178,6 @@ export type APIOptions = Readonly<{ newFocusPath: FocusPath, oldFocusPath: FocusPath, keypadHeight?: number, - focusedElement?: HTMLElement, ) => unknown; /** * @deprecated - metadata is no longer used by the Group widget diff --git a/packages/perseus/src/widgets/matrix/matrix.tsx b/packages/perseus/src/widgets/matrix/matrix.tsx index d38e7886ae4..e294f1967af 100644 --- a/packages/perseus/src/widgets/matrix/matrix.tsx +++ b/packages/perseus/src/widgets/matrix/matrix.tsx @@ -3,7 +3,6 @@ import {linterContextDefault} from "@khanacademy/perseus-linter"; import {StyleSheet} from "aphrodite"; import classNames from "classnames"; import * as React from "react"; -import ReactDOM from "react-dom"; import _ from "underscore"; import {PerseusI18nContext} from "../../components/i18n-context"; @@ -115,6 +114,7 @@ type State = { class Matrix extends React.Component implements Widget { static contextType = PerseusI18nContext; declare context: React.ContextType; + answerRefs: Record = {}; // @ts-expect-error - TS2564 - Property 'cursorPosition' has no initializer and is not definitely assigned in the constructor. cursorPosition: [number, number]; @@ -170,9 +170,8 @@ class Matrix extends React.Component implements Widget { focusInputPath: (arg1: any) => void = (path) => { const inputID = getRefForPath(path); - // eslint-disable-next-line react/no-string-refs - // @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. - this.refs[inputID].focus(); + const inputComponent = this.answerRefs[inputID]; + inputComponent.focus(); }; blurInputPath: (arg1: any) => void = (path) => { @@ -181,17 +180,10 @@ class Matrix extends React.Component implements Widget { } const inputID = getRefForPath(path); - // eslint-disable-next-line react/no-string-refs - // @ts-expect-error - TS2339 - Property 'blur' does not exist on type 'ReactInstance'. - this.refs[inputID].blur(); + const inputComponent = this.answerRefs[inputID]; + inputComponent.blur(); }; - getDOMNodeForPath(path: FocusPath) { - const inputID = getRefForPath(path); - // eslint-disable-next-line react/no-string-refs - return ReactDOM.findDOMNode(this.refs[inputID]); - } - handleKeyDown: (arg1: any, arg2: any, arg3: any) => void = ( row, col, @@ -201,8 +193,8 @@ class Matrix extends React.Component implements Widget { const maxCol = this.props.matrixBoardSize[1]; let enterTheMatrix = null; - // eslint-disable-next-line react/no-string-refs - const curInput = this.refs[getRefForPath(getInputPath(row, col))]; + const inputID = getRefForPath(getInputPath(row, col)); + const curInput = this.answerRefs[inputID]; // @ts-expect-error - TS2339 - Property 'getStringValue' does not exist on type 'ReactInstance'. const curValueString = curInput.getStringValue(); // @ts-expect-error - TS2339 - Property 'getSelectionStart' does not exist on type 'ReactInstance'. @@ -243,8 +235,8 @@ class Matrix extends React.Component implements Widget { e.preventDefault(); // Focus the input and move the cursor to the end of it. - // eslint-disable-next-line react/no-string-refs - const input = this.refs[getRefForPath(nextPath)]; + const inputID = getRefForPath(nextPath); + const input = this.answerRefs[inputID]; // Multiply by 2 to ensure the cursor always ends up at the end; // Opera sometimes sees a carriage return as 2 characters. @@ -252,13 +244,10 @@ class Matrix extends React.Component implements Widget { const inputValString = input.getStringValue(); const valueLength = inputValString.length * 2; - // @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. input.focus(); if (e.key === "ArrowRight") { - // @ts-expect-error - TS2339 - Property 'setSelectionRange' does not exist on type 'ReactInstance'. input.setSelectionRange(0, 0); } else { - // @ts-expect-error - TS2339 - Property 'setSelectionRange' does not exist on type 'ReactInstance'. input.setSelectionRange(valueLength, valueLength); } } @@ -377,9 +366,13 @@ class Matrix extends React.Component implements Widget { className: outside ? "outside" : "inside", - ref: getRefForPath( - getInputPath(row, col), - ), + ref: (ref) => { + this.answerRefs[ + getRefForPath( + getInputPath(row, col), + ) + ] = ref; + }, // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions value: rowVals ? rowVals[col] : null, style: { @@ -462,7 +455,6 @@ class Matrix extends React.Component implements Widget { implements Widget { static contextType = PerseusI18nContext; declare context: React.ContextType; + tickControlRef: HTMLInputElement | null = null; + static defaultProps: DefaultProps = { range: [0, 10], labelStyle: "decimal", @@ -346,9 +346,7 @@ class NumberLine extends React.Component implements Widget { focus() { if (this.props.isTickCtrl) { - // eslint-disable-next-line react/no-string-refs - // @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. - this.refs["tick-ctrl"].focus(); + this.tickControlRef?.focus(); return true; } return false; @@ -356,20 +354,18 @@ class NumberLine extends React.Component implements Widget { focusInputPath: (arg1: any) => void = (path) => { if (path.length === 1) { - // eslint-disable-next-line react/no-string-refs - // @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. - this.refs[path[0]].focus(); + this.tickControlRef?.focus(); } }; blurInputPath: (arg1: any) => void = (path) => { if (path.length === 1) { - // eslint-disable-next-line react/no-string-refs - // @ts-expect-error - TS2339 - Property 'blur' does not exist on type 'ReactInstance'. - this.refs[path[0]].blur(); + this.tickControlRef?.blur(); } }; + // There's only one input path for the tick control, but the renderer + // expects this method to be implemented. getInputPaths: () => ReadonlyArray> = () => { if (this.props.isTickCtrl) { return [["tick-ctrl"]]; @@ -377,14 +373,6 @@ class NumberLine extends React.Component implements Widget { return []; }; - getDOMNodeForPath(inputPath: FocusPath) { - if (inputPath?.length === 1) { - // eslint-disable-next-line react/no-string-refs - return ReactDOM.findDOMNode(this.refs[inputPath[0]]); - } - return null; - } - _renderGraphie: () => React.ReactElement = () => { // Position variables const range = this.props.range; @@ -675,12 +663,16 @@ class NumberLine extends React.Component implements Widget { } else { Input = NumberInput; } + // TODO: keypadElement is only needed for SimpleKeypadInput and causes a React warning + // when passed to NumberInput. This should be refactored to only pass the prop when needed, + // but requires addressing type incompatibilities in the onChange handlers. tickCtrl = (