From 794689c1e942f2426386ab1e6aed3918ef54f549 Mon Sep 17 00:00:00 2001 From: Paul Carduner Date: Sat, 20 Nov 2021 04:55:33 -0800 Subject: [PATCH] Focus the currently focused node when it mounts (#593) * Add failing test for focus after node insertion * Redo the way ToplevelBlockEditable works to be more friendly * Focus the currently focused node as soon as it mounts * Only focus a node on mount if the editor has focus * Add tests for focusing after a top-level deletion * Add tests for focusing after deleting a child node --- packages/cmb-toolkit/src/simulate.ts | 4 +- .../codemirror-blocks/spec/activation-test.ts | 77 +++++- .../codemirror-blocks/src/components/Node.tsx | 19 ++ .../src/components/NodeEditable.tsx | 258 ++++++++++-------- packages/codemirror-blocks/src/editor.ts | 8 + .../codemirror-blocks/src/state/actions.ts | 2 +- .../src/ui/ToplevelBlockEditable.tsx | 45 +-- 7 files changed, 273 insertions(+), 140 deletions(-) diff --git a/packages/cmb-toolkit/src/simulate.ts b/packages/cmb-toolkit/src/simulate.ts index f2845c8c0..31e44e0b1 100644 --- a/packages/cmb-toolkit/src/simulate.ts +++ b/packages/cmb-toolkit/src/simulate.ts @@ -143,7 +143,7 @@ export function insertText(text: string) { // See https://github.com/testing-library/dom-testing-library/pull/235 // for some discussion about this. fireEvent.input(activeEl, { - target: { innerHTML: text }, + target: { innerHTML: activeEl.innerHTML + text }, }); } } @@ -171,7 +171,7 @@ function makeKeyEvent>( // https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values function getKeyCode(key: string): number { // The key code for an (uppercase) letter is that letter's ascii value. - if (key.match(/^[A-Z]$/)) { + if (key.match(/^[A-Za-z0-9]$/)) { return key.charCodeAt(0); } // The key code for a digit is that digit's ascii value. diff --git a/packages/codemirror-blocks/spec/activation-test.ts b/packages/codemirror-blocks/spec/activation-test.ts index 50a03abff..19d2e4ae8 100644 --- a/packages/codemirror-blocks/spec/activation-test.ts +++ b/packages/codemirror-blocks/spec/activation-test.ts @@ -1,6 +1,5 @@ import wescheme from "../src/languages/wescheme"; -import "codemirror/addon/search/searchcursor.js"; - +import { screen } from "@testing-library/react"; import { cmd_ctrl, teardown, @@ -10,6 +9,8 @@ import { mountCMB, isNodeEditable, elementForNode, + keyPress, + click, } from "../src/toolkit/test-utils"; import { API } from "../src/CodeMirrorBlocks"; import { ASTNode } from "../src/ast"; @@ -131,6 +132,78 @@ describe("when dealing with node activation,", () => { }); }); +describe("inserting a new node", () => { + let cmb: API; + beforeEach(() => { + cmb = mountCMB(wescheme).cmb; + }); + it("should focus the node that was just inserted", () => { + cmb.focus(); + // start inserting a new node + keyPress("a"); + insertText("BrandNewLiteral"); + // confirm the insertion by pressing Enter + keyDown("Enter"); + // confirm that the new new was saved to the ast and focused + expect(cmb.getAst().toString()).toBe("aBrandNewLiteral"); + expect(cmb.getValue()).toEqual("aBrandNewLiteral"); + expect( + screen.getByRole("treeitem", { name: /aBrandNewLiteral/ }) + ).toHaveFocus(); + }); +}); + +describe("switching to block mode", () => { + let cmb: API; + beforeEach(() => { + cmb = mountCMB(wescheme).cmb; + cmb.setBlockMode(false); + cmb.setValue("foo bar"); + }); + + it("should not change the focused element", () => { + const blockModeBtn = screen.getByRole("button", { + name: /Switch to blocks mode/, + }); + blockModeBtn.focus(); + expect(blockModeBtn).toHaveFocus(); + click(blockModeBtn); + expect(blockModeBtn).toHaveFocus(); + }); +}); + +describe("deleting a node", () => { + let cmb: API; + beforeEach(() => { + cmb = mountCMB(wescheme).cmb; + }); + it("should focus on the previous node when a top-level node is deleted", () => { + cmb.setValue("firstLiteral\nsecondLiteral\nthirdLiteral"); + mouseDown(screen.getByRole("treeitem", { name: /thirdLiteral/ })); + keyDown(" "); + keyDown("Delete"); + expect(cmb.getValue()).toBe("firstLiteral\nsecondLiteral\n"); + expect( + screen.getByRole("treeitem", { name: /secondLiteral/ }) + ).toHaveFocus(); + expect(cmb.getCursor()).toMatchInlineSnapshot(` + Object { + "ch": 0, + "line": 1, + } + `); + }); + + it("should focus on the previous child node when a child node is deleted", () => { + cmb.setValue("(someFunc firstArg secondArg)"); + mouseDown(screen.getByRole("treeitem", { name: /secondArg/ })); + keyDown(" "); + keyDown("Delete"); + expect(cmb.getValue()).toBe("(someFunc firstArg)"); + expect(screen.getByRole("treeitem", { name: /firstArg/ })).toHaveFocus(); + }); +}); + describe("cut/copy/paste", () => { let cmb!: API; let literal1!: ASTNode; diff --git a/packages/codemirror-blocks/src/components/Node.tsx b/packages/codemirror-blocks/src/components/Node.tsx index 3b4cfa364..98d5631c2 100644 --- a/packages/codemirror-blocks/src/components/Node.tsx +++ b/packages/codemirror-blocks/src/components/Node.tsx @@ -240,6 +240,25 @@ const Node = ({ expandable = true, ...props }: Props) => { } }, [props.node.id]); + // If this is the currently focused node, then focus the node element + // as soon as we finish rendering. + const focusedNode = useSelector(selectors.getFocusedNode); + useEffect(() => { + if ( + !editor?.hasFocus() && + document.activeElement !== document.body && + document.activeElement !== null && + document.activeElement.id !== "SR_fix_for_slow_dom" + ) { + // If the editor doesn't have focus, then we don't want to steal + // focus from something else. + return; + } + if (nodeElementRef.current && focusedNode?.id === props.node.id) { + nodeElementRef.current?.focus(); + } + }, [editor, focusedNode?.id, props.node.id]); + if (editable) { if (!editor) { throw new Error("can't edit nodes before codemirror has mounted"); diff --git a/packages/codemirror-blocks/src/components/NodeEditable.tsx b/packages/codemirror-blocks/src/components/NodeEditable.tsx index 510e50d9d..20f16e5f5 100644 --- a/packages/codemirror-blocks/src/components/NodeEditable.tsx +++ b/packages/codemirror-blocks/src/components/NodeEditable.tsx @@ -52,127 +52,151 @@ type Props = Omit & { editor: CMBEditor; }; -const NodeEditable = (props: Props) => { - const element = useRef(null); - const dispatch: AppDispatch = useDispatch(); - - const ast = useSelector(selectors.getAST); - - const { initialValue, isErrored } = useSelector((state: RootState) => { - const nodeId = props.target.node ? props.target.node.id : "editing"; - const isErrored = selectors.getErrorId(state) == nodeId; - - const initialValue = - props.value === null ? props.target.getText(ast, props.editor) : ""; - - return { isErrored, initialValue }; - }); - - // select and focus the element on mount - useEffect(() => { - const currentEl = element.current; - if (!currentEl) { - // element has been unmounted already, nothing to do. - return; - } - selectElement(currentEl, props.isInsertion); - }, [props.isInsertion]); - - useEffect(() => { - const text = props.value || initialValue || ""; - const annt = (props.isInsertion ? "inserting" : "editing") + ` ${text}`; - say(annt + `. Use Enter to save, and Alt-Q to cancel`); - dispatch(actions.setSelectedNodeIds([])); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); - - const onBlur = (e: React.SyntheticEvent) => { - e.stopPropagation(); - const { target } = props; - dispatch((dispatch: AppDispatch, getState: () => RootState) => { - // we grab the value directly from the content editable element - // to deal with this issue: - // https://github.com/lovasoa/react-contenteditable/issues/161 - const value = element.current?.textContent; - const focusedNode = selectors.getFocusedNode(getState()); - // if there's no insertion value, or the new value is the same as the - // old one, preserve focus on original node and return silently - if (value === initialValue || !value) { - props.onDisableEditable(); - const nid = focusedNode && focusedNode.nid; - dispatch(activateByNid(props.editor, nid)); +export type NodeEditableHandle = { + /** + * Selects and focuses the text in the node editable component + */ + select: () => void; +}; + +const NodeEditable = React.forwardRef( + (props, ref) => { + const element = useRef(null); + const dispatch: AppDispatch = useDispatch(); + + const ast = useSelector(selectors.getAST); + + const { initialValue, isErrored } = useSelector((state: RootState) => { + const nodeId = props.target.node ? props.target.node.id : "editing"; + const isErrored = selectors.getErrorId(state) == nodeId; + + const initialValue = + props.value === null ? props.target.getText(ast, props.editor) : ""; + + return { isErrored, initialValue }; + }); + + // select and focus the element on mount + useEffect(() => { + const currentEl = element.current; + if (!currentEl) { + // element has been unmounted already, nothing to do. return; } + selectElement(currentEl, props.isInsertion); + }, [props.isInsertion]); - const annt = `${props.isInsertion ? "inserted" : "changed"} ${value}`; - const result = dispatch(insert(value, target, props.editor, annt)); - if (result.successful) { - dispatch(activateByNid(props.editor, null, { allowMove: false })); - props.onChange(null); - props.onDisableEditable(); - dispatch(actions.clearError()); - say(annt); - } else { - console.error(result.exception); - dispatch(actions.setErrorId(target.node ? target.node.id : "editing")); + // expose an imperative handle for selecting the contents + // of the rendered element in case it needs to be done after + // later. This is only necessary in cases where the component + // is rendered to a container that won't be in the document + // until later, as is the case with TopLevelNodeEditable + React.useImperativeHandle(ref, () => ({ + select: () => { if (element.current) { - selectElement(element.current, false); + selectElement(element.current, props.isInsertion); + } + }, + })); + + useEffect(() => { + const text = props.value || initialValue || ""; + const annt = (props.isInsertion ? "inserting" : "editing") + ` ${text}`; + say(annt + `. Use Enter to save, and Alt-Q to cancel`); + dispatch(actions.setSelectedNodeIds([])); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + const onBlur = (e: React.SyntheticEvent) => { + e.stopPropagation(); + const { target } = props; + dispatch((dispatch: AppDispatch, getState: () => RootState) => { + // we grab the value directly from the content editable element + // to deal with this issue: + // https://github.com/lovasoa/react-contenteditable/issues/161 + const value = element.current?.textContent; + const focusedNode = selectors.getFocusedNode(getState()); + // if there's no insertion value, or the new value is the same as the + // old one, preserve focus on original node and return silently + if (value === initialValue || !value) { + props.onDisableEditable(); + const nid = focusedNode && focusedNode.nid; + dispatch(activateByNid(props.editor, nid)); + return; + } + + const annt = `${props.isInsertion ? "inserted" : "changed"} ${value}`; + const result = dispatch(insert(value, target, props.editor, annt)); + if (result.successful) { + dispatch(activateByNid(props.editor, null, { allowMove: false })); + props.onChange(null); + props.onDisableEditable(); + dispatch(actions.clearError()); + say(annt); + } else { + console.error(result.exception); + dispatch( + actions.setErrorId(target.node ? target.node.id : "editing") + ); + if (element.current) { + selectElement(element.current, false); + } + } + }); + }; + + const onKeyDown = (e: React.KeyboardEvent) => { + const el = e.target as HTMLDivElement; + switch (CodeMirror.keyName(e)) { + case "Enter": { + // blur the element to trigger handleBlur + // which will save the edit + element.current?.blur(); + return; + } + case "Alt-Q": + case "Esc": { + el.innerHTML = initialValue; + e.stopPropagation(); + props.onChange(null); + props.onDisableEditable(); + dispatch(actions.clearError()); + dispatch(activateByNid(props.editor, null, { allowMove: false })); + return; } } - }); - }; - - const onKeyDown = (e: React.KeyboardEvent) => { - const el = e.target as HTMLDivElement; - switch (CodeMirror.keyName(e)) { - case "Enter": { - // blur the element to trigger handleBlur - // which will save the edit - element.current?.blur(); - return; - } - case "Alt-Q": - case "Esc": { - el.innerHTML = initialValue; - e.stopPropagation(); - props.onChange(null); - props.onDisableEditable(); - dispatch(actions.clearError()); - dispatch(activateByNid(props.editor, null, { allowMove: false })); - return; - } - } - }; - - const { contentEditableProps, extraClasses, value } = props; - - const classes = ( - [ - "blocks-literal", - "quarantine", - "blocks-editing", - "blocks-node", - { "blocks-error": isErrored }, - ] as ClassNamesArgument[] - ).concat(extraClasses); - const text = value ?? initialValue; - return ( - - ); -}; + }; + + const { contentEditableProps, extraClasses, value } = props; + + const classes = ( + [ + "blocks-literal", + "quarantine", + "blocks-editing", + "blocks-node", + { "blocks-error": isErrored }, + ] as ClassNamesArgument[] + ).concat(extraClasses); + const text = value ?? initialValue; + return ( + + ); + } +); export default NodeEditable; diff --git a/packages/codemirror-blocks/src/editor.ts b/packages/codemirror-blocks/src/editor.ts index e361086a8..1c5f4e67e 100644 --- a/packages/codemirror-blocks/src/editor.ts +++ b/packages/codemirror-blocks/src/editor.ts @@ -134,6 +134,11 @@ export interface ReadonlyCMBEditor extends ReadonlyRangedText { start: Pos, options: { caseFold: boolean } ): CodeMirror.SearchCursor; + + /** + * Whether or not the editor currently has focus + */ + hasFocus(): boolean; } export interface RangedText extends ReadonlyRangedText { @@ -231,6 +236,9 @@ export class CodeMirrorFacade implements CMBEditor { ): void { this.codemirror.replaceRange(replacement, from, to, origin); } + hasFocus(): boolean { + return this.codemirror.hasFocus(); + } focus(): void { this.codemirror.focus(); } diff --git a/packages/codemirror-blocks/src/state/actions.ts b/packages/codemirror-blocks/src/state/actions.ts index ef101a9dd..ed6978f56 100644 --- a/packages/codemirror-blocks/src/state/actions.ts +++ b/packages/codemirror-blocks/src/state/actions.ts @@ -330,7 +330,7 @@ export function activateByNid( // which confuses the screenreader. In these situations, we focus on // a dummy element that just says "stand by" (see ToggleEditor.js). // When the new node is available, focus will shift automatically. - if (!document.contains(newNode.element)) { + if (newNode.element && !document.contains(newNode.element)) { const sr = document.getElementById("SR_fix_for_slow_dom"); // In the event that everything has been unmounted, // for example in a unit test, then neither newNode.element nor diff --git a/packages/codemirror-blocks/src/ui/ToplevelBlockEditable.tsx b/packages/codemirror-blocks/src/ui/ToplevelBlockEditable.tsx index 414f65fcb..f2201a707 100644 --- a/packages/codemirror-blocks/src/ui/ToplevelBlockEditable.tsx +++ b/packages/codemirror-blocks/src/ui/ToplevelBlockEditable.tsx @@ -1,10 +1,10 @@ -import React, { useEffect, useMemo } from "react"; +import React, { useEffect, useMemo, useRef } from "react"; import ReactDOM from "react-dom"; import { useDispatch } from "react-redux"; -import NodeEditable from "../components/NodeEditable"; +import NodeEditable, { NodeEditableHandle } from "../components/NodeEditable"; import * as actions from "../state/actions"; import type { AppDispatch } from "../state/store"; -import type { CMBEditor } from "../editor"; +import { CMBEditor } from "../editor"; import { Quarantine } from "../state/reducers"; type Props = { @@ -26,28 +26,37 @@ const ToplevelBlockEditable = (props: Props) => { const onChange = (text: string) => dispatch(actions.changeQuarantine(text)); const { from, to, value } = props.quarantine; - // add a marker to codemirror, with an empty "widget" into which - // the react component will be rendered. - // We use useMemo to make sure this marker only gets added the first - // time this component is rendered. - const { container, marker } = useMemo(() => { + // create an empty container element to hold the rendered react + // component. We use useMemo to make sure this element only gets + // created the first time this component is rendered. + const { container } = useMemo(() => { const container = document.createElement("span"); container.classList.add("react-container"); - const marker = props.editor.replaceMarkerWidget(from, to, container); - // call endOperation to flush all buffered updates - // forcing codemirror to put the marker into the document's DOM - // right away, making it immediately focusable/selectable. - // SHARED.editor.endOperation(); - return { container, marker }; - }, [props.editor, from, to]); - // make sure to clear the marker from codemirror - // when the component unmounts + return { container }; + }, []); + + const nodeEditableHandle = useRef(null); + + // after react has finished rendering, attach the container to codemirror + // as a marker widget and select its contents. + // + // This has to happen after react finishes rendering because codemirror + // calls focus() when you call CodeMirror.setBookmark, which triggers + // this cryptic react warning: unstable_flushDiscreteUpdates: Cannot + // flush updates when React is already rendering + // See https://stackoverflow.com/questions/58123011/cryptic-react-error-unstable-flushdiscreteupdates-cannot-flush-updates-when-re + // and https://github.com/facebook/react/issues/20141 for more info. useEffect(() => { + const marker = props.editor.replaceMarkerWidget(from, to, container); + nodeEditableHandle.current?.select(); + // make sure to clear the marker from codemirror + // when the component unmounts return () => marker.clear(); - }, [marker]); + }, [container, props.editor, from, to]); return ReactDOM.createPortal(