Inserting a root node does not update focus correctly

[schanzer]

Describe the bug
Inserting a b c as root nodes - anywhere in the document - should put focus on c when the editor is re-rendered. Instead, focus is lost.

To Reproduce
Steps to reproduce the behavior:

1. Load the default editor, and switch to block mode
2. Place the cursor on the line beneath quuz, and type a b c.
3. Hit enter
4. See error

Expected behavior
a, b, and c appear as root nodes, with c activated

Desktop (please complete the following information):

• OS: OS X Catalina
• Browser: Brave
• Version 1.31 (chromium 95)

Additional context
This would make a good test case for #468

[pcardune]

I'll take a stab at this one.

Some questions you might be able to answer for me though:

We track state for "focused node" and "selected nodes", which are both separate from the browser's document.activeElement and window.getSelection() and codemirror's "selected ranges", but are sometimes the same. Which of these are actually distinct states and which are supposed to be kept in sync?

Would this be a correct explanation of the semantics:

• selected nodes - multiple nodes can be selected, including nodes that are in different parts of the tree. Selected nodes get a dashed border.
• focused node - only one node can be "focused" at a time. A focused node has a solid border. The arrow keys let you move focus between nodes. Clicking on a node will also focus it. A focused node is not "selected" unless you press spacebar, in which case it will be both focused and selected. The children of a focused node are not focused just because their parent is focused, but when a node is "selected", all of its children are "selected" as well. Changing the focused node does not affect the set of selected nodes.
• document.activeElement - The html element that has focus. When navigating between nodes using arrow keys, both the "focused node" state and document.activeElement are updated to point to the AST node, and it's dom element respectively. But if the user clicks on an element that's does not correspond to an AST node (like the search box), then only document.activeElement is updated - the "focused node" will still be the "focused node" (this is the part I'm not sure about it).

How editing actions work:

• deleting nodes - pressing the "delete" or "backspace" key while document.activeElement corresponds to an ast node (and not another element on the page or the codemirror text area or the contenteditable element for a node in edit mode) should delete the current "focused node" and it's children and all the "selected nodes". After the deletion, there should be no "selected nodes" and the "focused node" should be _______?
• copying nodes - pressing "ctrl-c" or whatever the copy shortcut is while document.activeElement corresponds to an ast node will copy the pretty printed text of the "focused node" and its children to the copy buffer. It does not copy "selected nodes". It should not change the set of selected nodes, nor the "focused node" nor document.activeElement.
• cutting nodes - pressing "ctrl-x" or whatever the cut shortcut is while document.activeElement corresponds to an ast node, will do the same thing as "copying nodes", followed by deleting the "focused node" (but not the selected nodes as in "deleting nodes"). After the cutting, the "focused node" should be _______?
• pasting nodes - pressing "ctrl-v" or whatever the past shortcut is while document.activeElement corresponds to an ast node, will replace the focused node and its children with nodes that were copied. The "focused node" should be the root node of the nodes that were just pasted and document.activeElement should point to the new dom element for that root node.
• editing nodes - pressing "enter" while document.activeElement corresponds to an ast node will switch the "focused node" to "edit mode" allowing you to type in code that will replace that node and its children. After typing "enter", the "focused node" and its children are replaced, and both document.activeElement and the "focused node" are updated as in "pasting nodes". If instead of pressing "enter", you click somewhere else on the screen, then the "focused node" and its children are replaced, document.activeElement changes to whatever element you clicked on and there is no "focused node" (unless the element you clicked on corresponds to another ast node, in which case that node becomes the "focused node".
• inserting nodes - typing or pasting text at the top level (i.e. in the codemirror text area and not in a node being edited) creates a "quarantine" that is just like a node in edit mode. Whatever node might have been the "focused node" will no longer be the "focused node" as there is no "focused node" when inserting. As in "editing nodes", when you press "Enter", the node generated from the text you typed in becomes the "focused node". If you click elsewhere, the new node is still created, but it does not become the "focused node".
• dragging a node to the trashcan - behaves the same as "cutting nodes", but without putting anything into the paste buffer.

Of the above editing actions, the only one that operates on "selected nodes" is "deleting nodes". When the "focused node" changes, document.activeElement is updated to point to the dom element corresponding to that focused node. But if document.activeElement is changed through a different mechanism, the "focused node" does not change.

Do I have all this right? Do tests for this behavior belong in "activation-tests"? Is there a semantic difference in the meaning behind "activation" vs "focus"?

[schanzer]

We track state for "focused node" and "selected nodes", which are both separate from the browser's document.activeElement and window.getSelection() and codemirror's "selected ranges", but are sometimes the same. Which of these are actually distinct states and which are supposed to be kept in sync?

When editing a text file, sighted users have to consider one bit of state at a time: "where is the cursor?", or "what is selected?" When something is selected, their cursor sort of melts into one end of the most-recent selection. Visually-impaired (VI) users must always keep track of where their "focus cursor" is at all times. When nothing is selected, their focus cursor is synonymous with the actual cursor. But if something is selected, their focus cursor persists and can move around without modifying the selection! We refer to the node that is focused as being "activated", thus activateByNId. In answer to your last question, we wanted to differentiate "active" from "focused" because the browser has its own notion of focus. (Although confusingly enough, the browser calls the focused DOM element the "active" element....)

We have to track focusId separately from document.activeElement. When the editor is focused, the "focus cursor" is always document.activeElement. But if the user switches focus to the toolbar, the toggle button, or anywhere else on the page, we don't want to lose track of that cursor! Focusing back on the editor should return focus to the expected node.

We also have to track selection separately from window.getSelection because we may have selected nodes that CM elides from the document. It might be possible to use CodeMirror's own selection state here, though, which would be a really nice simplification. However, I ran into trouble trying this a long time ago because CM doesn't allow selection inside an atomic range, and all markerWidgets are atomic. That means CM won't let us select a child of a rootNode - only the rootNode itself.

Your semantics for selected nodes, focused node, and activeElement are all correct

deleting nodes - Mostly correct. Only selected nodes can be deleted. Sina recommended this, since a destructive operation should require users to "opt in" by hitting the spacebar. This will change both the selection state (nothing selected) and the focused node (computed based on the edit).

copying nodes - hitting Ctrl-C should copy the focused node and all selected nodes. It does not change selection state, or the focused node.

cutting nodes - the same as hitting copy and then delete. If nodes A and B are selected and node C is focused, then all three are copied and A+B are deleted. C retains focus.

pasting nodes - If a node is selected, pasting will replace it with the clipboard contents. If nothing is selected, Ctrl-V will insert the clipboard after the focused node (and Shift-Ctrl-V will insert the clipboard before the focused node). Focus is updated to be the last child of the inserted subtree.

editing nodes - your semantics are exactly right.

inserting nodes - your semantics are exactly right. One addendum - if you hit "enter" when editing and save the edit, focus switches to the last child of the inserted subtree.

dragging to the trash - same as "deleting nodes". One thing I don't believe we've spec'd out is dragging an item to the trash when multiple nodes are selected. My instinct is to say that we only delete the dragged node, and we leave selection alone unless the dragged node happened to also be selected.

In general, I've tried to put all of the tests regarding active/focus updates into activation-test. But I can see an argument for separate test files that deal with inserting, editing/deleting, and copy/paste/cut. Setting trashcan drags aside, each of the six actions listed above needs tests for:

1. at the top (CM level), performed with or on a single root node when selection is empty
2. at the top (CM level), performed with or on a single root node when selection is non-empty
3. at the top (CM level), performed with or on multiple root nodes when selection is empty
4. at the top (CM level), performed with or on multiple root nodes when selection is non-empty
5. on a child node, performed with or on a single node when selection is empty
6. on a child node, performed with or on a single node when selection is non-empty
7. on a child node, performed with or on multiple nodes when selection is empty
8. on a child node, performed with or on multiple nodes when selection is non-empty

So ideally, that's 32 tests. 40 if you count paste-before (Shift-Ctrl-V) as a separate action. Many of these tests are already in in the suite, but I'm certain we need more. I'm happy to own organizing and writing whatever is missing, though I've hit a wall when it comes to mocking paste and drag events.

[pcardune]

Thank you for providing such a thorough response to my questions. I'll need to review this carefully to make sure I understand everything.