From 851c2154daaa759325bd2baf6b7da2f44628b4bb Mon Sep 17 00:00:00 2001 From: M <> Date: Mon, 14 Sep 2026 14:21:55 +0200 Subject: [PATCH 1/3] CON-15: a keyboard and touch path for moving a page, in four steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving a page was drag & drop only. HTML5 drag & drop has no keyboard path at all, and on a touch screen it does not fire even once — so on a phone moving a page was not awkward, it was impossible. docs/06 carried that as an open point ever since the page's own move picker was dropped. One menu answers both. Its trigger is a real button on every tree row, so it is in the tab order and under a finger; its entries are the four steps every outline editor has — up, down, in under the sibling above, out to behind the parent. Reimplementing the drag on pointer events would have given the touch screen a gesture and the keyboard still nothing. Dragging stays the primary gesture and is untouched: it says where a page lands better than any list can. Where each step lands is `src/domain/move-tree.ts`, away from the menu, because it is a question about the tree and because the order keys are only checkable in isolation. A step that picks a *parent* writes no key and lets the new level sort by title, which is exactly what dropping onto that row already does — the tree must not look different depending on whether a mouse or the keyboard moved the page. A key is written only where a position within a level is genuinely chosen: up, down and out. The trigger follows the "+" on the Pages heading: hidden until the row is hovered or focused, because the tree is read far more often than it is rearranged. A coarse pointer has no hover to reveal it with, so there it is always visible — without that, the touch half of this ticket would have shipped invisible. Co-Authored-By: Claude Opus 5 --- src/domain/move-tree.test.ts | 148 +++++++++++++++++++++++++++++++++ src/domain/move-tree.ts | 114 ++++++++++++++++++++++++++ src/ui/PageMoveMenu.test.tsx | 146 +++++++++++++++++++++++++++++++++ src/ui/PageMoveMenu.tsx | 154 +++++++++++++++++++++++++++++++++++ src/ui/icons.tsx | 14 ++++ src/ui/layout/Sidebar.tsx | 41 +++++++++- src/ui/move-page.ts | 47 +++++++++++ 7 files changed, 663 insertions(+), 1 deletion(-) create mode 100644 src/domain/move-tree.test.ts create mode 100644 src/domain/move-tree.ts create mode 100644 src/ui/PageMoveMenu.test.tsx create mode 100644 src/ui/PageMoveMenu.tsx diff --git a/src/domain/move-tree.test.ts b/src/domain/move-tree.test.ts new file mode 100644 index 0000000..94c404a --- /dev/null +++ b/src/domain/move-tree.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, it } from 'vitest' +import { buildPages, buildTree, flattenTree, orderKeyOf } from './pages' +import { planMove, siblingsOf } from './move-tree' +import type { MoveDirection } from './move-tree' +import type { Page } from './pages' +import type { Revision } from './revision' + +function rev(partial: Partial & { id: string }): Revision { + return { + author: 'alice', + createdAt: 1000, + group: 'engineering', + slug: partial.id, + title: partial.id, + parentSlug: null, + order: null, + parentRevs: [], + summary: null, + content: '', + ...partial, + } +} + +/** A tree from `slug:parent` pairs, with each page's title as its own slug. */ +function tree(...spec: [slug: string, parent: string | null][]): Page[] { + return buildPages(spec.map(([slug, parent]) => rev({ id: slug, parentSlug: parent }))) +} + +/** + * The move applied, as `buildPages` would hand the result back. + * + * The re-sort is the point, not bookkeeping: `planMove` reads a level by + * filtering an already globally sorted array, which is what `buildPages` + * produces and what `space.pages` always is. A test that changed one key and + * left the array where it was would be asking the function a question it never + * gets in the app — and would answer the second move from the tree as it + * looked before the first. + */ +function applyMove(pages: Page[], slug: string, direction: MoveDirection): Page[] { + const move = planMove(pages, slug, direction) + if (!move) throw new Error(`${direction} was not available for ${slug}`) + return pages + .map((page) => + page.slug === slug ? { ...page, parentSlug: move.parentSlug, order: move.order } : page, + ) + .sort((a, b) => { + const left = orderKeyOf(a) + const right = orderKeyOf(b) + if (left !== right) return left < right ? -1 : 1 + return a.slug < b.slug ? -1 : 1 + }) +} + +/** + * The tree somebody would see after the move, indented — an order key is not + * what anybody is checking, the row it puts the page on is. + */ +function after(pages: Page[], slug: string, direction: MoveDirection): string[] { + return flattenTree(buildTree(applyMove(pages, slug, direction))).map( + (node) => `${' '.repeat(node.depth)}${node.slug}`, + ) +} + +const FLAT = tree(['a', null], ['b', null], ['c', null], ['d', null]) + +describe('siblingsOf', () => { + it('keeps the level in the order the tree draws it', () => { + expect(siblingsOf(FLAT, null).map((page) => page.slug)).toEqual(['a', 'b', 'c', 'd']) + const nested = tree(['a', null], ['x', 'a'], ['y', 'a']) + expect(siblingsOf(nested, 'a').map((page) => page.slug)).toEqual(['x', 'y']) + }) +}) + +describe('planMove — up and down', () => { + it('swaps with the row above, and with the row below', () => { + expect(after(FLAT, 'c', 'up')).toEqual(['a', 'c', 'b', 'd']) + expect(after(FLAT, 'b', 'down')).toEqual(['a', 'c', 'b', 'd']) + }) + + it('reaches the first and the last position, not just the middle', () => { + expect(after(FLAT, 'b', 'up')).toEqual(['b', 'a', 'c', 'd']) + expect(after(FLAT, 'c', 'down')).toEqual(['a', 'b', 'd', 'c']) + }) + + it('is a round trip: up and back down leaves the order it found', () => { + expect(after(applyMove(FLAT, 'c', 'up'), 'c', 'down')).toEqual(['a', 'b', 'c', 'd']) + expect(after(applyMove(FLAT, 'b', 'down'), 'b', 'up')).toEqual(['a', 'b', 'c', 'd']) + }) + + it('has nowhere to go at the ends of a level', () => { + expect(planMove(FLAT, 'a', 'up')).toBeNull() + expect(planMove(FLAT, 'd', 'down')).toBeNull() + }) + + it('counts siblings, not rows: a subtree in between is stepped over whole', () => { + const nested = tree(['a', null], ['b', null], ['deep', 'b'], ['c', null]) + // `a` moving down passes `b` and everything hanging under it in one step + expect(after(nested, 'a', 'down')).toEqual(['b', ' deep', 'a', 'c']) + }) + + it('stays where it is when the level holds only one page', () => { + const only = tree(['a', null], ['x', 'a']) + expect(planMove(only, 'x', 'up')).toBeNull() + expect(planMove(only, 'x', 'down')).toBeNull() + }) +}) + +describe('planMove — in and out', () => { + it('files the page under the sibling above it', () => { + expect(after(FLAT, 'b', 'in')).toEqual(['a', ' b', 'c', 'd']) + }) + + it('writes no key of its own — the new level sorts it by its title', () => { + // The same answer dropping the page onto that row with a mouse gives, so + // the tree does not depend on which input device moved the page. + const nested = tree(['a', null], ['x', 'a'], ['y', 'a'], ['b', null]) + expect(planMove(nested, 'b', 'in')).toEqual({ parentSlug: 'a', order: null }) + expect(after(nested, 'b', 'in')).toEqual(['a', ' b', ' x', ' y']) + }) + + it('has no sibling above it to go in under', () => { + expect(planMove(FLAT, 'a', 'in')).toBeNull() + }) + + it('puts the page directly behind its parent, not at the end of that level', () => { + const nested = tree(['a', null], ['x', 'a'], ['b', null], ['c', null]) + expect(after(nested, 'x', 'out')).toEqual(['a', 'x', 'b', 'c']) + }) + + it('keeps the page ahead of its parent-level neighbour it was never behind', () => { + const nested = tree(['a', null], ['x', 'a'], ['y', 'a'], ['b', null]) + // both children come out one after the other and stay in their order + expect(after(applyMove(nested, 'x', 'out'), 'y', 'out')).toEqual(['a', 'y', 'x', 'b']) + }) + + it('cannot come out of the top level', () => { + expect(planMove(FLAT, 'a', 'out')).toBeNull() + }) + + it('takes the page and its own subtree along, in and out', () => { + const nested = tree(['a', null], ['b', null], ['deep', 'b'], ['deeper', 'deep']) + expect(after(nested, 'b', 'in')).toEqual(['a', ' b', ' deep', ' deeper']) + }) + + it('answers null for a slug the space does not have', () => { + expect(planMove(FLAT, 'nope', 'up')).toBeNull() + }) +}) diff --git a/src/domain/move-tree.ts b/src/domain/move-tree.ts new file mode 100644 index 0000000..1726ac2 --- /dev/null +++ b/src/domain/move-tree.ts @@ -0,0 +1,114 @@ +import { keyBetween } from './order' +import { orderKeyOf } from './pages' +import type { Page } from './pages' + +/** + * The four moves an outliner has, expressed as placements. + * + * Dragging says where a page ends up better than any list of slugs can — which + * is why the page's own move action was dropped (docs/06). But HTML5 drag & + * drop has no keyboard path at all, and on a touch screen it does not fire + * even once: there, moving a page was simply impossible. These four are the + * vocabulary every outline editor uses for the same job, and every one of them + * is a single, predictable step somebody can repeat and watch. + * + * Kept here rather than in the sidebar because "where does this page land" is + * a question about the tree, not about a menu — and because the interesting + * part, the order key, is only checkable in isolation. + */ +export type MoveDirection = + /** swap with the sibling above */ + | 'up' + /** swap with the sibling below */ + | 'down' + /** become the last child of the sibling above */ + | 'in' + /** leave the parent and follow directly behind it */ + | 'out' + +/** + * Where a move puts the page: whose child it becomes, and at which key. + * + * `order: null` means "no key of its own" — the new level then sorts it by its + * title (src/domain/order.ts). That is what picking a *parent* produces, and + * it is deliberately the same answer dropping a page onto a row gives: the + * tree must not end up looking different depending on whether the page was + * moved with a mouse or with the keyboard. A key is only written where a + * position within a level is genuinely being chosen — up, down and out. + */ +export type TreeMove = { parentSlug: string | null; order: string | null } + +/** + * One level, in the order it is drawn. `pages` comes from `buildPages`, which + * has already sorted globally by order key — so filtering keeps that order and + * no second sort is needed here. + */ +export function siblingsOf(pages: Page[], parentSlug: string | null): Page[] { + return pages.filter((page) => page.parentSlug === parentSlug) +} + +/** + * Where `slug` would land, or `null` when the move has nowhere to go: the top + * row of a level cannot go up, the bottom row cannot go down, a page at the + * root cannot come further out, and a page with no sibling above it has + * nothing to move in under. The menu disables exactly those entries, so an + * unavailable move is visible before it is tried rather than reported as an + * error afterwards. + * + * A caller still has to check `canMoveUnder`: not for these four — none of + * them can reach into the page's own subtree, because a sibling and a parent + * are never descendants — but because `useMovePage` is the one place that + * publishes, and it checks everything it publishes. + */ +export function planMove(pages: Page[], slug: string, direction: MoveDirection): TreeMove | null { + const page = pages.find((entry) => entry.slug === slug) + if (!page) return null + + const level = siblingsOf(pages, page.parentSlug) + const index = level.findIndex((entry) => entry.slug === slug) + if (index === -1) return null + + const previous = level[index - 1] + const next = level[index + 1] + + switch (direction) { + case 'up': { + if (!previous) return null + // Between the two rows above it: the one it swaps with, and whatever is + // above *that*. Writing "the key of the row above" would collide rather + // than overtake. + const above = level[index - 2] + return { + parentSlug: page.parentSlug, + order: keyBetween(above ? orderKeyOf(above) : null, orderKeyOf(previous)), + } + } + case 'down': { + if (!next) return null + const below = level[index + 2] + return { + parentSlug: page.parentSlug, + order: keyBetween(orderKeyOf(next), below ? orderKeyOf(below) : null), + } + } + case 'in': { + if (!previous) return null + // No key: this step chooses a parent, not a position, and dropping the + // same page onto the same row with a mouse chooses no position either. + // See `TreeMove` — one destination, one result, whatever moved it. + return { parentSlug: previous.slug, order: null } + } + case 'out': { + const parent = pages.find((entry) => entry.slug === page.parentSlug) + if (!parent) return null + // Directly behind the parent in the parent's own level, which is where + // the row visually already is — the step is outwards, not downwards. + const uncles = siblingsOf(pages, parent.parentSlug) + const after = uncles[uncles.findIndex((entry) => entry.slug === parent.slug) + 1] + return { + parentSlug: parent.parentSlug, + order: keyBetween(orderKeyOf(parent), after ? orderKeyOf(after) : null), + } + } + } +} diff --git a/src/ui/PageMoveMenu.test.tsx b/src/ui/PageMoveMenu.test.tsx new file mode 100644 index 0000000..eff3f94 --- /dev/null +++ b/src/ui/PageMoveMenu.test.tsx @@ -0,0 +1,146 @@ +// @vitest-environment jsdom +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { act } from 'react' +import { createRoot } from 'react-dom/client' +import { buildPages } from '../domain/pages' +import type { Page } from '../domain/pages' +import type { Revision } from '../domain/revision' +import type { TreeMove } from '../domain/move-tree' +import { PageMoveMenu } from './PageMoveMenu' +import { moveEntries } from './move-page' + +/** + * The menu is the only way to move a page without a pointer that can drag, so + * what is pinned here is that it is reachable at all: a real button, entries + * that say which page they move past, and the ones that lead nowhere disabled + * rather than silently failing. docs/06-ui-information-architecture.md + */ +function rev(partial: Partial & { id: string }): Revision { + return { + author: 'alice', + createdAt: 1000, + group: 'engineering', + slug: partial.id, + title: partial.id.toUpperCase(), + parentSlug: null, + order: null, + parentRevs: [], + summary: null, + content: '', + ...partial, + } +} + +function tree(...spec: [slug: string, parent: string | null][]): Page[] { + return buildPages(spec.map(([slug, parent]) => rev({ id: slug, parentSlug: parent }))) +} + +const PAGES = tree(['a', null], ['b', null], ['x', 'b']) + +function render(page: Page, pages = PAGES, onMove: (move: TreeMove) => void = vi.fn()) { + const host = document.createElement('div') + document.body.appendChild(host) + const root = createRoot(host) + act(() => { + root.render() + }) + return host +} + +const pageOf = (slug: string, pages = PAGES) => pages.find((page) => page.slug === slug)! +const trigger = (host: HTMLElement) => host.querySelector('button')! +const items = (host: HTMLElement) => [...host.querySelectorAll('[role="menuitem"]')] as HTMLButtonElement[] + +describe('moveEntries', () => { + it('names the page a move would land under or come out of', () => { + const labels = moveEntries(PAGES, 'x').map((entry) => entry.label) + expect(labels).toContain('Move out of B') + + const deep = tree(['a', null], ['b', null], ['c', null]) + expect(moveEntries(deep, 'c').map((entry) => entry.label)).toContain('Move under B') + }) + + it('keeps the generic label for a move that has nowhere to go', () => { + const entries = moveEntries(PAGES, 'a') + const up = entries.find((entry) => entry.direction === 'up')! + expect(up.move).toBeNull() + expect(up.label).toBe('Move up') + }) +}) + +describe('PageMoveMenu', () => { + beforeEach(() => { + document.body.innerHTML = '' + }) + + it('is a button, not a gesture — the whole point of it', () => { + const host = render(pageOf('b')) + expect(trigger(host).getAttribute('aria-label')).toBe('Move B') + expect(trigger(host).getAttribute('aria-haspopup')).toBe('menu') + expect(trigger(host).disabled).toBe(false) + }) + + it('opens on a click and offers the four steps', () => { + const host = render(pageOf('b')) + act(() => trigger(host).click()) + expect(items(host).map((item) => item.textContent)).toEqual([ + 'Move up', + 'Move down', + 'Move under A', + 'Move out', + ]) + }) + + it('disables the steps that lead nowhere instead of failing on the click', () => { + const host = render(pageOf('b')) + act(() => trigger(host).click()) + const [up, down, moveIn, out] = items(host) + expect(up!.disabled).toBe(false) // B can swap with A + expect(down!.disabled).toBe(true) // nothing below it + expect(moveIn!.disabled).toBe(false) + expect(out!.disabled).toBe(true) // already at the top level + }) + + it('hands the destination up and closes again', () => { + const onMove = vi.fn() + const host = render(pageOf('x'), PAGES, onMove) + act(() => trigger(host).click()) + const out = items(host).find((item) => item.textContent?.startsWith('Move out'))! + act(() => out.click()) + expect(onMove).toHaveBeenCalledWith({ parentSlug: null, order: expect.any(String) }) + expect(items(host)).toHaveLength(0) + }) + + it('closes on Escape and gives the focus back to the trigger', () => { + const host = render(pageOf('b')) + act(() => trigger(host).click()) + act(() => { + items(host)[0]!.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }), + ) + }) + expect(items(host)).toHaveLength(0) + expect(document.activeElement).toBe(trigger(host)) + }) + + it('walks the entries with the arrow keys, skipping the disabled ones', () => { + const host = render(pageOf('b')) + act(() => trigger(host).click()) + // opening already focused the first usable entry + expect(document.activeElement).toBe(items(host)[0]) + act(() => { + document.activeElement!.dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), + ) + }) + // "Move down" and "Move out" are disabled for B, so "Move under A" is next + expect(document.activeElement?.textContent).toBe('Move under A') + }) + + it('has nothing to offer for the only page in a space, and says so', () => { + const alone = tree(['a', null]) + const host = render(pageOf('a', alone), alone) + expect(trigger(host).disabled).toBe(true) + expect(trigger(host).title).toContain('nowhere to move it') + }) +}) diff --git a/src/ui/PageMoveMenu.tsx b/src/ui/PageMoveMenu.tsx new file mode 100644 index 0000000..c5932a4 --- /dev/null +++ b/src/ui/PageMoveMenu.tsx @@ -0,0 +1,154 @@ +import { useEffect, useId, useRef, useState } from 'react' +import type { TreeMove } from '../domain/move-tree' +import type { Page } from '../domain/pages' +import { moveEntries } from './move-page' +import { MoveIcon } from './icons' + +/** + * The keyboard's and the touch screen's way of moving a page. + * + * Dragging in the tree is the better gesture and stays the primary one — it + * says where a page ends up better than any list can (docs/06). What it is + * not is reachable: HTML5 drag & drop has no keyboard path, and on a touch + * screen it does not fire at all, so moving a page there was not merely + * awkward, it was impossible. + * + * One menu answers both. Its trigger is a real button, so it is in the tab + * order and under a finger; its entries are the four steps an outline editor + * has. Reimplementing the drag on pointer events would have given touch a + * gesture and the keyboard still nothing. + * + * The trigger is hidden until the row is hovered or something inside it is + * focused — the tree is read far more often than it is rearranged, the same + * reasoning as the "+" on the Pages heading. On a coarse pointer there is no + * hover to reveal it with, so there it is simply always visible. + */ +export function PageMoveMenu({ + page, + pages, + busy, + onMove, +}: { + page: Page + pages: Page[] + busy: boolean + onMove: (move: TreeMove) => void +}) { + const [open, setOpen] = useState(false) + const triggerRef = useRef(null) + const menuRef = useRef(null) + const menuId = useId() + + const entries = moveEntries(pages, page.slug) + const available = entries.some((entry) => entry.move !== null) + + const close = (returnFocus: boolean) => { + setOpen(false) + if (returnFocus) triggerRef.current?.focus() + } + + // Opening puts the focus on the first entry that can actually be used: a + // menu that opens with nothing focused is a menu the keyboard has to find + // its way into first. + useEffect(() => { + if (!open) return + const first = menuRef.current?.querySelector('button:not([disabled])') + first?.focus() + }, [open]) + + // Clicking anywhere else closes it. `pointerdown` rather than `click`, so + // the menu is gone before whatever was clicked reacts — otherwise a click on + // another row's trigger closes this menu and opens nothing. + useEffect(() => { + if (!open) return + const onPointerDown = (event: PointerEvent) => { + const target = event.target as Node + if (menuRef.current?.contains(target) || triggerRef.current?.contains(target)) return + setOpen(false) + } + window.addEventListener('pointerdown', onPointerDown) + return () => window.removeEventListener('pointerdown', onPointerDown) + }, [open]) + + const onMenuKeyDown = (event: React.KeyboardEvent) => { + if (event.key === 'Escape') { + event.stopPropagation() + close(true) + return + } + if (event.key !== 'ArrowDown' && event.key !== 'ArrowUp') return + event.preventDefault() + const items = [...(menuRef.current?.querySelectorAll('button') ?? [])].filter( + (item) => !item.disabled, + ) + if (items.length === 0) return + const at = items.indexOf(document.activeElement as HTMLButtonElement) + const step = event.key === 'ArrowDown' ? 1 : -1 + // Wrapping, because a four-entry menu with two of them disabled is one + // keypress from either end whichever way it wraps. + items[(at + step + items.length) % items.length]!.focus() + } + + return ( +
+ + + {open ? ( + + ) : null} +
+ ) +} diff --git a/src/ui/icons.tsx b/src/ui/icons.tsx index 5a91ad0..2aed91b 100644 --- a/src/ui/icons.tsx +++ b/src/ui/icons.tsx @@ -131,6 +131,20 @@ export function ListTreeIcon(props: IconProps) { ) } +/** + * Moving a page: arrows up and down over a vertical rule. Not a drag handle — + * the gesture behind it is a menu of steps, not a grab. src/ui/PageMoveMenu.tsx + */ +export function MoveIcon(props: IconProps) { + return ( + + + + + + ) +} + export function SettingsIcon(props: IconProps) { return ( diff --git a/src/ui/layout/Sidebar.tsx b/src/ui/layout/Sidebar.tsx index 88dae45..fe79d9a 100644 --- a/src/ui/layout/Sidebar.tsx +++ b/src/ui/layout/Sidebar.tsx @@ -12,6 +12,8 @@ import { keyBetween } from '../../domain/order' import { spaceAccess } from '../../domain/space-access' import { useSession } from '../../session/session' import { useMovePage } from '../move-page' +import { PageMoveMenu } from '../PageMoveMenu' +import type { TreeMove } from '../../domain/move-tree' import { InitialsDisc, SectionLabel } from '../controls' import { ChevronDownIcon, @@ -124,6 +126,18 @@ type TreeDnd = { onDropInGap: (parentSlug: string | null, before: PageNode | null, after: PageNode | null) => void } +/** + * The move menu's side of a tree row: what it may move, and where to send the + * result. Separate from `TreeDnd` because it answers the question drag & drop + * cannot — moving without a pointer that can drag. src/ui/PageMoveMenu.tsx + */ +type TreeMoves = { + enabled: boolean + pages: Page[] + busySlug: string | null + onMove: (page: Page, move: TreeMove) => void +} + /** A fixed entry: icon, label, and grey when it is the page you are on. */ function NavRow({ to, @@ -228,6 +242,16 @@ export function Sidebar({ group, space, snapshot, info, inSettings }: Props) { }, } + const moves: TreeMoves = { + enabled: signedIn, + pages: space.pages, + busySlug, + onMove: (page, target) => { + setError(null) + void move(page, { parent: pageBySlug(target.parentSlug), order: target.order }) + }, + } + // A drag can end without the source seeing `dragend`: it is cancelled with // Escape, dropped outside the window, or the row unmounts mid-drag because a // relay event rebuilt the tree. The drag state would then stay set, and the @@ -352,6 +376,7 @@ export function Sidebar({ group, space, snapshot, info, inSettings }: Props) { forcedOpen={forcedOpen} onToggle={toggleBranch} dnd={dnd} + moves={moves} /> )} @@ -387,6 +412,7 @@ function TreeBranch({ forcedOpen, onToggle, dnd, + moves, }: { nodes: PageNode[] base: string @@ -396,6 +422,7 @@ function TreeBranch({ forcedOpen: Set onToggle: (slug: string) => void dnd: TreeDnd + moves: TreeMoves }) { /** * The two rows a gap sits between, with the dragged page skipped: it is @@ -471,7 +498,7 @@ function TreeBranch({ ? 'Drag onto a page to file it under it, or between two rows to sort it there' : undefined } - className={`flex items-center rounded-md ${ + className={`group/row flex items-center rounded-md ${ dnd.enabled ? 'cursor-grab select-none active:cursor-grabbing' : '' } ${ sameTarget(dnd.over, { kind: 'page', id: node.slug }) @@ -526,6 +553,17 @@ function TreeBranch({ /> ) : null} + {/* The keyboard's and the touch screen's way to the same move + the drag does. Only signed in: the menu publishes an event, + and an entry that cannot lead anywhere is worse than none. */} + {moves.enabled ? ( + moves.onMove(node, target)} + /> + ) : null} {open ? ( @@ -537,6 +575,7 @@ function TreeBranch({ forcedOpen={forcedOpen} onToggle={onToggle} dnd={dnd} + moves={moves} /> ) : null} diff --git a/src/ui/move-page.ts b/src/ui/move-page.ts index 315ffba..27c85cc 100644 --- a/src/ui/move-page.ts +++ b/src/ui/move-page.ts @@ -3,6 +3,8 @@ import { classifyRejection } from '../nostr/client' import { publishPlacement } from '../nostr/publish-placement' import { useSession } from '../session/session' import { canMoveUnder } from '../domain/pages' +import { planMove } from '../domain/move-tree' +import type { MoveDirection, TreeMove } from '../domain/move-tree' import type { Page } from '../domain/pages' /** @@ -95,3 +97,48 @@ export function useMovePage(relayUrl: string, groupId: string, pages: Page[]): M return { move, busySlug, error, setError, signedIn: session.status === 'signed-in' } } + +/** + * The four entries of the move menu, with the names of the pages they would + * move past — so the menu reads as what will happen rather than as four bare + * directions. A move that has nowhere to go keeps its generic label: there is + * no page to name, and the entry is drawn disabled anyway. + * + * Here rather than in `PageMoveMenu.tsx` because these are words, not a + * component, and next to `useMovePage` because the two answer the same question + * from opposite ends: this one what a move would do, that one what the relay + * said when it was done. The arithmetic is in `src/domain/move-tree.ts`. + */ +const MOVE_LABEL: Record = { + up: 'Move up', + down: 'Move down', + in: 'Move in', + out: 'Move out', +} + +const MOVE_ORDER: MoveDirection[] = ['up', 'down', 'in', 'out'] + +export type MoveEntry = { + direction: MoveDirection + label: string + /** null when the move has nowhere to go */ + move: TreeMove | null +} + +export function moveEntries(pages: Page[], slug: string): MoveEntry[] { + const page = pages.find((entry) => entry.slug === slug) ?? null + const parent = page?.parentSlug + ? (pages.find((entry) => entry.slug === page.parentSlug) ?? null) + : null + + return MOVE_ORDER.map((direction) => { + const move = planMove(pages, slug, direction) + let label = MOVE_LABEL[direction] + if (move && direction === 'in') { + const into = pages.find((entry) => entry.slug === move.parentSlug) + if (into) label = `Move under ${into.title}` + } + if (move && direction === 'out' && parent) label = `Move out of ${parent.title}` + return { direction, label, move } + }) +} From aab9f674887c6a06e50d78b9ee8ecfd583ab6e47 Mon Sep 17 00:00:00 2001 From: M <> Date: Mon, 14 Sep 2026 14:24:48 +0200 Subject: [PATCH 2/3] =?UTF-8?q?CON-15:=20"Move=20to=E2=80=A6",=20for=20the?= =?UTF-8?q?=20destination=20four=20steps=20cannot=20reach?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four steps reach a neighbour. Filing a page under one at the other end of the wiki is one drag with a mouse and, without one, a dozen repeats of "move down" — so the menu gets a second panel: every page the move may land under plus the top level, in tree order, filterable by name. This is the one thing dragging has no equivalent of, and deliberately so: a drag can only end where the pointer can reach, a list can name a row that is scrolled away or folded shut. Left out are the page itself, its own subtree and the parent it already has — the same three a drop refuses, refused here for the same reason rather than reported as an error after the click. Picking a destination writes no order key, the same as "in" and the same as dropping onto that row. docs/02 said this of the drop; it now says it of choosing a parent, however the parent was chosen. Co-Authored-By: Claude Opus 5 --- docs/02-data-model-events.md | 10 +- docs/06-ui-information-architecture.md | 35 +++++- src/ui/PageMoveMenu.test.tsx | 74 +++++++++++- src/ui/PageMoveMenu.tsx | 155 ++++++++++++++++++------- src/ui/move-page.ts | 34 +++++- 5 files changed, 258 insertions(+), 50 deletions(-) diff --git a/docs/02-data-model-events.md b/docs/02-data-model-events.md index 609b0df..3686347 100644 --- a/docs/02-data-model-events.md +++ b/docs/02-data-model-events.md @@ -249,9 +249,13 @@ extension sorts after it — so a gap can always be subdivided, however often. Consequences worth knowing: -- Dropping a page **onto** another one clears its key: in its new level it - sorts by title until somebody drags it into place. That is predictable, and - it avoids carrying a key from a level where it meant something else. +- Choosing a **parent** clears the key: in its new level the page sorts by + title until somebody puts it in place. That is predictable, and it avoids + carrying a key from a level where it meant something else. It holds for every + way of choosing one — dropping the page onto a row, and the move menu's "in" + and "Move to…" alike ([06](06-ui-information-architecture.md)) — so the tree + does not depend on which input device moved the page. A key is written only + where a position within a level is actually chosen. - Two pages whose titles normalise identically compare equal; the slug breaks the tie, so every client shows the same order. - **Open:** the order is per page, so a *level* cannot be sorted in one go, and diff --git a/docs/06-ui-information-architecture.md b/docs/06-ui-information-architecture.md index a443d25..ba6201c 100644 --- a/docs/06-ui-information-architecture.md +++ b/docs/06-ui-information-architecture.md @@ -192,9 +192,38 @@ suffocates in a column that narrow, so it gets `max-w-4xl`. The page itself carries **no** move action any more: a picker on the page was tried and dropped again — dragging in the tree says where a page ends up - far better than a list of slugs can. **Open:** moving therefore has no - keyboard path, and none on a touch screen either, where HTML5 drag & drop - does not fire. + far better than a list of slugs can. + + **Without a pointer that can drag (CON-15).** Dragging stays the primary + gesture, but it is not a reachable one: HTML5 drag & drop has no keyboard + path at all, and on a touch screen it does not fire even once — there, + moving a page was impossible rather than awkward. Every row therefore + carries a **move button** (`src/ui/PageMoveMenu.tsx`), a real button in the + tab order and under a finger, opening a menu with two panels: + + - the four steps every outline editor has — **up**, **down**, **in** under + the sibling above, **out** to directly behind the parent. Each names the + page it moves past ("Move under Handbook"), and a step with nowhere to go + is drawn disabled rather than reporting an error after the click. Where + each one lands is `src/domain/move-tree.ts`. + - **Move to…**, the list of every page it may be filed under plus the top + level, in tree order and filterable by name. This is what dragging has no + equivalent of, deliberately: a drag can only end where the pointer can + reach, a list can name a row that is scrolled away or folded shut. Left + out are the page itself, its own subtree and the parent it already has — + the same three a drop refuses. + + A step that picks a *parent* ("in", "Move to…") writes **no** order key and + lets the new level sort by title, which is exactly what dropping onto that + row does. The tree must not look different depending on whether a mouse or + the keyboard moved the page. A key is written only where a position within a + level is genuinely being chosen: up, down and out. + + The trigger is hidden until the row is hovered or something inside it is + focused, like the "+" on the Pages heading — the tree is read far more often + than it is rearranged. A coarse pointer has no hover to reveal it with, so + `pointer-coarse` leaves it permanently visible there; without that line the + touch half of this would have shipped invisible. 4. **Settings** — one row to `/settings/spaces`, the entry into the settings hub (below). Stays visible even while already inside `/settings/*`, where the bar shows the hub's own nav (Profile/Spaces) instead of zones 2 and 3. diff --git a/src/ui/PageMoveMenu.test.tsx b/src/ui/PageMoveMenu.test.tsx index eff3f94..8c5cc39 100644 --- a/src/ui/PageMoveMenu.test.tsx +++ b/src/ui/PageMoveMenu.test.tsx @@ -7,7 +7,7 @@ import type { Page } from '../domain/pages' import type { Revision } from '../domain/revision' import type { TreeMove } from '../domain/move-tree' import { PageMoveMenu } from './PageMoveMenu' -import { moveEntries } from './move-page' +import { moveEntries, moveTargets } from './move-page' /** * The menu is the only way to move a page without a pointer that can drag, so @@ -47,6 +47,17 @@ function render(page: Page, pages = PAGES, onMove: (move: TreeMove) => void = vi return host } +/** + * Typing into a controlled input. Assigning `value` alone never reaches React: + * it tracks the value through its own setter and reads an unchanged one as no + * change, so the native setter has to be called before the event is fired. + */ +function typeInto(field: HTMLInputElement, value: string) { + const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')!.set! + setter.call(field, value) + field.dispatchEvent(new Event('input', { bubbles: true })) +} + const pageOf = (slug: string, pages = PAGES) => pages.find((page) => page.slug === slug)! const trigger = (host: HTMLElement) => host.querySelector('button')! const items = (host: HTMLElement) => [...host.querySelectorAll('[role="menuitem"]')] as HTMLButtonElement[] @@ -80,7 +91,7 @@ describe('PageMoveMenu', () => { expect(trigger(host).disabled).toBe(false) }) - it('opens on a click and offers the four steps', () => { + it('opens on a click and offers the four steps, plus the long way', () => { const host = render(pageOf('b')) act(() => trigger(host).click()) expect(items(host).map((item) => item.textContent)).toEqual([ @@ -88,6 +99,7 @@ describe('PageMoveMenu', () => { 'Move down', 'Move under A', 'Move out', + 'Move to…', ]) }) @@ -143,4 +155,62 @@ describe('PageMoveMenu', () => { expect(trigger(host).disabled).toBe(true) expect(trigger(host).title).toContain('nowhere to move it') }) + + it('offers every page it may land under, and the top level when it is nested', () => { + const host = render(pageOf('x')) + act(() => trigger(host).click()) + act(() => items(host).find((item) => item.textContent === 'Move to…')!.click()) + expect(items(host).map((item) => item.textContent)).toEqual(['Top level', 'A']) + }) + + it('files the page under the destination that was picked', () => { + const onMove = vi.fn() + const host = render(pageOf('x'), PAGES, onMove) + act(() => trigger(host).click()) + act(() => items(host).find((item) => item.textContent === 'Move to…')!.click()) + act(() => items(host).find((item) => item.textContent === 'A')!.click()) + // a parent, not a position: the new level sorts it by title, exactly as a + // drop onto that row would + expect(onMove).toHaveBeenCalledWith({ parentSlug: 'a', order: null }) + }) + + it('filters the destinations by name, and says so when none is left', () => { + const host = render(pageOf('x')) + act(() => trigger(host).click()) + act(() => items(host).find((item) => item.textContent === 'Move to…')!.click()) + act(() => typeInto(host.querySelector('input')!, 'zz')) + expect(items(host)).toHaveLength(0) + expect(host.textContent).toContain('no page matches') + }) + + it('closes the whole menu on Escape, not just the destination list', () => { + const host = render(pageOf('x')) + act(() => trigger(host).click()) + act(() => items(host).find((item) => item.textContent === 'Move to…')!.click()) + act(() => { + host.querySelector('input')!.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }), + ) + }) + expect(host.querySelector('[role="menu"]')).toBeNull() + expect(document.activeElement).toBe(trigger(host)) + }) +}) + +describe('moveTargets', () => { + it('leaves out the page itself, its own subtree and the parent it has', () => { + const deep = tree(['a', null], ['b', null], ['x', 'b'], ['deep', 'x']) + // for `b`: not itself, not x or deep (its subtree), and it has no parent + expect(moveTargets(deep, 'b').map((target) => target.slug)).toEqual(['a']) + }) + + it('offers the top level only to a page that is not already there', () => { + expect(moveTargets(PAGES, 'x')[0]).toMatchObject({ slug: null, title: 'Top level' }) + expect(moveTargets(PAGES, 'a').some((target) => target.slug === null)).toBe(false) + }) + + it('has nothing to offer when the space holds a single page', () => { + const alone = tree(['a', null]) + expect(moveTargets(alone, 'a')).toEqual([]) + }) }) diff --git a/src/ui/PageMoveMenu.tsx b/src/ui/PageMoveMenu.tsx index c5932a4..f5f9b03 100644 --- a/src/ui/PageMoveMenu.tsx +++ b/src/ui/PageMoveMenu.tsx @@ -1,8 +1,9 @@ import { useEffect, useId, useRef, useState } from 'react' import type { TreeMove } from '../domain/move-tree' import type { Page } from '../domain/pages' -import { moveEntries } from './move-page' -import { MoveIcon } from './icons' +import { moveEntries, moveTargets } from './move-page' +import { INPUT } from './controls' +import { MoveIcon, PageIcon } from './icons' /** * The keyboard's and the touch screen's way of moving a page. @@ -14,9 +15,10 @@ import { MoveIcon } from './icons' * awkward, it was impossible. * * One menu answers both. Its trigger is a real button, so it is in the tab - * order and under a finger; its entries are the four steps an outline editor - * has. Reimplementing the drag on pointer events would have given touch a - * gesture and the keyboard still nothing. + * order and under a finger. It has two panels: the four steps an outline + * editor has, for a neighbour, and a list of every page the move may land + * under, for the other end of the wiki. Reimplementing the drag on pointer + * events would have given touch a gesture and the keyboard still nothing. * * The trigger is hidden until the row is hovered or something inside it is * focused — the tree is read far more often than it is rearranged, the same @@ -35,36 +37,56 @@ export function PageMoveMenu({ onMove: (move: TreeMove) => void }) { const [open, setOpen] = useState(false) + /** the four steps, or the list of destinations */ + const [picking, setPicking] = useState(false) + const [filter, setFilter] = useState('') const triggerRef = useRef(null) const menuRef = useRef(null) const menuId = useId() const entries = moveEntries(pages, page.slug) - const available = entries.some((entry) => entry.move !== null) + const targets = moveTargets(pages, page.slug) + const available = entries.some((entry) => entry.move !== null) || targets.length > 0 - const close = (returnFocus: boolean) => { + const needle = filter.trim().toLowerCase() + const shown = needle ? targets.filter((t) => t.title.toLowerCase().includes(needle)) : targets + + const close = () => { setOpen(false) - if (returnFocus) triggerRef.current?.focus() + setPicking(false) + setFilter('') + triggerRef.current?.focus() + } + + const send = (move: TreeMove) => { + close() + onMove(move) } - // Opening puts the focus on the first entry that can actually be used: a - // menu that opens with nothing focused is a menu the keyboard has to find - // its way into first. + // Opening puts the focus inside: on the first usable entry, or on the filter + // field once the list is up. A menu that opens with nothing focused is one + // the keyboard has to find its way into first. useEffect(() => { if (!open) return - const first = menuRef.current?.querySelector('button:not([disabled])') - first?.focus() - }, [open]) + const field = menuRef.current?.querySelector('input') + if (field) { + field.focus() + return + } + menuRef.current?.querySelector('button:not([disabled])')?.focus() + }, [open, picking]) - // Clicking anywhere else closes it. `pointerdown` rather than `click`, so - // the menu is gone before whatever was clicked reacts — otherwise a click on - // another row's trigger closes this menu and opens nothing. + // Clicking anywhere else closes it. `pointerdown` rather than `click`, so it + // is gone before whatever was clicked reacts — otherwise a click on another + // row's trigger closes this menu and opens nothing. useEffect(() => { if (!open) return const onPointerDown = (event: PointerEvent) => { const target = event.target as Node if (menuRef.current?.contains(target) || triggerRef.current?.contains(target)) return setOpen(false) + setPicking(false) + setFilter('') } window.addEventListener('pointerdown', onPointerDown) return () => window.removeEventListener('pointerdown', onPointerDown) @@ -72,23 +94,32 @@ export function PageMoveMenu({ const onMenuKeyDown = (event: React.KeyboardEvent) => { if (event.key === 'Escape') { + // Not swallowed by the sidebar or a parent: this is the innermost thing + // Escape can mean while the menu is up. event.stopPropagation() - close(true) + close() return } if (event.key !== 'ArrowDown' && event.key !== 'ArrowUp') return event.preventDefault() - const items = [...(menuRef.current?.querySelectorAll('button') ?? [])].filter( - (item) => !item.disabled, - ) + const items = [ + ...(menuRef.current?.querySelectorAll('button[role="menuitem"]') ?? []), + ].filter((item) => !item.disabled) if (items.length === 0) return const at = items.indexOf(document.activeElement as HTMLButtonElement) const step = event.key === 'ArrowDown' ? 1 : -1 - // Wrapping, because a four-entry menu with two of them disabled is one - // keypress from either end whichever way it wraps. + // Wrapping: a four-entry menu with two of them disabled is one keypress + // from either end whichever way it wraps. From the filter field (index -1) + // ArrowDown therefore lands on the first row, which is what it looks like + // it should do. items[(at + step + items.length) % items.length]!.focus() } + const itemClass = + 'flex h-8 w-full items-center gap-2 rounded-md px-2.5 text-left text-sm text-fg-muted ' + + 'hover:bg-surface-hover hover:text-fg focus-visible:bg-surface-hover focus-visible:text-fg ' + + 'disabled:pointer-events-none disabled:opacity-40' + return (
- ))} + {picking ? ( + <> + {/* A wiki has more pages than fit in a popup, and scrolling a + list of them with the keyboard is the slow way to a page + whose name is already known. */} + setFilter(event.target.value)} + placeholder="Filter pages…" + aria-label={`Filter destinations for ${page.title}`} + className={`${INPUT} mb-1`} + /> +
+ {shown.length === 0 ? ( +
no page matches
+ ) : ( + shown.map((target) => ( + + )) + )} +
+ + ) : ( + <> + {entries.map((entry) => ( + + ))} +
+ + + )}
) : null}
diff --git a/src/ui/move-page.ts b/src/ui/move-page.ts index 27c85cc..9235653 100644 --- a/src/ui/move-page.ts +++ b/src/ui/move-page.ts @@ -2,7 +2,7 @@ import { useState } from 'react' import { classifyRejection } from '../nostr/client' import { publishPlacement } from '../nostr/publish-placement' import { useSession } from '../session/session' -import { canMoveUnder } from '../domain/pages' +import { buildTree, canMoveUnder, flattenTree } from '../domain/pages' import { planMove } from '../domain/move-tree' import type { MoveDirection, TreeMove } from '../domain/move-tree' import type { Page } from '../domain/pages' @@ -142,3 +142,35 @@ export function moveEntries(pages: Page[], slug: string): MoveEntry[] { return { direction, label, move } }) } + +/** A page the move menu offers as a new parent, at its depth in the tree. */ +export type MoveTarget = { slug: string | null; title: string; depth: number } + +/** + * Every page `slug` may be filed under, plus the top level, in tree order. + * + * The four steps reach a neighbour; this reaches the other end of the wiki, + * which with a mouse is one drag and without one would otherwise be a dozen + * repeats of "move down". Dragging has no equivalent of it — that is the point, + * not an oversight: a drag can only end somewhere the pointer can get to, and a + * list can name a row that is scrolled away or folded shut. + * + * What is left out is left out for the same reasons a drop is refused rather + * than reported as an error afterwards: the page itself, its own subtree (the + * branch would point into itself and drop out of the tree), and the parent it + * already has, because filing it there publishes an event that changes nothing. + */ +export function moveTargets(pages: Page[], slug: string): MoveTarget[] { + const page = pages.find((entry) => entry.slug === slug) + if (!page) return [] + + const targets: MoveTarget[] = [] + if (page.parentSlug !== null) targets.push({ slug: null, title: 'Top level', depth: 0 }) + + for (const node of flattenTree(buildTree(pages))) { + if (node.slug === slug || node.slug === page.parentSlug) continue + if (!canMoveUnder(pages, slug, node.slug)) continue + targets.push({ slug: node.slug, title: node.title, depth: node.depth + 1 }) + } + return targets +} From b42b1a10ec054b0f0eee650c8cbf7aed2070a3f8 Mon Sep 17 00:00:00 2001 From: M <> Date: Mon, 14 Sep 2026 21:37:50 +0200 Subject: [PATCH 3/3] CON-15: a step that would move nothing is disabled, not silently dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two siblings share an order key whenever their titles normalise identically and neither has been placed by hand. `keyBetween` answers "behind the pair" for equal bounds — right for a drop, wrong for a step, which asks to overtake one row and would pass both. The step back then computed the very same key, so the menu entry looked enabled and did nothing when clicked, because `useMovePage` drops a no-op placement without a word. A tie above or below is now stepped over as a whole run, and any plan that lands the page on the placement it already has is reported as "nowhere to go" — the same answer the first and last row of a level already get, and the menu greys it out before it is tried. --- docs/02-data-model-events.md | 6 +- docs/06-ui-information-architecture.md | 23 +- src/domain/move-tree.test.ts | 30 +++ src/domain/move-tree.ts | 113 ++++++---- src/ui/PageMoveMenu.test.tsx | 181 +++++++++++++-- src/ui/PageMoveMenu.tsx | 298 +++++++++++++++++-------- src/ui/layout/Sidebar.test.tsx | 170 ++++++++++++++ src/ui/move-page.ts | 33 ++- 8 files changed, 693 insertions(+), 161 deletions(-) create mode 100644 src/ui/layout/Sidebar.test.tsx diff --git a/docs/02-data-model-events.md b/docs/02-data-model-events.md index 3686347..15fd165 100644 --- a/docs/02-data-model-events.md +++ b/docs/02-data-model-events.md @@ -257,7 +257,11 @@ Consequences worth knowing: does not depend on which input device moved the page. A key is written only where a position within a level is actually chosen. - Two pages whose titles normalise identically compare equal; the slug breaks - the tie, so every client shows the same order. + the tie, so every client shows the same order. A position *between* two of + them is then not expressible as a key at all, which the move menu's up and + down have to answer for: they step past the whole run of equal keys rather + than into it. One row further than asked, and a move — the alternative is a + key the page already has, published as nothing at all. - **Open:** the order is per page, so a *level* cannot be sorted in one go, and reordering needs a signature per page moved. - **Open:** nothing cleans up the placement of a deleted page. A `31818` whose diff --git a/docs/06-ui-information-architecture.md b/docs/06-ui-information-architecture.md index ba6201c..94a7110 100644 --- a/docs/06-ui-information-architecture.md +++ b/docs/06-ui-information-architecture.md @@ -202,10 +202,15 @@ suffocates in a column that narrow, so it gets `max-w-4xl`. tab order and under a finger, opening a menu with two panels: - the four steps every outline editor has — **up**, **down**, **in** under - the sibling above, **out** to directly behind the parent. Each names the - page it moves past ("Move under Handbook"), and a step with nowhere to go - is drawn disabled rather than reporting an error after the click. Where - each one lands is `src/domain/move-tree.ts`. + the sibling above, **out** to directly behind the parent. The two that + change a page's parent name it ("Move under Handbook", "Move out of + Handbook"); up and down stay generic, because the row they pass is the + one directly above or below and pointing at it adds nothing. A step with + nowhere to go is drawn disabled rather than reporting an error after the + click — and so is one that would land the page exactly where it already + hangs, which `useMovePage` would otherwise drop without a word, leaving an + enabled entry that does nothing. Where each one lands is + `src/domain/move-tree.ts`. - **Move to…**, the list of every page it may be filed under plus the top level, in tree order and filterable by name. This is what dragging has no equivalent of, deliberately: a drag can only end where the pointer can @@ -224,6 +229,16 @@ suffocates in a column that narrow, so it gets `max-w-4xl`. than it is rearranged. A coarse pointer has no hover to reveal it with, so `pointer-coarse` leaves it permanently visible there; without that line the touch half of this would have shipped invisible. + + The open panel is **portalled into the body** and positioned against the + window rather than drawn inside the row, for two reasons that both bite + exactly where the menu matters most. The tree scrolls in an + `overflow-y-auto` container, which clips anything absolutely positioned + inside it: a row in the lower part of the bar would open a menu with its + lower half — the destination list — cut away. And every row is a + `draggable` element, so a press inside the menu (selecting filter text, + sliding onto an entry) would be handed to the row as the start of a drag. + The panel flips above the row when the window has no room below it. 4. **Settings** — one row to `/settings/spaces`, the entry into the settings hub (below). Stays visible even while already inside `/settings/*`, where the bar shows the hub's own nav (Profile/Spaces) instead of zones 2 and 3. diff --git a/src/domain/move-tree.test.ts b/src/domain/move-tree.test.ts index 94c404a..f4effd4 100644 --- a/src/domain/move-tree.test.ts +++ b/src/domain/move-tree.test.ts @@ -63,6 +63,18 @@ function after(pages: Page[], slug: string, direction: MoveDirection): string[] const FLAT = tree(['a', null], ['b', null], ['c', null], ['d', null]) +/** + * A level whose three titles all normalise to the same key, so all three sort + * equal and only the slug tells them apart (docs/02). Reachable without + * trying: a rename keeps the page's slug (`src/ui/PageEditor.tsx`), so two + * pages can end up named the same thing. + */ +const TIED = buildPages([ + rev({ id: 'a1', title: 'Setup' }), + rev({ id: 'b2', title: 'setup' }), + rev({ id: 'c3', title: 'SETUP' }), +]) + describe('siblingsOf', () => { it('keeps the level in the order the tree draws it', () => { expect(siblingsOf(FLAT, null).map((page) => page.slug)).toEqual(['a', 'b', 'c', 'd']) @@ -105,6 +117,24 @@ describe('planMove — up and down', () => { }) }) +describe('planMove — siblings that share an order key', () => { + it('steps past the whole run rather than landing in a gap that is not there', () => { + // One row would be the better answer and is not expressible as a key: all + // three sort equal, so there is nothing between them to aim at. + expect(after(TIED, 'a1', 'down')).toEqual(['b2', 'c3', 'a1']) + }) + + it('never hands back the key the page already has', () => { + const moved = applyMove(TIED, 'a1', 'down') + const back = planMove(moved, 'a1', 'up') + // The step that publishes nothing is the one that hurts: `useMovePage` + // drops a move that changes neither parent nor key silently, so the entry + // looks enabled and does nothing at all. + expect(back?.order).not.toBe(moved.find((page) => page.slug === 'a1')!.order) + expect(after(moved, 'a1', 'up')).toEqual(['a1', 'b2', 'c3']) + }) +}) + describe('planMove — in and out', () => { it('files the page under the sibling above it', () => { expect(after(FLAT, 'b', 'in')).toEqual(['a', ' b', 'c', 'd']) diff --git a/src/domain/move-tree.ts b/src/domain/move-tree.ts index 1726ac2..d6f44e0 100644 --- a/src/domain/move-tree.ts +++ b/src/domain/move-tree.ts @@ -21,7 +21,7 @@ export type MoveDirection = | 'up' /** swap with the sibling below */ | 'down' - /** become the last child of the sibling above */ + /** become a child of the sibling above (sorted by title, like a drop onto it) */ | 'in' /** leave the parent and follow directly behind it */ | 'out' @@ -51,9 +51,10 @@ export function siblingsOf(pages: Page[], parentSlug: string | null): Page[] { * Where `slug` would land, or `null` when the move has nowhere to go: the top * row of a level cannot go up, the bottom row cannot go down, a page at the * root cannot come further out, and a page with no sibling above it has - * nothing to move in under. The menu disables exactly those entries, so an + * nothing to move in under — and when the step would land the page exactly + * where it already hangs. The menu disables exactly those entries, so an * unavailable move is visible before it is tried rather than reported as an - * error afterwards. + * error afterwards, or worse, silently dropped by `useMovePage`. * * A caller still has to check `canMoveUnder`: not for these four — none of * them can reach into the page's own subtree, because a sibling and a parent @@ -71,44 +72,78 @@ export function planMove(pages: Page[], slug: string, direction: MoveDirection): const previous = level[index - 1] const next = level[index + 1] - switch (direction) { - case 'up': { - if (!previous) return null - // Between the two rows above it: the one it swaps with, and whatever is - // above *that*. Writing "the key of the row above" would collide rather - // than overtake. - const above = level[index - 2] - return { - parentSlug: page.parentSlug, - order: keyBetween(above ? orderKeyOf(above) : null, orderKeyOf(previous)), + const move = ((): TreeMove | null => { + switch (direction) { + case 'up': { + if (!previous) return null + // Between the two rows above it: the one it swaps with, and whatever is + // above *that*. Writing "the key of the row above" would collide rather + // than overtake. + const above = level[index - 2] + const ceiling = above ? orderKeyOf(above) : null + return { + parentSlug: page.parentSlug, + // A tie above it has no gap to land in (see `tied`), so the step + // goes in front of the whole run instead. Further than one row, but + // it is a move; asking for the gap would hand back the key the page + // already has. + order: tied(ceiling, orderKeyOf(previous)) + ? keyBetween(null, orderKeyOf(previous)) + : keyBetween(ceiling, orderKeyOf(previous)), + } } - } - case 'down': { - if (!next) return null - const below = level[index + 2] - return { - parentSlug: page.parentSlug, - order: keyBetween(orderKeyOf(next), below ? orderKeyOf(below) : null), + case 'down': { + if (!next) return null + const below = level[index + 2] + const floor = below ? orderKeyOf(below) : null + return { + parentSlug: page.parentSlug, + order: tied(orderKeyOf(next), floor) + ? keyBetween(orderKeyOf(next), null) + : keyBetween(orderKeyOf(next), floor), + } } - } - case 'in': { - if (!previous) return null - // No key: this step chooses a parent, not a position, and dropping the - // same page onto the same row with a mouse chooses no position either. - // See `TreeMove` — one destination, one result, whatever moved it. - return { parentSlug: previous.slug, order: null } - } - case 'out': { - const parent = pages.find((entry) => entry.slug === page.parentSlug) - if (!parent) return null - // Directly behind the parent in the parent's own level, which is where - // the row visually already is — the step is outwards, not downwards. - const uncles = siblingsOf(pages, parent.parentSlug) - const after = uncles[uncles.findIndex((entry) => entry.slug === parent.slug) + 1] - return { - parentSlug: parent.parentSlug, - order: keyBetween(orderKeyOf(parent), after ? orderKeyOf(after) : null), + case 'in': { + if (!previous) return null + // No key: this step chooses a parent, not a position, and dropping the + // same page onto the same row with a mouse chooses no position either. + // See `TreeMove` — one destination, one result, whatever moved it. + return { parentSlug: previous.slug, order: null } + } + case 'out': { + const parent = pages.find((entry) => entry.slug === page.parentSlug) + if (!parent) return null + // Directly behind the parent in the parent's own level, which is where + // the row visually already is — the step is outwards, not downwards. + const uncles = siblingsOf(pages, parent.parentSlug) + const after = uncles[uncles.findIndex((entry) => entry.slug === parent.slug) + 1] + return { + parentSlug: parent.parentSlug, + order: keyBetween(orderKeyOf(parent), after ? orderKeyOf(after) : null), + } } } - } + })() + + // A step that publishes the placement the page already has is worse than one + // that is greyed out: `useMovePage` drops it as a no-op without a word, so + // the entry looks enabled and does nothing at all when clicked. Reporting it + // as "nowhere to go" is the same answer the first and last row of a level + // already get. + if (move && move.parentSlug === page.parentSlug && move.order === page.order) return null + return move +} + +/** + * Whether two neighbouring keys leave no gap between them. + * + * `keyBetween` answers "behind the pair" for equal bounds, which is the right + * answer for a drop — a gesture that pointed at a place between two rows that + * do not have one. For a *step* it is not: the page is asked to overtake one + * row and would pass both, and the step back then computes the very same key + * and moves nothing. Two siblings share a key whenever their titles normalise + * identically and neither has been placed by hand (docs/02). + */ +function tied(before: string | null, after: string | null): boolean { + return before !== null && after !== null && before >= after } diff --git a/src/ui/PageMoveMenu.test.tsx b/src/ui/PageMoveMenu.test.tsx index 8c5cc39..2516911 100644 --- a/src/ui/PageMoveMenu.test.tsx +++ b/src/ui/PageMoveMenu.test.tsx @@ -1,11 +1,13 @@ // @vitest-environment jsdom -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { act } from 'react' import { createRoot } from 'react-dom/client' +import type { Root } from 'react-dom/client' import { buildPages } from '../domain/pages' import type { Page } from '../domain/pages' import type { Revision } from '../domain/revision' import type { TreeMove } from '../domain/move-tree' +import { planMove } from '../domain/move-tree' import { PageMoveMenu } from './PageMoveMenu' import { moveEntries, moveTargets } from './move-page' @@ -37,12 +39,26 @@ function tree(...spec: [slug: string, parent: string | null][]): Page[] { const PAGES = tree(['a', null], ['b', null], ['x', 'b']) -function render(page: Page, pages = PAGES, onMove: (move: TreeMove) => void = vi.fn()) { +/** + * Every root is torn down again afterwards. An open menu of a root left + * mounted keeps listening for the pointerdown that dismisses it, and its panel + * lives in `document.body` rather than in the host — emptying the body under + * it leaves React holding a node it can no longer remove. + */ +const mounted: Root[] = [] + +function render( + page: Page, + pages = PAGES, + onMove: (move: TreeMove) => void = vi.fn(), + busy = false, +) { const host = document.createElement('div') document.body.appendChild(host) const root = createRoot(host) + mounted.push(root) act(() => { - root.render() + root.render() }) return host } @@ -60,7 +76,17 @@ function typeInto(field: HTMLInputElement, value: string) { const pageOf = (slug: string, pages = PAGES) => pages.find((page) => page.slug === slug)! const trigger = (host: HTMLElement) => host.querySelector('button')! -const items = (host: HTMLElement) => [...host.querySelectorAll('[role="menuitem"]')] as HTMLButtonElement[] +/** + * The open panel is portalled into `document.body` and is deliberately not + * below the row any more (`PageMoveMenu.tsx`), so everything inside it is + * looked for in the document rather than in the host the row was rendered + * into. + */ +const panel = () => document.querySelector('[role="menu"],[role="dialog"]') as HTMLElement | null +const items = () => [...document.querySelectorAll('[role="menuitem"]')] as HTMLButtonElement[] +const field = () => document.querySelector('input') as HTMLInputElement +const press = (target: Element, key: string) => + target.dispatchEvent(new KeyboardEvent('keydown', { key, bubbles: true })) describe('moveEntries', () => { it('names the page a move would land under or come out of', () => { @@ -84,6 +110,13 @@ describe('PageMoveMenu', () => { document.body.innerHTML = '' }) + afterEach(() => { + act(() => { + for (const root of mounted.splice(0)) root.unmount() + }) + document.body.innerHTML = '' + }) + it('is a button, not a gesture — the whole point of it', () => { const host = render(pageOf('b')) expect(trigger(host).getAttribute('aria-label')).toBe('Move B') @@ -94,7 +127,7 @@ describe('PageMoveMenu', () => { it('opens on a click and offers the four steps, plus the long way', () => { const host = render(pageOf('b')) act(() => trigger(host).click()) - expect(items(host).map((item) => item.textContent)).toEqual([ + expect(items().map((item) => item.textContent)).toEqual([ 'Move up', 'Move down', 'Move under A', @@ -106,7 +139,7 @@ describe('PageMoveMenu', () => { it('disables the steps that lead nowhere instead of failing on the click', () => { const host = render(pageOf('b')) act(() => trigger(host).click()) - const [up, down, moveIn, out] = items(host) + const [up, down, moveIn, out] = items() expect(up!.disabled).toBe(false) // B can swap with A expect(down!.disabled).toBe(true) // nothing below it expect(moveIn!.disabled).toBe(false) @@ -117,21 +150,55 @@ describe('PageMoveMenu', () => { const onMove = vi.fn() const host = render(pageOf('x'), PAGES, onMove) act(() => trigger(host).click()) - const out = items(host).find((item) => item.textContent?.startsWith('Move out'))! + const out = items().find((item) => item.textContent?.startsWith('Move out'))! act(() => out.click()) expect(onMove).toHaveBeenCalledWith({ parentSlug: null, order: expect.any(String) }) - expect(items(host)).toHaveLength(0) + expect(items()).toHaveLength(0) + }) + + it('hangs the open menu off the body, not off the row', () => { + // The tree scrolls in an `overflow-y-auto` container and every row is a + // drag source. Inside either, the panel is clipped at the bottom of the + // bar and a press in it starts a drag — so it is portalled out. + const host = render(pageOf('b')) + act(() => trigger(host).click()) + expect(panel()!.parentElement).toBe(document.body) + expect(host.contains(panel())).toBe(false) + }) + + it('hands up the step the domain planned, for each of the four', () => { + const level = tree(['a', null], ['b', null], ['c', null]) + for (const [label, direction] of [ + ['Move up', 'up'], + ['Move down', 'down'], + ['Move under A', 'in'], + ] as const) { + const onMove = vi.fn() + const host = render(pageOf('b', level), level, onMove) + act(() => trigger(host).click()) + act(() => items().find((item) => item.textContent === label)!.click()) + expect(onMove).toHaveBeenCalledWith(planMove(level, 'b', direction)) + act(() => mounted.pop()!.unmount()) + host.remove() + } + }) + + it('opens on ArrowDown, so the keyboard never has to guess at a click', () => { + const host = render(pageOf('b')) + act(() => press(trigger(host), 'ArrowDown')) + expect(items()).not.toHaveLength(0) + expect(document.activeElement).toBe(items()[0]) }) it('closes on Escape and gives the focus back to the trigger', () => { const host = render(pageOf('b')) act(() => trigger(host).click()) act(() => { - items(host)[0]!.dispatchEvent( + items()[0]!.dispatchEvent( new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }), ) }) - expect(items(host)).toHaveLength(0) + expect(items()).toHaveLength(0) expect(document.activeElement).toBe(trigger(host)) }) @@ -139,7 +206,7 @@ describe('PageMoveMenu', () => { const host = render(pageOf('b')) act(() => trigger(host).click()) // opening already focused the first usable entry - expect(document.activeElement).toBe(items(host)[0]) + expect(document.activeElement).toBe(items()[0]) act(() => { document.activeElement!.dispatchEvent( new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), @@ -149,6 +216,41 @@ describe('PageMoveMenu', () => { expect(document.activeElement?.textContent).toBe('Move under A') }) + it('reaches the last destination with ArrowUp out of the filter field', () => { + // The field is not a menu entry, so "one step up from where I am" has no + // meaning there — counting from index -1 stopped one short of the end. + const host = render(pageOf('x')) + act(() => trigger(host).click()) + act(() => items().find((item) => item.textContent === 'Move to…')!.click()) + expect(document.activeElement).toBe(field()) + act(() => press(field(), 'ArrowUp')) + expect(document.activeElement).toBe(items()[items().length - 1]) + }) + + it('keeps the focus when a click elsewhere closes it', () => { + const host = render(pageOf('b')) + act(() => trigger(host).click()) + const outside = document.body.appendChild(document.createElement('div')) + act(() => { + outside.dispatchEvent(new Event('pointerdown', { bubbles: true })) + }) + expect(items()).toHaveLength(0) + // Not on : the keyboard would otherwise start over at the top of + // the page after every dismissed menu. + expect(document.activeElement).toBe(trigger(host)) + }) + + it('opens at the four steps again, with the filter it was left with cleared', () => { + const host = render(pageOf('x')) + act(() => trigger(host).click()) + act(() => items().find((item) => item.textContent === 'Move to…')!.click()) + act(() => typeInto(field(), 'zz')) + act(() => trigger(host).click()) + act(() => trigger(host).click()) + expect(document.querySelector('input')).toBeNull() + expect(items().map((item) => item.textContent)).toContain('Move to…') + }) + it('has nothing to offer for the only page in a space, and says so', () => { const alone = tree(['a', null]) const host = render(pageOf('a', alone), alone) @@ -159,16 +261,16 @@ describe('PageMoveMenu', () => { it('offers every page it may land under, and the top level when it is nested', () => { const host = render(pageOf('x')) act(() => trigger(host).click()) - act(() => items(host).find((item) => item.textContent === 'Move to…')!.click()) - expect(items(host).map((item) => item.textContent)).toEqual(['Top level', 'A']) + act(() => items().find((item) => item.textContent === 'Move to…')!.click()) + expect(items().map((item) => item.textContent)).toEqual(['Top level', 'A']) }) it('files the page under the destination that was picked', () => { const onMove = vi.fn() const host = render(pageOf('x'), PAGES, onMove) act(() => trigger(host).click()) - act(() => items(host).find((item) => item.textContent === 'Move to…')!.click()) - act(() => items(host).find((item) => item.textContent === 'A')!.click()) + act(() => items().find((item) => item.textContent === 'Move to…')!.click()) + act(() => items().find((item) => item.textContent === 'A')!.click()) // a parent, not a position: the new level sorts it by title, exactly as a // drop onto that row would expect(onMove).toHaveBeenCalledWith({ parentSlug: 'a', order: null }) @@ -177,27 +279,62 @@ describe('PageMoveMenu', () => { it('filters the destinations by name, and says so when none is left', () => { const host = render(pageOf('x')) act(() => trigger(host).click()) - act(() => items(host).find((item) => item.textContent === 'Move to…')!.click()) - act(() => typeInto(host.querySelector('input')!, 'zz')) - expect(items(host)).toHaveLength(0) - expect(host.textContent).toContain('no page matches') + act(() => items().find((item) => item.textContent === 'Move to…')!.click()) + act(() => typeInto(field(), 'zz')) + expect(items()).toHaveLength(0) + expect(panel()!.textContent).toContain('no page matches') }) it('closes the whole menu on Escape, not just the destination list', () => { const host = render(pageOf('x')) act(() => trigger(host).click()) - act(() => items(host).find((item) => item.textContent === 'Move to…')!.click()) + act(() => items().find((item) => item.textContent === 'Move to…')!.click()) act(() => { - host.querySelector('input')!.dispatchEvent( + field().dispatchEvent( new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }), ) }) - expect(host.querySelector('[role="menu"]')).toBeNull() + expect(panel()).toBeNull() expect(document.activeElement).toBe(trigger(host)) }) + + it('shuts the trigger while a move of its own is still in flight', () => { + // One signature per step, and the relay has not answered yet. A second + // click would plan the next step against the tree as it still looks, so + // the entry it offers is the one that has just been published. + const host = render(pageOf('b'), PAGES, vi.fn(), true) + expect(trigger(host).disabled).toBe(true) + act(() => trigger(host).click()) + expect(panel()).toBeNull() + }) + + it('does not hang the filter field inside a role="menu"', () => { + // `menu` admits menu items and nothing else, and a textbox in one is read + // out by some screen readers and skipped by others. The panel around the + // field is a dialog; the destinations below it are the menu. + const host = render(pageOf('x')) + act(() => trigger(host).click()) + act(() => items().find((item) => item.textContent === 'Move to…')!.click()) + const menu = document.querySelector('[role="menu"]')! + expect(menu.querySelector('input')).toBeNull() + expect(field().closest('[role="menu"]')).toBeNull() + expect(field().closest('[role="dialog"]')).not.toBeNull() + }) }) describe('moveTargets', () => { + it('indents each destination by its depth in the tree', () => { + const deep = tree(['a', null], ['b', null], ['x', 'b'], ['deep', 'x']) + // `a` reads as a child of the top-level entry it is listed under, so the + // list is a tree rather than a flat run of names: top level 0, the pages + // below it one deeper than the node they hang from. + expect(moveTargets(deep, 'deep')).toEqual([ + { slug: null, title: 'Top level', depth: 0 }, + { slug: 'a', title: 'A', depth: 1 }, + { slug: 'b', title: 'B', depth: 1 }, + ]) + }) + it('leaves out the page itself, its own subtree and the parent it has', () => { const deep = tree(['a', null], ['b', null], ['x', 'b'], ['deep', 'x']) // for `b`: not itself, not x or deep (its subtree), and it has no parent diff --git a/src/ui/PageMoveMenu.tsx b/src/ui/PageMoveMenu.tsx index f5f9b03..3545439 100644 --- a/src/ui/PageMoveMenu.tsx +++ b/src/ui/PageMoveMenu.tsx @@ -1,7 +1,8 @@ -import { useEffect, useId, useRef, useState } from 'react' +import { useCallback, useEffect, useId, useRef, useState } from 'react' +import { createPortal } from 'react-dom' import type { TreeMove } from '../domain/move-tree' import type { Page } from '../domain/pages' -import { moveEntries, moveTargets } from './move-page' +import { canMoveSomewhere, moveEntries, moveTargets } from './move-page' import { INPUT } from './controls' import { MoveIcon, PageIcon } from './icons' @@ -25,6 +26,23 @@ import { MoveIcon, PageIcon } from './icons' * reasoning as the "+" on the Pages heading. On a coarse pointer there is no * hover to reveal it with, so there it is simply always visible. */ + +/** `w-60`, as a number: the panel is positioned by hand, not laid out. */ +const MENU_WIDTH = 240 +/** Between the row and the panel. */ +const GAP = 4 +/** Never flush against the edge of the window. */ +const EDGE = 8 +/** + * Below this the panel is not worth opening downwards: the filter field and + * the first destinations have to be visible without scrolling, or the list + * that is the point of the menu is the first thing to be cut off. + */ +const ROOM = 220 + +/** Where the panel goes, in viewport coordinates. */ +type Anchor = { left: number; top?: number; bottom?: number; maxHeight: number } + export function PageMoveMenu({ page, pages, @@ -40,26 +58,73 @@ export function PageMoveMenu({ /** the four steps, or the list of destinations */ const [picking, setPicking] = useState(false) const [filter, setFilter] = useState('') + const [anchor, setAnchor] = useState(null) const triggerRef = useRef(null) const menuRef = useRef(null) const menuId = useId() - const entries = moveEntries(pages, page.slug) - const targets = moveTargets(pages, page.slug) - const available = entries.some((entry) => entry.move !== null) || targets.length > 0 + // Only while the menu is open. Both walk the whole tree, and there is one of + // these per row: computing them for a shut menu was the sidebar re-rendering + // the entire wiki twice per page on every event the relay pushed. What the + // closed trigger needs is one bit, and `canMoveSomewhere` is the cheap way + // to it. + const available = canMoveSomewhere(pages, page.slug) + const entries = open ? moveEntries(pages, page.slug) : [] + const targets = open ? moveTargets(pages, page.slug) : [] const needle = filter.trim().toLowerCase() const shown = needle ? targets.filter((t) => t.title.toLowerCase().includes(needle)) : targets - const close = () => { + /** + * The panel hangs under the trigger, but it is not *inside* it: see the + * portal below. So it is measured off the trigger's box and flipped above + * the row when the window has no room below — the rows near the bottom of + * the tree are the ones a long destination list matters most for. + */ + // Both of these read refs and call setters only, so they are stable for the + // life of the row — which is what lets the window listeners below hold on to + // one copy instead of being torn down and rebuilt on every keystroke in the + // filter field. + const anchorNow = useCallback((): Anchor => { + const rect = triggerRef.current?.getBoundingClientRect() + if (!rect) return { left: EDGE, top: EDGE, maxHeight: ROOM } + const below = window.innerHeight - rect.bottom - GAP - EDGE + const above = rect.top - GAP - EDGE + const flip = below < ROOM && above > below + return { + // Right-aligned with the trigger, pulled back in when that would hang + // the panel off either edge of the window. + left: Math.max( + EDGE, + Math.min(rect.right - MENU_WIDTH, window.innerWidth - MENU_WIDTH - EDGE), + ), + ...(flip ? { bottom: window.innerHeight - rect.top + GAP } : { top: rect.bottom + GAP }), + maxHeight: Math.max(ROOM, flip ? above : below), + } + }, []) + + const close = useCallback((restoreFocus: boolean) => { setOpen(false) setPicking(false) setFilter('') - triggerRef.current?.focus() + // Only when the focus is still in the menu that is going away — otherwise + // it lands on and the keyboard starts over at the top of the page. + // A click elsewhere brings its own focus and has to keep it. + if (restoreFocus) triggerRef.current?.focus() + }, []) + + // Measuring in the handler rather than in a layout effect: the panel is then + // placed in the same render that opens it, so it never paints in a corner + // first, and the effect that moves the focus into it finds it already there. + const openMenu = () => { + setAnchor(anchorNow()) + setPicking(false) + setFilter('') + setOpen(true) } const send = (move: TreeMove) => { - close() + close(true) onMove(move) } @@ -76,6 +141,20 @@ export function PageMoveMenu({ menuRef.current?.querySelector('button:not([disabled])')?.focus() }, [open, picking]) + // A panel positioned against the window has to be told when the window + // moves under it. `capture`, because the tree scrolls in a container of its + // own and a scroll event does not bubble out of it. + useEffect(() => { + if (!open) return + const follow = () => setAnchor(anchorNow()) + window.addEventListener('resize', follow) + window.addEventListener('scroll', follow, true) + return () => { + window.removeEventListener('resize', follow) + window.removeEventListener('scroll', follow, true) + } + }, [open, anchorNow]) + // Clicking anywhere else closes it. `pointerdown` rather than `click`, so it // is gone before whatever was clicked reacts — otherwise a click on another // row's trigger closes this menu and opens nothing. @@ -84,20 +163,18 @@ export function PageMoveMenu({ const onPointerDown = (event: PointerEvent) => { const target = event.target as Node if (menuRef.current?.contains(target) || triggerRef.current?.contains(target)) return - setOpen(false) - setPicking(false) - setFilter('') + close(menuRef.current?.contains(document.activeElement) ?? false) } window.addEventListener('pointerdown', onPointerDown) return () => window.removeEventListener('pointerdown', onPointerDown) - }, [open]) + }, [open, close]) const onMenuKeyDown = (event: React.KeyboardEvent) => { if (event.key === 'Escape') { // Not swallowed by the sidebar or a parent: this is the innermost thing // Escape can mean while the menu is up. event.stopPropagation() - close() + close(true) return } if (event.key !== 'ArrowDown' && event.key !== 'ArrowUp') return @@ -106,13 +183,19 @@ export function PageMoveMenu({ ...(menuRef.current?.querySelectorAll('button[role="menuitem"]') ?? []), ].filter((item) => !item.disabled) if (items.length === 0) return - const at = items.indexOf(document.activeElement as HTMLButtonElement) + const from = items.indexOf(document.activeElement as HTMLButtonElement) const step = event.key === 'ArrowDown' ? 1 : -1 // Wrapping: a four-entry menu with two of them disabled is one keypress - // from either end whichever way it wraps. From the filter field (index -1) - // ArrowDown therefore lands on the first row, which is what it looks like - // it should do. - items[(at + step + items.length) % items.length]!.focus() + // from either end whichever way it wraps. The filter field is not one of + // the entries, so from there the two keys mean the first and the last — + // counting a step from "index -1" would land one short of the end. + const to = + from === -1 + ? step === 1 + ? 0 + : items.length - 1 + : (from + step + items.length) % items.length + items[to]!.focus() } const itemClass = @@ -120,8 +203,102 @@ export function PageMoveMenu({ 'hover:bg-surface-hover hover:text-fg focus-visible:bg-surface-hover focus-visible:text-fg ' + 'disabled:pointer-events-none disabled:opacity-40' + const panelClass = + 'z-30 rounded-lg border border-line bg-surface-2 p-1 shadow-lg overflow-y-auto scroll-slim' + const panelStyle = { + position: 'fixed' as const, + width: MENU_WIDTH, + left: anchor?.left, + top: anchor?.top, + bottom: anchor?.bottom, + maxHeight: anchor?.maxHeight, + } + + const panel = picking ? ( + // Not `role="menu"` while the filter is in it: a menu may only contain + // menu items, and a textbox inside one is read out by some screen readers + // and skipped by others. A dialog holding a field and a menu is the same + // thing to look at and an honest description of it. + + ) : ( +