fix(focus): correct traversal order

Author: posva

.vscode/launch.json

@@ -0,0 +1,17 @@
+{
+  // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
+  "version": "0.2.0",
+  "configurations": [
+    {
+      "type": "pwa-node",
+      "request": "launch",
+      "name": "Debug Current Test File",
+      "autoAttachChildProcesses": true,
+      "skipFiles": ["<node_internals>/**", "**/node_modules/**"],
+      "program": "${workspaceRoot}/node_modules/vitest/vitest.mjs",
+      "args": ["run", "${relativeFile}"],
+      "smartStep": true,
+      "console": "integratedTerminal"
+    }
+  ]
+}

packages/cli/components.d.ts

@@ -5,6 +5,7 @@ import '@vue/runtime-core'
 
 declare module '@vue/runtime-core' {
   export interface GlobalComponents {
+    Box: typeof import('vue-termui')['TuiBox']
     Div: typeof import('vue-termui')['TuiBox']
     Link: typeof import('vue-termui')['TuiLink']
     Text: typeof import('vue-termui')['TuiText']

packages/cli/demo/bugs/Focusables.vue

@@ -0,0 +1,44 @@
+<script setup lang="ts">
+import {
+  ref,
+  reactive,
+  MouseDataEvent,
+  MouseEventType,
+  useStdoutDimensions,
+  useTitle,
+} from 'vue-termui'
+import Input from './Input.vue'
+
+const [cols, rows] = useStdoutDimensions()
+
+const disabled = ref(1)
+onKeyData(['d', 'D'], () => {
+  disabled.value = (disabled.value + 1) % 4
+})
+</script>
+
+<template>
+  <Box
+    :width="cols"
+    :height="30"
+    justifyContent="center"
+    flexDirection="column"
+    alignItems="center"
+    borderStyle="round"
+    title="Focusables"
+  >
+    <Text underline>Cannot be focused</Text>
+    <Box title="Other stuff" borderStyle="round">
+      <Text>I am just some text</Text>
+    </Box>
+    <Box title="Focused here" flexDirection="column" borderStyle="round">
+      <Link
+        v-for="i in 4"
+        :href="`https://esm.dev/labs/${i - 1}`"
+        :disabled="disabled !== i - 1"
+        >Lab {{ i }}{{ disabled !== i - 1 ? ' (disabled)' : '' }}</Link
+      >
+      <Link href="https://example.com">Another one</Link>
+    </Box>
+  </Box>
+</template>

packages/core/src/app/createApp.ts

@@ -71,8 +71,6 @@ export function createApp(
     log.clear()
 
     const rootEl = new DOMElement('tui:root')
-    // for debugging purposes
-    rootEl.toString = () => `<Root>`
 
     const focusManager = createFocusManager(rootEl)
 

packages/core/src/focus/FocusManager.spec.ts

@@ -1,15 +1,83 @@
-import { DOMElement } from '../renderer/dom'
 import { createFocusManager } from './FocusManager'
+import { DOMElement, DOMNode, TextNode, CommentNode } from '../renderer/dom'
+import {
+  ComponentInternalInstance,
+  VNode,
+  ref,
+  ComputedRef,
+  h,
+} from '@vue/runtime-core'
+import { Focusable } from './types'
+
+/**
+ * create internal instance
+ * @param el
+ * @returns
+ */
+function focusable(
+  _el: DOMNode | string,
+  options: Partial<Focusable> | false = {}
+): ComponentInternalInstance {
+  const el = typeof _el === 'string' ? new TextNode(_el) : _el
+  const vnode = { el } as VNode<DOMNode, DOMElement>
+  // @ts-expect-error: we only need the el for focus
+  const instance = { vnode } as ComponentInternalInstance
+
+  el.focusable =
+    options === false
+      ? null
+      : {
+          active: ref(false) as ComputedRef<boolean>,
+          disabled: ref(false),
+          id: Symbol(),
+          _i: instance,
+          ...options,
+        }
+
+  return instance
+}
+
+focusable(new TextNode('a'))
+focusable('a')
+
+interface DOMTree extends Array<DOMTree | DOMNode | string> {}
+
+function createFocusableTree(
+  tree: DOMTree,
+  parent: DOMElement = new DOMElement('tui:root')
+) {
+  tree.forEach((node) => {
+    if (Array.isArray(node)) {
+      parent.insertNode(createFocusableTree(node, new DOMElement('tui:box')))
+    } else {
+      const ci = focusable(node)
+      parent.insertNode(ci.vnode.el as DOMNode)
+    }
+  })
+
+  return parent
+}
 
 describe('FocusManager', () => {
   it('creates a focus manager', () => {
-    const root = new DOMElement('tui:root')
+    const root = createFocusableTree([])
+    const fm = createFocusManager(root)
+
+    expect(fm.activeElement.value).toBeNull()
+    expect(fm.focusNext()).toBeFalsy()
+    expect(fm.activeElement.value).toBeNull()
+    expect(fm.focusPrevious()).toBeFalsy()
+    expect(fm.activeElement.value).toBeNull()
+  })
+
+  it('loops through one single item with next', () => {
+    const root = createFocusableTree(['1'])
     const fm = createFocusManager(root)
 
     expect(fm.activeElement.value).toBeNull()
-    expect(fm.focusNext()).toBeUndefined()
+    expect(fm.focusNext()).toBeFalsy()
     expect(fm.activeElement.value).toBeNull()
-    expect(fm.focusPrevious()).toBeUndefined()
+    expect(fm.focusPrevious()).toBeFalsy()
     expect(fm.activeElement.value).toBeNull()
   })
 })

packages/core/src/focus/FocusManager.ts

@@ -6,10 +6,15 @@ import {
   shallowRef,
   unref,
 } from '@vue/runtime-core'
-import { nextSibling, previousSibling } from '../renderer/nodeOpts'
+import {
+  getLastLeaf,
+  previousDeepSibling,
+  nextDeepSibling,
+} from '../renderer/nodeOpts'
 import { DOMElement, DOMNode, isDOMElement } from '../renderer/dom'
 import { checkCurrentInstance, getElementFromInstance, noop } from '../utils'
 import { Focusable, FocusId } from './types'
+import { indentHTML } from '../utils/indentHTML'
 
 export interface FocusManager {
   activeElement: ShallowRef<Focusable | null | undefined>
@@ -26,7 +31,7 @@ export interface FocusManager {
   _changeFocusableId(newId: FocusId, oldId: FocusId): void
 
   /**
-   * Focus a Focusable or remove the current focus by passing `null`.
+   * Focus a Focusable or remove the current focus by passing `null`. Returns the currently Focusable or null otherwise.
    *
    * @param id - id of the focusable
    */
@@ -92,62 +97,63 @@ export function createFocusManager(
     }
   }
 
-  const focusNext: FocusManager['focusNext'] = () => {
-    // the rootNode cannot be focused, so it's safe to start from there
-    const startNode: DOMNode =
-      getElementFromInstance(activeElement.value?._i) || rootNode
-    let node: DOMNode | null = startNode
-    // startNode === rootNode ? rootNode.childNodes[0] : nextSibling(startNode)
+  const focusPrevious: FocusManager['focusPrevious'] = () => {
+    const lastNode = getLastLeaf(rootNode)
+    const activeNode = getElementFromInstance(activeElement.value?._i)
+    let startNode = activeNode
+    let cursor: DOMNode | null | undefined = activeNode
 
     do {
-      node = nextDeepSibling(node, true)
-      // we reached the end of the tree and we didn't start at the root node
-      // so let's try going from the root
-      if (!node && startNode !== rootNode && cyclic) {
-        node = nextDeepSibling(rootNode, true)!
+      // get the previous deep node or the very last node
+      cursor = (cursor && previousDeepSibling(cursor)) || lastNode
+
+      // cursor cannot be empty but startNode can
+      if (cursor === startNode) {
+        break
       }
+      // it is safe to set the starter node to the lastNode now
+      // so we can detect full loops
+      startNode = startNode || lastNode
     } while (
-      node &&
-      // we did a full loop
-      node !== startNode &&
+      cursor &&
       // we skip any non focusable
-      (!node.focusable ||
+      (!cursor.focusable ||
         // or disabled element
-        node.focusable.disabled.value)
+        cursor.focusable.disabled.value)
     )
 
-    if (node && node.focusable) {
-      return focus(unref(node.focusable.id))
+    if (cursor.focusable) {
+      return focus(unref(cursor.focusable.id))
     }
   }
 
-  const focusPrevious: FocusManager['focusPrevious'] = () => {
-    // the rootNode cannot be focused, so it's safe to start from there
-    const lastNode = getLastNode(rootNode)
-    const startNode: DOMNode =
-      getElementFromInstance(activeElement.value?._i) || lastNode
-    let node: DOMNode | null = startNode
-    // startNode === rootNode ? rootNode.childNodes[0] : nextSibling(startNode)
+  const focusNext: FocusManager['focusNext'] = () => {
+    const firstNode = rootNode
+    const activeNode = getElementFromInstance(activeElement.value?._i)
+    let startNode = activeNode
+    let cursor: DOMNode | null | undefined = activeNode
 
     do {
-      node = nextDeepSibling(node, false)
-      // we reached the rootNode of the tree and we didn't start at the root node
-      // so let's try going from the root
-      if (!node && startNode !== lastNode && cyclic) {
-        node = nextDeepSibling(lastNode, false)!
+      // get the next deep node or the very first node (the root)
+      cursor = (cursor && nextDeepSibling(cursor)) || firstNode
+
+      // cursor cannot be empty but startNode can
+      if (cursor === startNode) {
+        break
       }
+      // it is safe to set the starter node to the lastNode now
+      // so we can detect full loops
+      startNode = startNode || firstNode
     } while (
-      node &&
-      // we did a full loop
-      node !== lastNode &&
+      cursor &&
       // we skip any non focusable
-      (!node.focusable ||
+      (!cursor.focusable ||
         // or disabled element
-        node.focusable.disabled.value)
+        cursor.focusable.disabled.value)
     )
 
-    if (node && node.focusable) {
-      return focus(unref(node.focusable.id))
+    if (cursor.focusable) {
+      return focus(unref(cursor.focusable.id))
     }
   }
 
@@ -159,6 +165,7 @@ export function createFocusManager(
   const _add: FocusManager['_add'] = (focusable) => {
     focusableMap.set(unref(focusable.id), focusable)
     const el = getElementFromInstance(focusable._i)
+    // TODO: can el be an array? If so, should we error and allow only single element root?
     if (!el) {
       throw new Error('NO VNODE wat')
     }
@@ -168,6 +175,12 @@ export function createFocusManager(
     const id = unref(focusable.id)
     const existingFocusable = focusableMap.get(id)
     if (existingFocusable) {
+      // remove the cyclic referenc
+      const el = getElementFromInstance(focusable._i)
+      if (el) {
+        el.focusable = null
+      }
+
       focusableMap.delete(id)
       // if the focusable being removed is focused, remove focus
       if (activeElement.value === existingFocusable) {
@@ -199,41 +212,3 @@ export function createFocusManager(
     trapFocus,
   }
 }
-
-/**
- * Gets the next deep sibling. A DFS tree search. Returns `null` if we reached the root.
- *
- * @param node - node to start at
- */
-function nextDeepSibling(node: DOMNode, forward: boolean): DOMNode | null {
-  // check if the node has children
-  if (forward && isDOMElement(node) && node.childNodes.length > 0) {
-    return node.childNodes[0]
-  } else {
-    // get the next sibling based on the parent
-    let nextNode = (forward ? nextSibling : previousSibling)(node)
-    if (nextNode) return nextNode
-    // no next sibling, find the closest parent next sibling
-    nextNode = node.parentNode
-    while (nextNode) {
-      const sibling = (forward ? nextSibling : previousSibling)(nextNode)
-      if (sibling) {
-        return sibling
-      }
-      // try again with the parent
-      nextNode = nextNode.parentNode
-    }
-
-    // we reached the root
-    return null
-  }
-}
-
-function getLastNode(node: DOMElement): DOMNode {
-  let cursor: DOMNode = node
-  while (isDOMElement(cursor) && cursor.childNodes.length > 0) {
-    cursor = cursor.childNodes[cursor.childNodes.length - 1]
-  }
-
-  return cursor
-}

packages/core/src/focus/Focusable.ts

@@ -78,7 +78,12 @@ export function useFocus({
       focus(unref(id))
     }
   })
-  onUnmounted(() => _remove(focusable))
+  onUnmounted(() => {
+    _remove(focusable)
+    // this is okay because the focusable is being destroyed
+    // @ts-expect-error: avoid cyclic references
+    focusable._i = null
+  })
 
   return focusable
 }

packages/core/src/index.spec.ts

@@ -53,6 +53,7 @@ describe('Full App', () => {
 
     stdin.emit('data', 'A')
     await nextTick()
+    // TODO: we probably need a custom nextTick that triggers after a render
     await delay(40)
 
     expect(stdout.write).toHaveBeenLastCalledWith(