focus: rework and untangle existing focus management logic in the sdk #3718

mimecuvalo · 2024-05-08T09:00:28Z

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

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

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
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

run test-focus.spec.ts
check MultipleExample
check EditorFocusExample
check autoFocus
check style panel usage and focus events in general
check text editing focus, lots of different devices, mobile/desktop

Release Notes

Focus: rework and untangle existing focus management logic in the SDK

vercel · 2024-05-08T09:00:35Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
examples	✅ Ready (Inspect)	Visit Preview	May 15, 2024 2:10pm

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
tldraw-docs	⬜️ Ignored (Inspect)	Visit Preview	May 15, 2024 2:10pm

mimecuvalo · 2024-05-08T09:02:58Z

apps/dotcom/src/components/BoardHistorySnapshot/BoardHistorySnapshot.tsx

@@ -67,7 +67,6 @@ export function BoardHistorySnapshot({
 					}}
 					overrides={[fileSystemUiOverrides]}
 					inferDarkMode
-					autoFocus


this is already the default

mimecuvalo · 2024-05-08T09:03:27Z

apps/dotcom/src/components/CursorChatBubble.tsx

-				ref.current?.focus()
-			})
+		// Focus the input
+		const raf = requestAnimationFrame(() => {


this double raf doesn't seem necessary... but maybe i'm missing something.

yeah your version works in chrome, safari and firefox. cc @TodePond do you remember if this double raf was intentional?

mimecuvalo · 2024-05-08T09:03:50Z

apps/dotcom/src/components/PeopleMenu/UserPresenceEditor.tsx

@@ -52,8 +52,8 @@ export function UserPresenceEditor() {
 					onCancel={toggleEditingName}
 					onBlur={handleBlur}
 					shouldManuallyMaintainScrollPositionWhenFocused
-					autofocus
-					autoselect
+					autoFocus


drive-by rename to make autoFocus across the SDK consistent

mimecuvalo · 2024-05-08T09:06:45Z

packages/editor/src/lib/hooks/useCanvasEvents.ts

@@ -39,15 +39,6 @@ export function useCanvasEvents() {
 					name: 'pointer_down',
 					...getPointerInfo(e),
 				})
-
-				if (editor.getOpenMenus().length > 0) {


this logic path had bitrotted, as mentioned in the PR description

mimecuvalo · 2024-05-08T09:06:57Z

packages/editor/src/lib/hooks/useCanvasEvents.ts

@@ -97,9 +88,6 @@ export function useCanvasEvents() {

 			function onTouchStart(e: React.TouchEvent) {
 				;(e as any).isKilled = true
-				// todo: investigate whether this effects keyboard shortcuts


doesn't look like this is necessary anymore...

did you test this on iOS? seems fine on android

yes, i've been testing on the iOS simulator. will definitely test again once it's on staging.

mimecuvalo · 2024-05-08T09:07:38Z

packages/tldraw/src/lib/defaultSideEffects.ts

@@ -2,16 +2,6 @@ import { Editor } from '@tldraw/editor'

 export function registerDefaultSideEffects(editor: Editor) {
 	return [
-		editor.sideEffects.registerAfterChangeHandler('instance', (prev, next) => {


moved to FocusManager

mimecuvalo · 2024-05-08T09:07:51Z

packages/tldraw/src/lib/shapes/frame/components/FrameHeading.tsx

@@ -58,14 +58,6 @@ export const FrameHeading = function FrameHeading({
 			// On iOS, we must focus here
 			el.focus()
 			el.select()
-
-			requestAnimationFrame(() => {


this double raf doesn't seem necessary anymore...

mimecuvalo · 2024-05-08T09:08:54Z

packages/tldraw/src/lib/shapes/shared/useEditableText.ts

-	// When the label blurs, deselect all of the text and complete.
-	// This makes it so that the canvas does not have to be focused
-	// in order to exit the editing state and complete the editing state
-	const handleBlur = useCallback(() => {


we were doing a lot of hoops and somersaults to account for what TldrawUiButtonPicker should have been managing

mimecuvalo · 2024-05-08T09:09:50Z

packages/tldraw/src/lib/ui/components/primitives/TldrawUiButtonPicker.tsx

+			// This is fun little micro-optimization to make sure that the focus
+			// is retained on a text label. That way, you can continue typing
+			// after selecting a style.
+			const origActiveEl = rPointingOriginalActiveElement.current


The logic for what useEditableText was trying to accomplish was moved here

mimecuvalo · 2024-05-08T09:10:19Z

packages/tldraw/src/lib/ui/components/primitives/Button/TldrawUiButton.tsx

-		const editor = useEditor()
-
-		// If the button is getting disabled while it's focused, move focus to the editor
-		// so that the user can continue using keyboard shortcuts


As mentioned in the PR desc., it looks like this isn't needed anymore...

are you sure? document.activeElement still reverts to the body when the button becomes inactive in my tests. admittedly that no longer seems to prevent keyboard shortcuts from working but maybe it could cause other issues?

i think i'm specifically making sure that the original PR (#2630) is working. if it causes other issues, then i think i would rather that we know what those issues are vs. this covering them up potentially (as a fix for something unrelated)

mimecuvalo · 2024-05-08T10:15:59Z

@MitjaBezensek @SomeHats actually hold off on reviewing - there's something i have to fix first

ds300 · 2024-05-13T12:57:10Z

packages/editor/src/lib/editor/Editor.ts

+	/**
+	 * Dispatch a focus event.
+	 *
+	 * @example
+	 * ```ts
+	 * editor.focus()
+	 * ```
+	 *
+	 * @public
+	 */
+	focus(): this {
+		this.focusManager.focus()
+		return this
+	}


I wonder whether this should set the instanceState.isFocused to true if it's not already?

or really maybe it should be the job of the focus manager

i think possibly: that it's tricky. if there are multiple editors on the page, i could see an argument that you might want to temporarily translate/move a shape on one editor (Editor A), but the focus remains on a separate editor (Editor B). and that just because you focus Editor A, it doesn't mean that you want Editor A to take over control from Editor B. but perhaps that's overthinking it?

ds300 · 2024-05-13T13:21:36Z

packages/tldraw/src/lib/shapes/shared/useEditableText.ts

+			if (shapeId === id) {
+				rInput.current?.select()
+			}


you sure this is still working on iOS?

i've been testing on the iOS sim - will definitely test on staging once it lands to double check!

ds300

I don't feel super qualified to review this but things all seem to work on mac and android in my testing so 👍🏼

The only thing I'm feeling pause on is the behaviour/api of the focus manager.

should calling focusManager.focus update the instanceState.isFocused property, and same for the .blur method?
should we keep the manager itself private and just have a .focus and .blur method on the editor that call the underlying manager methods? I know how steve feels about namespaces.

mimecuvalo · 2024-05-15T13:52:35Z

should calling focusManager.focus update the instanceState.isFocused property, and same for the .blur method?

yeah, it's a good question - like i said above: i think possibly: that it's tricky. if there are multiple editors on the page, i could see an argument that you might want to temporarily translate/move a shape on one editor (Editor A), but the focus remains on a separate editor (Editor B). and that just because you focus Editor A, it doesn't mean that you want Editor A to take over control from Editor B. but perhaps that's overthinking it?

should we keep the manager itself private and just have a .focus and .blur method on the editor that call the underlying manager methods? I know how steve feels about namespaces.

fair point - i think i can make the focus manager be private actually. (to be clear, no one is calling blur() explicitly at the moment, it's only used in FocusManager at the moment)

focus: rework and untangle existing logic in the sdk

56e2443

mimecuvalo requested review from SomeHats and MitjaBezensek May 8, 2024 09:00

huppy-bot bot added improvement Improves existing features sdk Affects the tldraw sdk labels May 8, 2024

mimecuvalo commented May 8, 2024

View reviewed changes

whoops

6457b53

mimecuvalo commented May 8, 2024

View reviewed changes

vercel bot deployed to Preview – examples May 8, 2024 09:09 View deployment

mimecuvalo commented May 8, 2024

View reviewed changes

not needed anymore

e34c9d6

vercel bot deployed to Preview – examples May 8, 2024 09:14 View deployment

merge

27fa83f

mimecuvalo temporarily deployed to deploy-staging May 8, 2024 10:08 — with GitHub Actions Inactive

vercel bot deployed to Preview – examples May 8, 2024 10:11 View deployment

github-actions bot deployed to preview May 8, 2024 10:11 View deployment

vercel bot deployed to Preview – tldraw-docs May 8, 2024 10:12 View deployment

mimecuvalo removed request for SomeHats and MitjaBezensek May 8, 2024 10:15

mimecuvalo marked this pull request as draft May 8, 2024 10:15

fix tests

f3e687d

mimecuvalo temporarily deployed to deploy-staging May 13, 2024 08:24 — with GitHub Actions Inactive

vercel bot deployed to Preview – examples May 13, 2024 08:27 View deployment

github-actions bot deployed to preview May 13, 2024 08:27 View deployment

vercel bot deployed to Preview – tldraw-docs May 13, 2024 08:28 View deployment

maybe fix test

c14be22

mimecuvalo temporarily deployed to deploy-staging May 13, 2024 10:24 — with GitHub Actions Inactive

vercel bot deployed to Preview – examples May 13, 2024 10:27 View deployment

github-actions bot deployed to preview May 13, 2024 10:27 View deployment

ugh, try again

136f8cd

mimecuvalo temporarily deployed to deploy-staging May 13, 2024 10:34 — with GitHub Actions Inactive

vercel bot deployed to Preview – examples May 13, 2024 10:37 View deployment

github-actions bot deployed to preview May 13, 2024 10:37 View deployment

ds300 reviewed May 13, 2024

View reviewed changes

ds300 approved these changes May 13, 2024

View reviewed changes

make private

0556f0b

mimecuvalo temporarily deployed to deploy-staging May 15, 2024 13:54 — with GitHub Actions Inactive

vercel bot deployed to Preview – examples May 15, 2024 13:57 View deployment

github-actions bot deployed to preview May 15, 2024 13:58 View deployment

api docs

f95eaac

mimecuvalo temporarily deployed to deploy-staging May 15, 2024 14:05 — with GitHub Actions Inactive

api tweak

ddc890d

mimecuvalo temporarily deployed to deploy-staging May 15, 2024 14:06 — with GitHub Actions Inactive

vercel bot deployed to Preview – examples May 15, 2024 14:10 View deployment

github-actions bot deployed to preview May 15, 2024 14:13 View deployment

mimecuvalo added this pull request to the merge queue May 17, 2024

Merged via the queue into main with commit b4c1f60 May 17, 2024
10 checks passed

mimecuvalo deleted the mime/focus-management branch May 17, 2024 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

focus: rework and untangle existing focus management logic in the sdk #3718

focus: rework and untangle existing focus management logic in the sdk #3718

mimecuvalo commented May 8, 2024 •

edited

vercel bot commented May 8, 2024 •

edited

mimecuvalo May 8, 2024

mimecuvalo May 8, 2024

ds300 May 13, 2024

mimecuvalo May 8, 2024

mimecuvalo May 8, 2024

mimecuvalo May 8, 2024

ds300 May 13, 2024

mimecuvalo May 15, 2024

mimecuvalo May 8, 2024

mimecuvalo May 8, 2024

mimecuvalo May 8, 2024

mimecuvalo May 8, 2024

mimecuvalo May 8, 2024

ds300 May 13, 2024 •

edited

mimecuvalo May 15, 2024

mimecuvalo commented May 8, 2024

ds300 May 13, 2024 •

edited

ds300 May 13, 2024

mimecuvalo May 15, 2024

ds300 May 13, 2024

mimecuvalo May 15, 2024

ds300 left a comment

mimecuvalo commented May 15, 2024

focus: rework and untangle existing focus management logic in the sdk #3718

focus: rework and untangle existing focus management logic in the sdk #3718

Conversation

mimecuvalo commented May 8, 2024 • edited

Places affected

Future Changes

Methodology

Change Type

Test Plan

Release Notes

vercel bot commented May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds300 May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimecuvalo commented May 8, 2024

ds300 May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds300 left a comment

Choose a reason for hiding this comment

mimecuvalo commented May 15, 2024

mimecuvalo commented May 8, 2024 •

edited

vercel bot commented May 8, 2024 •

edited

ds300 May 13, 2024 •

edited

ds300 May 13, 2024 •

edited