From b4c1f606e18e338b16e2386b3cddfb1d2fc2bcff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mime=20=C4=8Cuvalo?= Date: Fri, 17 May 2024 09:53:57 +0100 Subject: [PATCH] focus: rework and untangle existing focus management logic in the sdk (#3718) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Focus management is really scattered across the codebase. There's sort of a battle between different code paths to make the focus the correct desired state. It seemed to grow like a knot and once I started pulling on one thread to see if it was still needed you could see underneath that it was accounting for another thing underneath that perhaps wasn't needed. The impetus for this PR came but especially during the text label rework, now that it's much more easy to jump around from textfield to textfield. It became apparent that we were playing whack-a-mole trying to preserve the right focus conditions (especially on iOS, ugh). This tries to remove as many hacks as possible, and bring together in place the focus logic (and in the darkness, bind them). ## Places affected - [x] `useEditableText`: was able to remove a bunch of the focus logic here. In addition, it doesn't look like we need to save the selection range anymore. - lingering footgun that needed to be fixed anyway: if there are two labels in the same shape, because we were just checking `editingShapeId === id`, the two text labels would have just fought each other for control - [x] `useFocusEvents`: nixed and refactored — we listen to the store in `FocusManager` and then take care of autoFocus there - [x] `useSafariFocusOutFix`: nixed. not necessary anymore because we're not trying to refocus when blurring in `useEditableText`. original PR for reference: https://github.com/tldraw/brivate/pull/79 - [x] `defaultSideEffects`: moved logic to `FocusManager` - [x] `PointingShape` focus for `startTranslating`, decided to leave this alone actually. - [x] `TldrawUIButton`: it doesn't look like this focus bug fix is needed anymore, original PR for reference: https://github.com/tldraw/tldraw/pull/2630 - [x] `useDocumentEvents`: left alone its manual focus after the Escape key is hit - [x] `FrameHeading`: double focus/select doesn't seem necessary anymore - [x] `useCanvasEvents`: `onPointerDown` focus logic never happened b/c in `Editor.ts` we `clearedMenus` on pointer down - [x] `onTouchStart`: looks like `document.body.click()` is not necessary anymore ## Future Changes - [ ] a11y: work on having an accessebility focus ring - [ ] Page visibility API: (https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API) events when tab is back in focus vs. background, different kind of focus - [ ] Reexamine places we manually dispatch `pointer_down` events to see if they're necessary. - [ ] Minor: get rid of `useContainer` maybe? Is it really necessary to have this hook? you can just do `useEditor` → `editor.getContainer()`, feels superfluous. ## Methodology Looked for places where we do: - `body.click()` - places we do `container.focus()` - places we do `container.blur()` - places we do `editor.updateInstanceState({ isFocused })` - places we do `autofocus` - searched for `document.activeElement` ### Change Type - [x] `sdk` — Changes the tldraw SDK - [ ] `dotcom` — Changes the tldraw.com web app - [ ] `docs` — Changes to the documentation, examples, or templates. - [ ] `vs code` — Changes to the vscode plugin - [ ] `internal` — Does not affect user-facing stuff - [ ] `bugfix` — Bug fix - [ ] `feature` — New feature - [x] `improvement` — Improving existing features - [ ] `chore` — Updating dependencies, other boring stuff - [ ] `galaxy brain` — Architectural changes - [ ] `tests` — Changes to any test code - [ ] `tools` — Changes to infrastructure, CI, internal scripts, debugging tools, etc. - [ ] `dunno` — I don't know ### Test Plan - [x] run test-focus.spec.ts - [x] check MultipleExample - [x] check EditorFocusExample - [x] check autoFocus - [x] check style panel usage and focus events in general - [x] check text editing focus, lots of different devices, mobile/desktop ### Release Notes - Focus: rework and untangle existing focus management logic in the SDK --- .../BoardHistorySnapshot.tsx | 1 - .../src/components/CursorChatBubble.tsx | 20 ++-- apps/dotcom/src/components/LocalEditor.tsx | 1 - .../src/components/MultiplayerEditor.tsx | 1 - .../PeopleMenu/UserPresenceEditor.tsx | 4 +- .../dotcom/src/components/SnapshotsEditor.tsx | 1 - apps/examples/e2e/tests/test-focus.spec.ts | 47 ++++++++++ .../ExternalContentSourcesExample.tsx | 2 +- .../src/examples/multiple/MultipleExample.tsx | 61 +++++++++---- .../src/examples/scroll/ScrollExample.tsx | 5 +- apps/vscode/editor/src/app.tsx | 1 - packages/editor/api-report.md | 4 +- packages/editor/src/lib/TldrawEditor.tsx | 21 +---- packages/editor/src/lib/editor/Editor.ts | 31 +++++++ .../src/lib/editor/managers/FocusManager.ts | 46 ++++++++++ .../editor/src/lib/hooks/useCanvasEvents.ts | 12 --- .../editor/src/lib/hooks/useDocumentEvents.ts | 2 +- .../editor/src/lib/hooks/useFocusEvents.ts | 32 ------- .../src/lib/hooks/useSafariFocusOutFix.ts | 32 ------- packages/store/src/lib/RecordType.ts | 2 +- packages/tldraw/api-report.md | 6 +- packages/tldraw/src/lib/defaultSideEffects.ts | 10 -- .../shapes/frame/components/FrameHeading.tsx | 8 -- .../src/lib/shapes/shared/useEditableText.ts | 91 +++---------------- .../tldraw/src/lib/shapes/text/TextArea.tsx | 1 - .../SelectTool/childStates/PointingShape.ts | 2 +- .../src/lib/ui/components/EditLinkDialog.tsx | 2 +- .../src/lib/ui/components/EmbedDialog.tsx | 2 +- .../ui/components/PageMenu/PageItemInput.tsx | 4 +- .../primitives/Button/TldrawUiButton.tsx | 10 -- .../primitives/TldrawUiButtonPicker.tsx | 11 +++ .../components/primitives/TldrawUiInput.tsx | 14 +-- .../src/lib/ui/hooks/useKeyboardShortcuts.ts | 4 +- packages/tldraw/src/test/Editor.test.tsx | 4 +- .../tldraw/src/test/TldrawEditor.test.tsx | 24 +---- 35 files changed, 235 insertions(+), 284 deletions(-) create mode 100644 packages/editor/src/lib/editor/managers/FocusManager.ts delete mode 100644 packages/editor/src/lib/hooks/useFocusEvents.ts delete mode 100644 packages/editor/src/lib/hooks/useSafariFocusOutFix.ts diff --git a/apps/dotcom/src/components/BoardHistorySnapshot/BoardHistorySnapshot.tsx b/apps/dotcom/src/components/BoardHistorySnapshot/BoardHistorySnapshot.tsx index e8a07d470..d9a57e759 100644 --- a/apps/dotcom/src/components/BoardHistorySnapshot/BoardHistorySnapshot.tsx +++ b/apps/dotcom/src/components/BoardHistorySnapshot/BoardHistorySnapshot.tsx @@ -67,7 +67,6 @@ export function BoardHistorySnapshot({ }} overrides={[fileSystemUiOverrides]} inferDarkMode - autoFocus />
diff --git a/apps/dotcom/src/components/CursorChatBubble.tsx b/apps/dotcom/src/components/CursorChatBubble.tsx index 45206863e..b4b6134fa 100644 --- a/apps/dotcom/src/components/CursorChatBubble.tsx +++ b/apps/dotcom/src/components/CursorChatBubble.tsx @@ -9,7 +9,7 @@ import { useRef, useState, } from 'react' -import { preventDefault, track, useContainer, useEditor, useTranslation } from 'tldraw' +import { preventDefault, track, useEditor, useTranslation } from 'tldraw' // todo: // - not cleaning up @@ -18,7 +18,6 @@ const CHAT_MESSAGE_TIMEOUT_CHATTING = 5000 export const CursorChatBubble = track(function CursorChatBubble() { const editor = useEditor() - const container = useContainer() const { isChatting, chatMessage } = editor.getInstanceState() const rTimeout = useRef(-1) @@ -31,14 +30,14 @@ export const CursorChatBubble = track(function CursorChatBubble() { rTimeout.current = setTimeout(() => { editor.updateInstanceState({ chatMessage: '', isChatting: false }) setValue('') - container.focus() + editor.focus() }, duration) } return () => { clearTimeout(rTimeout.current) } - }, [container, editor, chatMessage, isChatting]) + }, [editor, chatMessage, isChatting]) if (isChatting) return @@ -101,7 +100,6 @@ const CursorChatInput = track(function CursorChatInput({ }) { const editor = useEditor() const msg = useTranslation() - const container = useContainer() const ref = useRef(null) const placeholder = chatMessage || msg('cursor-chat.type-to-chat') @@ -126,11 +124,9 @@ const CursorChatInput = track(function CursorChatInput({ }, [editor, value, placeholder]) useLayoutEffect(() => { - // Focus the editor - let raf = requestAnimationFrame(() => { - raf = requestAnimationFrame(() => { - ref.current?.focus() - }) + // Focus the input + const raf = requestAnimationFrame(() => { + ref.current?.focus() }) return () => { @@ -140,8 +136,8 @@ const CursorChatInput = track(function CursorChatInput({ const stopChatting = useCallback(() => { editor.updateInstanceState({ isChatting: false }) - container.focus() - }, [editor, container]) + editor.focus() + }, [editor]) // Update the chat message as the user types const handleChange = useCallback( diff --git a/apps/dotcom/src/components/LocalEditor.tsx b/apps/dotcom/src/components/LocalEditor.tsx index fc42eb00a..43babc805 100644 --- a/apps/dotcom/src/components/LocalEditor.tsx +++ b/apps/dotcom/src/components/LocalEditor.tsx @@ -102,7 +102,6 @@ export function LocalEditor() { assetUrls={assetUrls} persistenceKey={SCRATCH_PERSISTENCE_KEY} onMount={handleMount} - autoFocus overrides={[sharingUiOverrides, fileSystemUiOverrides]} onUiEvent={handleUiEvent} components={components} diff --git a/apps/dotcom/src/components/MultiplayerEditor.tsx b/apps/dotcom/src/components/MultiplayerEditor.tsx index 10f53df4e..32e1f5765 100644 --- a/apps/dotcom/src/components/MultiplayerEditor.tsx +++ b/apps/dotcom/src/components/MultiplayerEditor.tsx @@ -158,7 +158,6 @@ export function MultiplayerEditor({ initialState={isReadonly ? 'hand' : 'select'} onUiEvent={handleUiEvent} components={components} - autoFocus inferDarkMode > diff --git a/apps/dotcom/src/components/PeopleMenu/UserPresenceEditor.tsx b/apps/dotcom/src/components/PeopleMenu/UserPresenceEditor.tsx index ab168473f..0cf47d336 100644 --- a/apps/dotcom/src/components/PeopleMenu/UserPresenceEditor.tsx +++ b/apps/dotcom/src/components/PeopleMenu/UserPresenceEditor.tsx @@ -52,8 +52,8 @@ export function UserPresenceEditor() { onCancel={toggleEditingName} onBlur={handleBlur} shouldManuallyMaintainScrollPositionWhenFocused - autofocus - autoselect + autoFocus + autoSelect /> ) : ( <> diff --git a/apps/dotcom/src/components/SnapshotsEditor.tsx b/apps/dotcom/src/components/SnapshotsEditor.tsx index 330cc875e..bc2cea9d1 100644 --- a/apps/dotcom/src/components/SnapshotsEditor.tsx +++ b/apps/dotcom/src/components/SnapshotsEditor.tsx @@ -89,7 +89,6 @@ export function SnapshotsEditor(props: SnapshotEditorProps) { }} components={components} renderDebugMenuItems={() => } - autoFocus inferDarkMode > diff --git a/apps/examples/e2e/tests/test-focus.spec.ts b/apps/examples/e2e/tests/test-focus.spec.ts index c2998f1f8..49708f702 100644 --- a/apps/examples/e2e/tests/test-focus.spec.ts +++ b/apps/examples/e2e/tests/test-focus.spec.ts @@ -175,4 +175,51 @@ test.describe('Focus', () => { null ) }) + + test('still focuses text after clicking on style button', async ({ page }) => { + await page.goto('http://localhost:5420/end-to-end') + await page.waitForSelector('.tl-canvas') + + const EditorA = (await page.$(`.tl-container`))! + expect(EditorA).toBeTruthy() + + // Create a new note, text should be focused + await page.keyboard.press('n') + await (await page.$('body'))?.click() + await page.waitForSelector('.tl-shape') + + const blueButton = await page.$('.tlui-button[data-testid="style.color.blue"]') + await blueButton?.dispatchEvent('pointerdown') + await blueButton?.click() + await blueButton?.dispatchEvent('pointerup') + + // Text should still be focused. + expect(await page.evaluate(() => document.activeElement?.nodeName === 'TEXTAREA')).toBe(true) + }) + + test('edit->edit, focus stays in the text areas when going from shape-to-shape', async ({ + page, + }) => { + await page.goto('http://localhost:5420/end-to-end') + await page.waitForSelector('.tl-canvas') + + const EditorA = (await page.$(`.tl-container`))! + expect(EditorA).toBeTruthy() + + // Create a new note, text should be focused + await page.keyboard.press('n') + await (await page.$('body'))?.click() + await page.waitForSelector('.tl-shape') + await page.keyboard.type('test') + + // create new note next to it + await page.keyboard.press('Tab') + + await (await page.$('body'))?.click() + + // First note's textarea should be focused. + expect(await EditorA.evaluate(() => !!document.querySelector('.tl-shape textarea:focus'))).toBe( + true + ) + }) }) diff --git a/apps/examples/src/examples/external-content-sources/ExternalContentSourcesExample.tsx b/apps/examples/src/examples/external-content-sources/ExternalContentSourcesExample.tsx index 619b79cd7..d8dd54836 100644 --- a/apps/examples/src/examples/external-content-sources/ExternalContentSourcesExample.tsx +++ b/apps/examples/src/examples/external-content-sources/ExternalContentSourcesExample.tsx @@ -67,7 +67,7 @@ export default function ExternalContentSourcesExample() { return (
- +
) } diff --git a/apps/examples/src/examples/multiple/MultipleExample.tsx b/apps/examples/src/examples/multiple/MultipleExample.tsx index 1eae026c8..490d7bf4b 100644 --- a/apps/examples/src/examples/multiple/MultipleExample.tsx +++ b/apps/examples/src/examples/multiple/MultipleExample.tsx @@ -1,5 +1,5 @@ import { createContext, useCallback, useContext, useState } from 'react' -import { Tldraw } from 'tldraw' +import { Editor, Tldraw } from 'tldraw' import 'tldraw/tldraw.css' // There's a guide at the bottom of this page! @@ -7,24 +7,35 @@ import 'tldraw/tldraw.css' // [1] const focusedEditorContext = createContext( {} as { - focusedEditor: string | null - setFocusedEditor: (id: string | null) => void + focusedEditor: Editor | null + setFocusedEditor: (id: Editor | null) => void } ) // [2] export default function MultipleExample() { - const [focusedEditor, _setFocusedEditor] = useState('A') + const [focusedEditor, _setFocusedEditor] = useState(null) const setFocusedEditor = useCallback( - (id: string | null) => { - if (focusedEditor !== id) { - _setFocusedEditor(id) + (editor: Editor | null) => { + if (focusedEditor !== editor) { + focusedEditor?.updateInstanceState({ isFocused: false }) + _setFocusedEditor(editor) + editor?.updateInstanceState({ isFocused: true }) } }, [focusedEditor] ) + const focusName = + focusedEditor === (window as any).EDITOR_A + ? 'A' + : focusedEditor === (window as any).EDITOR_B + ? 'B' + : focusedEditor === (window as any).EDITOR_C + ? 'C' + : 'none' + return (
setFocusedEditor(null)} > -

Focusing: {focusedEditor ?? 'none'}

+

Focusing: {focusName}