From fb1fd59b9d8c9c91fd641af8a7a4ee485c05ba5e Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Wed, 28 Jun 2023 09:40:59 -0400 Subject: [PATCH] [table] fix(Draggable): defensive checks for invalid child target refs --- .../components/editable-text/editableText.tsx | 2 +- packages/table/src/headers/editableName.tsx | 122 +++++++++--------- packages/table/src/interactions/draggable.tsx | 23 +++- 3 files changed, 83 insertions(+), 64 deletions(-) diff --git a/packages/core/src/components/editable-text/editableText.tsx b/packages/core/src/components/editable-text/editableText.tsx index 7ca005acab..65204b9870 100644 --- a/packages/core/src/components/editable-text/editableText.tsx +++ b/packages/core/src/components/editable-text/editableText.tsx @@ -61,7 +61,7 @@ export interface EditableTextProps extends IntentProps, Props { * N.B. this may be renamed to simply `ref` in a future major version of Blueprint, when this class component is * refactored into a function. */ - elementRef?: React.RefObject; + elementRef?: React.Ref; /** Whether the component is currently being edited. */ isEditing?: boolean; diff --git a/packages/table/src/headers/editableName.tsx b/packages/table/src/headers/editableName.tsx index e219bc18ee..df0fac88df 100644 --- a/packages/table/src/headers/editableName.tsx +++ b/packages/table/src/headers/editableName.tsx @@ -17,11 +17,11 @@ import classNames from "classnames"; import * as React from "react"; -import { EditableText, IntentProps, Props } from "@blueprintjs/core"; +import { DISPLAYNAME_PREFIX, EditableText, IntentProps, Props } from "@blueprintjs/core"; import * as Classes from "../common/classes"; -export interface EditableNameProps extends IntentProps, Props { +export interface EditableNameProps extends IntentProps, Props, React.RefAttributes { /** * The name displayed in the text box. Be sure to update this value when * rendering this component after a confirmed change. @@ -65,60 +65,64 @@ export interface EditableNameState { * * @see https://blueprintjs.com/docs/#table/api.editablename */ -export class EditableName extends React.PureComponent { - public constructor(props: EditableNameProps) { - super(props); - this.state = { - dirtyName: props.name, - isEditing: false, - savedName: props.name, - }; - } - - public componentDidUpdate(prevProps: EditableNameProps) { - const { name } = this.props; - if (name !== prevProps.name) { - this.setState({ savedName: name, dirtyName: name }); - } - } - - public render() { - const { className, intent, name } = this.props; - const { isEditing, dirtyName, savedName } = this.state; - return ( - - ); - } - - private handleEdit = () => { - this.setState({ isEditing: true, dirtyName: this.state.savedName }); - }; - - private handleCancel = (value: string) => { - // don't strictly need to clear the dirtyName, but it's better hygiene - this.setState({ isEditing: false, dirtyName: undefined }); - this.props.onCancel?.(value, this.props.index); - }; - - private handleChange = (value: string) => { - this.setState({ dirtyName: value }); - this.props.onChange?.(value, this.props.index); - }; - - private handleConfirm = (value: string) => { - this.setState({ isEditing: false, savedName: value, dirtyName: undefined }); - this.props.onConfirm?.(value, this.props.index); - }; -} +export const EditableName: React.FC = React.forwardRef((props, ref) => { + const [dirtyName, setDirtyName] = React.useState(props.name); + const [isEditing, setIsEditing] = React.useState(false); + const [savedName, setSavedName] = React.useState(props.name); + + React.useEffect(() => { + setDirtyName(props.name); + setSavedName(props.name); + }, [props.name]); + + const handleEdit = React.useCallback(() => { + setIsEditing(true); + setDirtyName(savedName); + }, [savedName]); + + const handleCancel = React.useCallback( + (value: string) => { + // don't strictly need to clear the dirtyName, but it's better hygiene + setIsEditing(false); + setDirtyName(undefined); + props.onCancel?.(value, props.index); + }, + [props.onCancel, props.index], + ); + + const handleChange = React.useCallback( + (value: string) => { + setDirtyName(value); + props.onChange?.(value, props.index); + }, + [props.onChange, props.index], + ); + + const handleConfirm = React.useCallback( + (value: string) => { + setIsEditing(false); + setSavedName(value); + setDirtyName(undefined); + props.onConfirm?.(value, props.index); + }, + [props.onConfirm, props.index], + ); + + return ( + + ); +}); +EditableName.displayName = `${DISPLAYNAME_PREFIX}.EditableName`; diff --git a/packages/table/src/interactions/draggable.tsx b/packages/table/src/interactions/draggable.tsx index da5a995cdc..aac5a1e841 100644 --- a/packages/table/src/interactions/draggable.tsx +++ b/packages/table/src/interactions/draggable.tsx @@ -74,19 +74,34 @@ export class Draggable extends React.PureComponent { } public componentDidUpdate(prevProps: DraggableProps) { - const propsWhitelist = { include: REATTACH_PROPS_KEYS }; - if (!CoreUtils.shallowCompareKeys(prevProps, this.props, propsWhitelist) && this.targetRef.current != null) { + const didEventHandlerPropsChange = !CoreUtils.shallowCompareKeys(prevProps, this.props, { + include: REATTACH_PROPS_KEYS, + }); + if (didEventHandlerPropsChange && isValidTarget(this.targetRef.current)) { this.events.attach(this.targetRef.current, this.props); } } public componentDidMount() { - if (this.targetRef.current != null) { + if (isValidTarget(this.targetRef.current)) { this.events.attach(this.targetRef.current, this.props); } } public componentWillUnmount() { - this.events?.detach(); + if (isValidTarget(this.targetRef.current)) { + this.events?.detach(); + } } } + +/** + * Used to ensure that target elements are valid at runtime for use by DragEvents. + * This check is done at runtime instead of compile time because of our use of `React.cloneElement()` + * and the associated untyped `ref` injection. + * + * @see https://github.com/palantir/blueprint/issues/6248 + */ +function isValidTarget(refElement: React.RefObject["current"]): refElement is HTMLElement { + return refElement != null && refElement instanceof HTMLElement; +}