diff --git a/apps/client/src/todos/lib/editable-caret.test.ts b/apps/client/src/todos/lib/editable-caret.test.ts new file mode 100644 index 0000000..380323a --- /dev/null +++ b/apps/client/src/todos/lib/editable-caret.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from 'vitest' +import type { QuickAddToken } from './quick-add' +import { tokensChanged } from './editable-caret' + +// docs/specs/quick-add.md — what a contenteditable costs. This predicate +// is the whole undo story: rewriting `innerHTML` empties the browser's +// undo stack, so the marks are redrawn only when the token set actually +// changes and left alone for every keystroke that does not move one. +// False here means the DOM is never touched, which is what keeps ⌘Z +// native for ordinary typing. +// +// The caret helpers beside this one are deliberately *not* unit tested: +// what they assert is DOM behaviour, the client has no DOM test +// environment (every test here is a `.ts` over extracted logic), and +// adding one for two functions would be a fourth test arrangement in a +// repo that already has three. They are covered in e2e/tests/quick-add, +// against a real browser. + +const token = ( + kind: QuickAddToken['kind'], + start: number, + end: number, +): QuickAddToken => ({ kind, start, end }) + +describe('tokensChanged', () => { + it('is false for the same tokens, so typing does not redraw', () => { + const text = 'Clean the gutters tomorrow at 3pm #Chores' + const before = [token('date', 18, 33), token('list', 34, 41)] + const after = [token('date', 18, 33), token('list', 34, 41)] + expect(tokensChanged(before, after, text, text)).toBe(false) + }) + + it('is false for two empty sets', () => { + expect(tokensChanged([], [], '', '')).toBe(false) + }) + + it('is true when a token appears', () => { + // Typing the "m" that completes "3pm": this is the one keystroke + // that costs an undo entry, and it has to redraw to earn it. + const before = [token('list', 34, 41)] + const after = [token('date', 18, 33), token('list', 34, 41)] + const text = 'Clean the gutters tomorrow at 3pm #Chores' + expect(tokensChanged(before, after, text, text)).toBe(true) + }) + + it('is true when a token disappears', () => { + const text = 'Clean the gutters tomorrow at 3pm #Chores' + const before = [token('date', 18, 33), token('list', 34, 41)] + const after = [token('date', 18, 33)] + expect(tokensChanged(before, after, text, text)).toBe(true) + }) + + it('is false when a token only slides along the line', () => { + // Typing a character *before* a token shifts its offsets without + // changing which words are marked. Found in the browser 2026-08-19: + // comparing raw offsets made this the common case rather than the + // rare one — every keystroke in the prose before a token forced a + // redraw, and with it lost that keystroke's undo entry, which is + // exactly what this predicate exists to prevent. What matters is + // which text is marked, not where it sits. + const text = 'Clean the gutters tomorrow at 3pm' + const shifted = 'Clean the Zgutters tomorrow at 3pm' + expect( + tokensChanged( + [token('date', 18, 33)], + [token('date', 19, 34)], + text, + shifted, + ), + ).toBe(false) + }) + + it('is true when a token moves onto different words', () => { + const before = [token('date', 0, 8)] + const after = [token('date', 9, 17)] + expect( + tokensChanged(before, after, 'tomorrow p1 today', 'tomorrow today p1'), + ).toBe(true) + }) + + it('is true when a token grows', () => { + // "tomorrow" becoming "tomorrow at 3pm" — same start, same kind, and + // the mark has to stretch. + const text = 'Clean the gutters tomorrow at 3pm' + const before = [token('date', 18, 26)] + const after = [token('date', 18, 33)] + expect(tokensChanged(before, after, text, text)).toBe(true) + }) + + it('is true when only the kind differs', () => { + // Same span, different meaning. Nothing in the app produces this + // today, but the mark is drawn from the token, so a set that differs + // only by kind is still a set that has changed. + const before = [token('date', 0, 2)] + const after = [token('priority', 0, 2)] + expect(tokensChanged(before, after, 'p1 milk', 'p1 milk')).toBe(true) + }) +}) diff --git a/apps/client/src/todos/lib/editable-caret.ts b/apps/client/src/todos/lib/editable-caret.ts new file mode 100644 index 0000000..53b42e5 --- /dev/null +++ b/apps/client/src/todos/lib/editable-caret.ts @@ -0,0 +1,158 @@ +import type { QuickAddToken } from './quick-add' + +// docs/specs/quick-add.md — what a contenteditable costs. +// +// The caret in a contenteditable is a DOM position: a node plus an offset +// into it. Everything else in quick add addresses text by a plain offset +// into the string — `replaceToken` returns one, the parser's tokens are +// `slice`-compatible ranges — so these two functions are the whole +// translation layer between the two worlds, and nothing above them has to +// know a Range exists. +// +// The `` this replaced had `setSelectionRange` for the same job. + +/** + * Where the caret is, as an offset into the element's plain text. + * + * `null` when the selection is somewhere else on the page, which is the + * ordinary case while a pill menu is open. + * + * Measured by asking a Range how much text precedes the caret rather than + * by walking and summing node lengths ourselves: a Range counts exactly + * what `innerText` would return over the same span, so the two agree even + * when the marks split the text into many nodes. + */ +export function caretOffset(el: HTMLElement): number | null { + const selection = document.getSelection() + if (!selection || selection.rangeCount === 0) return null + + const range = selection.getRangeAt(0) + if (!el.contains(range.endContainer)) return null + + const toCaret = range.cloneRange() + toCaret.selectNodeContents(el) + toCaret.setEnd(range.endContainer, range.endOffset) + return toCaret.toString().length +} + +/** + * Put the caret at a plain-text offset, counting through the text nodes. + * + * `>=` rather than `>` when finding the node: an offset landing exactly on + * a node's end belongs to that node, at its final position. Preferring the + * *next* node would put the caret on the far side of a mark's boundary, + * so typing after a token would land inside it. + * + * An offset past the end clamps to the last text node rather than throwing + * — the text can only have shrunk under a caret we are restoring, and the + * end of the line is the sane place to be. + */ +export function placeCaret(el: HTMLElement, offset: number): void { + const walker = document.createTreeWalker(el, NodeFilter.SHOW_TEXT) + let seen = 0 + let last: Node | null = null + + // `SHOW_TEXT` guarantees every node here is a Text, but the DOM types + // say `Node` — narrowed by checking rather than asserted, so this stays + // honest if the filter ever changes. + for (let node = walker.nextNode(); node; node = walker.nextNode()) { + const length = node.textContent?.length ?? 0 + if (seen + length >= offset) { + select(node, offset - seen) + return + } + seen += length + last = node + } + + // Nothing long enough: an empty field has no text node at all, so put + // the caret in the element itself rather than giving up. + if (last) select(last, last.textContent?.length ?? 0) + else select(el, 0) +} + +function select(node: Node, offset: number): void { + const range = document.createRange() + range.setStart(node, offset) + range.collapse(true) + const selection = document.getSelection() + selection?.removeAllRanges() + selection?.addRange(range) +} + +/** + * Must the marks be redrawn? + * + * **This is what keeps undo native.** Writing `innerHTML` empties the + * browser's undo stack — measured 2026-08-19, and the reason `⌘Z` does + * nothing at all in a naive highlighting editor. Answering `false` here + * leaves the DOM untouched, so the browser's own editing, and its undo + * history along with it, is never disturbed. + * + * **Compares the marked *words*, not their offsets.** Offsets were the + * first implementation and were wrong in use: typing anywhere before a + * token shifts every later token's `start` and `end`, so a keystroke in + * the middle of the summary counted as a change and forced a redraw. That + * made the redraw the common case rather than the rare one and cost an + * undo entry per keystroke — the exact failure this predicate exists to + * prevent. The marks are drawn around text, and text that has only slid + * along the line is still in the right DOM node. + * *(fixed 2026-08-19, found in the browser.)* + */ +export function tokensChanged( + before: readonly QuickAddToken[], + after: readonly QuickAddToken[], + beforeText: string, + afterText: string, +): boolean { + if (before.length !== after.length) return true + return before.some((token, index) => { + const other = after[index] + return ( + !other || + token.kind !== other.kind || + beforeText.slice(token.start, token.end) !== + afterText.slice(other.start, other.end) + ) + }) +} + +/** + * Where the caret is on screen, in viewport coordinates. + * + * `null` when the selection is outside `el`, or when the browser reports + * no rect at all — a collapsed range at the very start of an empty element + * gives an empty list rather than a zero-width rect. + * + * A collapsed range has no dimensions in some engines, so this expands it + * by one character where it can and measures that instead. The + * `getClientRects()[0]` path covers the ordinary case, and is preferred + * because it is the caret's own line rather than a character beside it — + * which matters on a wrapped field, where the two can be different lines. + */ +export function caretRect( + el: HTMLElement, +): { bottom: number; left: number } | null { + const selection = document.getSelection() + if (!selection || selection.rangeCount === 0) return null + + const range = selection.getRangeAt(0) + if (!el.contains(range.endContainer)) return null + + const direct = range.getClientRects()[0] + if (direct) return { bottom: direct.bottom, left: direct.left } + + // Collapsed with no rect of its own: measure the character before the + // caret, which sits on the same line, and take its trailing edge. + const probe = range.cloneRange() + const offset = range.endOffset + if (offset > 0) { + probe.setStart(range.endContainer, offset - 1) + const before = probe.getClientRects()[0] + if (before) return { bottom: before.bottom, left: before.right } + } + + // Nothing measurable — an empty field. Its own box is the best answer. + const box = el.getBoundingClientRect() + return { bottom: box.top + el.clientHeight, left: box.left } +} diff --git a/apps/client/src/todos/lib/quick-add.test.ts b/apps/client/src/todos/lib/quick-add.test.ts index cf3dc7b..b752b7a 100644 --- a/apps/client/src/todos/lib/quick-add.test.ts +++ b/apps/client/src/todos/lib/quick-add.test.ts @@ -460,4 +460,15 @@ describe('regressions', () => { expect(result.listId).toBe('s') expect(result.summary).toBe('Buy milk') }) + + it('takes a day from one phrase and a time from another', () => { + // Reported from use 2026-08-19: "next week when 3pm" set the week and + // silently dropped the time, leaving "3pm" sitting in the summary + // describing a due the todo did not have. Only the first chrono match + // per segment was read, so the second was thrown away before the day + // and time could be taken from different phrases. + const result = parseQuickAdd('Call them next week when 3pm', [], NOW) + expect(result.due).toEqual({ date: '2026-08-21', time: '15:00' }) + expect(result.summary).toBe('Call them when') + }) }) diff --git a/apps/client/src/todos/lib/quick-add.ts b/apps/client/src/todos/lib/quick-add.ts index 4feb5c5..6ac8e74 100644 --- a/apps/client/src/todos/lib/quick-add.ts +++ b/apps/client/src/todos/lib/quick-add.ts @@ -281,58 +281,69 @@ export function parseQuickAdd( // is reported and no date text is stripped from the summary // (see `QuickAddOptions.noDates`). for (const segment of options.noDates ? [] : gapsBetween(text, tokens)) { - const [parsed] = chrono.parse(segment.text, now, { forwardDate: true }) - if (!parsed) continue - const start = parsed.start - // **"Now" is not a due date.** A todo scheduled for this instant is - // overdue as soon as it exists, which nobody means, so a match that - // resolves to the reference instant is discarded rather than written. - // - // This also removes a whole family of false positives rather than - // listing them. chrono's casual parser reads a determiner followed by - // a unit letter — "the s", "a s", "the m" — as *now*, so typing "sort - // out the shed" invented a due date of today at the current minute the - // moment "the s" was on screen, then took those words out of the - // summary. Every one of those resolves to the reference instant, so - // one rule about meaning catches them all; matching the phrases would - // have been a list that the next casual pattern escapes. - // *(added 2026-08-14, found in review.)* - if (isNow(start, now)) continue - // The same family, for the few that land a minute out rather than on - // the instant — see `isPhantomDate`. - if (isPhantomDate(parsed.text)) continue - // `isCertain('hour')` is the all-day/timed distinction - // (docs/specs/todos.md — due times): a date chrono inferred rather - // than read is not a time the user asked for. - const namesTime = start.isCertain('hour') - if (namesTime) clock ??= start.date() - // A bare time ("3pm") also resolves to a day — today's — which must - // not outrank an explicit one stated elsewhere in the line. Only a - // match that actually names a day sets it. - const namesDay = - start.isCertain('day') || - start.isCertain('weekday') || - start.isCertain('month') - if (namesDay) day ??= start.date() - // **A match that sets nothing is not a match.** chrono recognises - // spans that name no date component at all — "this week" and "this - // year" both come back with an empty `knownValues` — so neither of - // the two branches above fires. Highlighting it anyway marked the - // words and then took them out of the summary, leaving a todo that - // had lost "this week" and gained no due date. - // - // "This week" is ambiguous in any case: chrono resolves it to - // tomorrow, which is nobody's reading of it. Leaving the words in the - // title is the honest outcome. - // *(added 2026-08-17, reported from use.)* - if (!namesDay && !namesTime) continue - // Offset back into the original string, since chrono indexed the - // segment rather than the whole line. - tokens.push({ - kind: 'date', - start: segment.offset + parsed.index, - end: segment.offset + parsed.index + parsed.text.length, - }) + // **Every match in the segment, not just the first.** `day` and + // `clock` are separate for exactly this case — a line can name a day + // in one phrase and a time in another — but taking only + // `chrono.parse(...)[0]` threw the second away before either could see + // it. "next week when 3pm" set the week and dropped the time, leaving + // "3pm" in the summary reporting a due the todo did not have. The + // `??=` below still means the first day and the first time win, which + // is the rule that was always intended. + // *(fixed 2026-08-19, reported from use.)* + for (const parsed of chrono.parse(segment.text, now, { + forwardDate: true, + })) { + const start = parsed.start + // **"Now" is not a due date.** A todo scheduled for this instant is + // overdue as soon as it exists, which nobody means, so a match that + // resolves to the reference instant is discarded rather than written. + // + // This also removes a whole family of false positives rather than + // listing them. chrono's casual parser reads a determiner followed by + // a unit letter — "the s", "a s", "the m" — as *now*, so typing "sort + // out the shed" invented a due date of today at the current minute the + // moment "the s" was on screen, then took those words out of the + // summary. Every one of those resolves to the reference instant, so + // one rule about meaning catches them all; matching the phrases would + // have been a list that the next casual pattern escapes. + // *(added 2026-08-14, found in review.)* + if (isNow(start, now)) continue + // The same family, for the few that land a minute out rather than on + // the instant — see `isPhantomDate`. + if (isPhantomDate(parsed.text)) continue + // `isCertain('hour')` is the all-day/timed distinction + // (docs/specs/todos.md — due times): a date chrono inferred rather + // than read is not a time the user asked for. + const namesTime = start.isCertain('hour') + if (namesTime) clock ??= start.date() + // A bare time ("3pm") also resolves to a day — today's — which must + // not outrank an explicit one stated elsewhere in the line. Only a + // match that actually names a day sets it. + const namesDay = + start.isCertain('day') || + start.isCertain('weekday') || + start.isCertain('month') + if (namesDay) day ??= start.date() + // **A match that sets nothing is not a match.** chrono recognises + // spans that name no date component at all — "this week" and "this + // year" both come back with an empty `knownValues` — so neither of + // the two branches above fires. Highlighting it anyway marked the + // words and then took them out of the summary, leaving a todo that + // had lost "this week" and gained no due date. + // + // "This week" is ambiguous in any case: chrono resolves it to + // tomorrow, which is nobody's reading of it. Leaving the words in the + // title is the honest outcome. + // *(added 2026-08-17, reported from use.)* + if (!namesDay && !namesTime) continue + // Offset back into the original string, since chrono indexed the + // segment rather than the whole line. + tokens.push({ + kind: 'date', + start: segment.offset + parsed.index, + end: segment.offset + parsed.index + parsed.text.length, + }) + } } // A time with no day means today — chrono's own reading, and the one a diff --git a/apps/client/src/todos/quick-add-field/quick-add-field.module.css b/apps/client/src/todos/quick-add-field/quick-add-field.module.css new file mode 100644 index 0000000..c8decc8 --- /dev/null +++ b/apps/client/src/todos/quick-add-field/quick-add-field.module.css @@ -0,0 +1,86 @@ +/* docs/specs/quick-add.md — the field wraps, and grows. + See quick-add-field.tsx. */ + +/* The line you type a todo into. + * + * One element, not two. It was an `` under a shadow layer holding + * the same text at the same metrics, which is what made the marks + * unpaddable: padding a token in the shadow moved that layer's text and + * nothing else. A contenteditable has no second layer to fall out of + * register with, so the marks below can be drawn as marks rather than as + * a box-shadow trick. + * + * It wraps and grows without bound — no `max-height`, no scrolling. A cap + * here would put the text back behind a scroll edge, which is the thing + * this change exists to remove; the *modal* carries the cap instead + * (quick-add-modal.module.css — `.popup`), so the pills and the buttons + * stay reachable while every line of the todo stays visible. */ +.field { + font: inherit; + font-size: var(--text-lg); + line-height: 1.5; + padding: var(--space-2); + border: 0; + /* No focus ring, matching the notes field below it and the `` this + replaced. + * + The global `:focus-visible` rule (styles/global.css) draws a 2px accent + outline on anything focusable, and exempts `input`, `select` and + `textarea` — the elements that carry a border to change instead. A + contenteditable `div` is in none of those lists, so it picked up a ring + the old field never had, around the one element in this modal that is + always focused the moment it opens. Two framed fields stacked is the + form this modal exists to replace. + *(added 2026-08-19, reported from use.)* */ + outline: none; + color: var(--ink); + caret-color: var(--ink); + /* `pre-wrap` keeps the runs of spaces someone types between tokens, and + wraps at the edge. `break-word` is the guard against a single + unbroken 200-character string — a pasted URL — widening the modal + past the viewport instead of breaking. */ + white-space: pre-wrap; + overflow-wrap: break-word; + /* An empty field is still a line tall, so the modal does not jolt when + the first character lands. */ + min-height: calc(1.5 * var(--text-lg)); +} + +.field:focus-visible { + outline: none; +} + +/* The placeholder, drawn rather than declared: `::placeholder` applies to + form controls, and this is not one. `:empty` is exactly right here — + the element holds one text node while it has any text at all, and none + when it does not. */ +.field:empty::before { + content: attr(data-placeholder); + color: var(--faint); + /* Without this the caret sits *after* the placeholder rather than at + the start of the line. */ + pointer-events: none; +} + +/* A recognised token: *marked* text, not a chip. + * + * Padded and softly rounded, which the shadow-layer arrangement could not + * do at any price — see the spec. `--radius-sm` rather than + * `--radius-full`: a pill-shaped run of words sitting in a line of prose + * reads as an object you could pick up and move, and this is your own + * text that the parser has understood, not a chip. + * + * Tinted `--accent` at the same 12% the preview pills use: the mark and + * the pill below it are two views of one token, so they share a hue. */ +.token { + color: var(--accent); + background: color-mix(in srgb, var(--accent) 12%, transparent); + padding: 2px 5px; + border-radius: var(--radius-sm); + /* A mark that wraps across two lines is drawn complete on both. Without + this the padding and the rounded corners are applied to the whole + inline box once, so the fragment on the second line loses its left + edge and the first loses its right. */ + box-decoration-break: clone; + -webkit-box-decoration-break: clone; +} diff --git a/apps/client/src/todos/quick-add-field/quick-add-field.tsx b/apps/client/src/todos/quick-add-field/quick-add-field.tsx new file mode 100644 index 0000000..a46d1b9 --- /dev/null +++ b/apps/client/src/todos/quick-add-field/quick-add-field.tsx @@ -0,0 +1,345 @@ +import { + useLayoutEffect, + useRef, + type ClipboardEvent, + type FormEvent, + type KeyboardEvent, + type ReactNode, + type RefObject, +} from 'react' +import { caretOffset, placeCaret, tokensChanged } from '../lib/editable-caret' +import type { QuickAddToken } from '../lib/quick-add' +import styles from './quick-add-field.module.css' + +/** + * The longest a summary may be. + * + * A todo's summary is a *title*; prose belongs in the notes field below, + * which is what that field is for. Without a bound the cost of every + * keystroke grows with the text — the whole line is re-parsed and the + * marks can be redrawn — which was measured at ~12ms per keystroke at + * 4,000 characters on a fast machine, so several times that on an + * ordinary one. 500 is far past any real todo and short enough that the + * per-keystroke work stays flat. + * *(added 2026-08-19, reported from use: holding ⌘V made typing crawl.)* + */ +const MAX_SUMMARY = 500 + +interface QuickAddFieldProps { + value: string + onChange: (value: string) => void + onKeyDown: (event: KeyboardEvent) => void + /** Whether the field holds focus, so the caller can close its menus. */ + onFocusChange: (focused: boolean) => void + placeholder: string + tokens: readonly QuickAddToken[] + /** Where a pill rewrite wants the caret, or null to leave it alone. */ + caretTo: RefObject + fieldRef: RefObject +} + +/** + * The line you type a todo into (docs/specs/quick-add.md — the field + * wraps, and grows). + * + * A contenteditable rather than a form control, for one reason: the + * recognised tokens are marked *inside* the text, and a padded mark is + * only possible when the marks are real elements in the element you type + * into. The `` this replaced kept its marks in a shadow layer + * underneath, which cannot pad a token without sliding that layer out of + * register with the text above it — measured at 34px across three tokens + * then, and 52px again against a wrapping `