-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
focus: rework and untangle existing focus management logic in the sdk #3718
Changes from 1 commit
56e2443
6457b53
e34c9d6
27fa83f
f3e687d
eb34263
ef11364
a9a1ae1
eeae2bd
13d97b8
42d5885
c152eae
071bcc3
3083948
c14be22
136f8cd
0556f0b
f95eaac
ddc890d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<any>(-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 <CursorChatInput value={value} setValue={setValue} chatMessage={chatMessage} /> | ||
|
@@ -101,7 +100,6 @@ const CursorChatInput = track(function CursorChatInput({ | |
}) { | ||
const editor = useEditor() | ||
const msg = useTranslation() | ||
const container = useContainer() | ||
|
||
const ref = useRef<HTMLInputElement>(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(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this double raf doesn't seem necessary... but maybe i'm missing something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah your version works in chrome, safari and firefox. cc @TodePond do you remember if this double raf was intentional? |
||
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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,8 +52,8 @@ export function UserPresenceEditor() { | |
onCancel={toggleEditingName} | ||
onBlur={handleBlur} | ||
shouldManuallyMaintainScrollPositionWhenFocused | ||
autofocus | ||
autoselect | ||
autoFocus | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. drive-by rename to make |
||
autoSelect | ||
/> | ||
) : ( | ||
<> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,6 +105,7 @@ import { deriveShapeIdsInCurrentPage } from './derivations/shapeIdsInCurrentPage | |
import { getSvgJsx } from './getSvgJsx' | ||
import { ClickManager } from './managers/ClickManager' | ||
import { EnvironmentManager } from './managers/EnvironmentManager' | ||
import { FocusManager } from './managers/FocusManager' | ||
import { HistoryManager } from './managers/HistoryManager' | ||
import { ScribbleManager } from './managers/ScribbleManager' | ||
import { SideEffectManager } from './managers/SideEffectManager' | ||
|
@@ -178,6 +179,10 @@ export interface TLEditorOptions { | |
* The editor's initial active tool (or other state node id). | ||
*/ | ||
initialState?: string | ||
/** | ||
* Whether to automatically focus the editor when it mounts. | ||
*/ | ||
autoFocus?: boolean | ||
/** | ||
* Whether to infer dark mode from the user's system preferences. Defaults to false. | ||
*/ | ||
|
@@ -193,6 +198,7 @@ export class Editor extends EventEmitter<TLEventMap> { | |
tools, | ||
getContainer, | ||
initialState, | ||
autoFocus, | ||
inferDarkMode, | ||
}: TLEditorOptions) { | ||
super() | ||
|
@@ -656,6 +662,9 @@ export class Editor extends EventEmitter<TLEventMap> { | |
|
||
this.root.enter(undefined, 'initial') | ||
|
||
this.focusManager = new FocusManager(this, autoFocus) | ||
this.disposables.add(this.focusManager.dispose) | ||
|
||
if (this.getInstanceState().followingUserId) { | ||
this.stopFollowingUser() | ||
} | ||
|
@@ -747,6 +756,13 @@ export class Editor extends EventEmitter<TLEventMap> { | |
*/ | ||
readonly sideEffects: SideEffectManager<this> | ||
|
||
/** | ||
* A manager for ensuring correct focus. See {@link FocusManager} for details. | ||
* | ||
* @public | ||
*/ | ||
readonly focusManager: FocusManager | ||
|
||
/** | ||
* Dispose the editor. | ||
* | ||
|
@@ -7947,6 +7963,21 @@ export class Editor extends EventEmitter<TLEventMap> { | |
return this | ||
} | ||
|
||
/** | ||
* Dispatch a focus event. | ||
* | ||
* @example | ||
* ```ts | ||
* editor.focus() | ||
* ``` | ||
* | ||
* @public | ||
*/ | ||
focus(): this { | ||
this.focusManager.focus() | ||
return this | ||
} | ||
Comment on lines
+8040
to
+8053
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder whether this should set the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or really maybe it should be the job of the focus manager There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
||
/** | ||
* A manager for recording multiple click events. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
import type { Editor } from '../Editor' | ||
|
||
/** | ||
* A manager for ensuring correct focus across the editor. | ||
* It will listen for changes in the instance state to make sure the | ||
* container is focused when the editor is focused. | ||
* Also, it will make sure that the focus is on things like text | ||
* labels when the editor is in editing mode. | ||
* | ||
* @public | ||
*/ | ||
export class FocusManager { | ||
private disposeStoreListener?: () => void | ||
|
||
constructor( | ||
public editor: Editor, | ||
autoFocus?: boolean | ||
) { | ||
this.disposeStoreListener = editor.store.listen( | ||
mimecuvalo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
({ changes }) => { | ||
for (const [prev, next] of Object.values(changes.updated)) { | ||
if (prev.typeName !== 'instance' || next.typeName !== 'instance') continue | ||
|
||
if (prev.isFocused !== next.isFocused) { | ||
next.isFocused ? this.focus() : this.blur() | ||
} | ||
} | ||
}, | ||
{ scope: 'session' } | ||
) | ||
|
||
const currentFocusState = editor.getInstanceState().isFocused | ||
if (autoFocus !== currentFocusState) { | ||
editor.updateInstanceState({ isFocused: autoFocus }) | ||
} | ||
} | ||
|
||
focus() { | ||
this.editor.getContainer().focus() | ||
} | ||
|
||
blur() { | ||
this.editor.complete() // stop any interaction | ||
this.editor.getContainer().blur() // blur the container | ||
} | ||
|
||
dispose() { | ||
this.disposeStoreListener?.() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already the default