From f580a334c4b9943b9d3ada28446a325e6c924628 Mon Sep 17 00:00:00 2001 From: pcardune Date: Wed, 17 Nov 2021 13:50:50 -0800 Subject: [PATCH 1/6] Add failing test for focus after node insertion --- packages/cmb-toolkit/src/simulate.ts | 4 +-- .../codemirror-blocks/spec/activation-test.ts | 25 +++++++++++++++++-- 2 files changed, 25 insertions(+), 4 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..e46372620 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,7 @@ import { mountCMB, isNodeEditable, elementForNode, + keyPress, } from "../src/toolkit/test-utils"; import { API } from "../src/CodeMirrorBlocks"; import { ASTNode } from "../src/ast"; @@ -131,6 +131,27 @@ 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(document.activeElement!.id).toEqual( + screen.getByRole("treeitem", { name: /aBrandNewLiteral/ }).id + ); + }); +}); + describe("cut/copy/paste", () => { let cmb!: API; let literal1!: ASTNode; From 593a3d75b62968a9483e5ca0fe8c97b82b374473 Mon Sep 17 00:00:00 2001 From: pcardune Date: Wed, 17 Nov 2021 14:12:27 -0800 Subject: [PATCH 2/6] Redo the way ToplevelBlockEditable works to be more friendly --- .../src/components/NodeEditable.tsx | 258 ++++++++++-------- .../src/ui/ToplevelBlockEditable.tsx | 45 +-- 2 files changed, 168 insertions(+), 135 deletions(-) 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/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( Date: Wed, 17 Nov 2021 14:13:03 -0800 Subject: [PATCH 3/6] Focus the currently focused node as soon as it mounts --- packages/codemirror-blocks/src/components/Node.tsx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/codemirror-blocks/src/components/Node.tsx b/packages/codemirror-blocks/src/components/Node.tsx index 3b4cfa364..dddb80437 100644 --- a/packages/codemirror-blocks/src/components/Node.tsx +++ b/packages/codemirror-blocks/src/components/Node.tsx @@ -240,6 +240,15 @@ 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 (nodeElementRef.current && focusedNode?.id === props.node.id) { + nodeElementRef.current?.focus(); + } + }, [focusedNode?.id, props.node.id]); + if (editable) { if (!editor) { throw new Error("can't edit nodes before codemirror has mounted"); From 43f1c0791ed4586a9c268cf836d061e8e316d9af Mon Sep 17 00:00:00 2001 From: pcardune Date: Fri, 19 Nov 2021 11:52:34 -0800 Subject: [PATCH 4/6] Only focus a node on mount if the editor has focus --- .../codemirror-blocks/spec/activation-test.ts | 26 ++++++++++++++++--- .../codemirror-blocks/src/components/Node.tsx | 8 ++++-- packages/codemirror-blocks/src/editor.ts | 8 ++++++ .../codemirror-blocks/src/state/actions.ts | 2 +- 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/packages/codemirror-blocks/spec/activation-test.ts b/packages/codemirror-blocks/spec/activation-test.ts index e46372620..548362cdd 100644 --- a/packages/codemirror-blocks/spec/activation-test.ts +++ b/packages/codemirror-blocks/spec/activation-test.ts @@ -10,6 +10,7 @@ import { isNodeEditable, elementForNode, keyPress, + click, } from "../src/toolkit/test-utils"; import { API } from "../src/CodeMirrorBlocks"; import { ASTNode } from "../src/ast"; @@ -146,9 +147,28 @@ describe("inserting a new node", () => { // confirm that the new new was saved to the ast and focused expect(cmb.getAst().toString()).toBe("aBrandNewLiteral"); expect(cmb.getValue()).toEqual("aBrandNewLiteral"); - expect(document.activeElement!.id).toEqual( - screen.getByRole("treeitem", { name: /aBrandNewLiteral/ }).id - ); + 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(); }); }); diff --git a/packages/codemirror-blocks/src/components/Node.tsx b/packages/codemirror-blocks/src/components/Node.tsx index dddb80437..4ea0efcdb 100644 --- a/packages/codemirror-blocks/src/components/Node.tsx +++ b/packages/codemirror-blocks/src/components/Node.tsx @@ -244,10 +244,14 @@ const Node = ({ expandable = true, ...props }: Props) => { // as soon as we finish rendering. const focusedNode = useSelector(selectors.getFocusedNode); useEffect(() => { - if (nodeElementRef.current && focusedNode?.id === props.node.id) { + if ( + editor?.hasFocus() && + nodeElementRef.current && + focusedNode?.id === props.node.id + ) { nodeElementRef.current?.focus(); } - }, [focusedNode?.id, props.node.id]); + }, [editor, focusedNode?.id, props.node.id]); if (editable) { if (!editor) { 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 From a1e8b13e75707b20c6f36957c5d9a802432a90e3 Mon Sep 17 00:00:00 2001 From: pcardune Date: Fri, 19 Nov 2021 16:38:47 -0800 Subject: [PATCH 5/6] Add tests for focusing after a top-level deletion --- .../codemirror-blocks/spec/activation-test.ts | 23 +++++++++++++++++++ .../codemirror-blocks/src/components/Node.tsx | 11 ++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/packages/codemirror-blocks/spec/activation-test.ts b/packages/codemirror-blocks/spec/activation-test.ts index 548362cdd..716e69d71 100644 --- a/packages/codemirror-blocks/spec/activation-test.ts +++ b/packages/codemirror-blocks/spec/activation-test.ts @@ -172,6 +172,29 @@ describe("switching to block mode", () => { }); }); +describe("deleting a top-level node", () => { + let cmb: API; + beforeEach(() => { + cmb = mountCMB(wescheme).cmb; + }); + it("should focus on the previous node", () => { + 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, + } + `); + }); +}); + 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 4ea0efcdb..9b735dbb7 100644 --- a/packages/codemirror-blocks/src/components/Node.tsx +++ b/packages/codemirror-blocks/src/components/Node.tsx @@ -245,10 +245,15 @@ const Node = ({ expandable = true, ...props }: Props) => { const focusedNode = useSelector(selectors.getFocusedNode); useEffect(() => { if ( - editor?.hasFocus() && - nodeElementRef.current && - focusedNode?.id === props.node.id + !editor?.hasFocus() && + document.activeElement !== document.body && + document.activeElement !== null ) { + // 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]); From 92d8cd1d661ebf2ef861261c77df84a592c3d88b Mon Sep 17 00:00:00 2001 From: pcardune Date: Fri, 19 Nov 2021 17:14:35 -0800 Subject: [PATCH 6/6] Add tests for focusing after deleting a child node --- packages/codemirror-blocks/spec/activation-test.ts | 13 +++++++++++-- packages/codemirror-blocks/src/components/Node.tsx | 3 ++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/codemirror-blocks/spec/activation-test.ts b/packages/codemirror-blocks/spec/activation-test.ts index 716e69d71..19d2e4ae8 100644 --- a/packages/codemirror-blocks/spec/activation-test.ts +++ b/packages/codemirror-blocks/spec/activation-test.ts @@ -172,12 +172,12 @@ describe("switching to block mode", () => { }); }); -describe("deleting a top-level node", () => { +describe("deleting a node", () => { let cmb: API; beforeEach(() => { cmb = mountCMB(wescheme).cmb; }); - it("should focus on the previous node", () => { + 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(" "); @@ -193,6 +193,15 @@ describe("deleting a top-level node", () => { } `); }); + + 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", () => { diff --git a/packages/codemirror-blocks/src/components/Node.tsx b/packages/codemirror-blocks/src/components/Node.tsx index 9b735dbb7..98d5631c2 100644 --- a/packages/codemirror-blocks/src/components/Node.tsx +++ b/packages/codemirror-blocks/src/components/Node.tsx @@ -247,7 +247,8 @@ const Node = ({ expandable = true, ...props }: Props) => { if ( !editor?.hasFocus() && document.activeElement !== document.body && - document.activeElement !== null + 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.