From 410f997b075a274b6e8691db4c3831c71602dbe3 Mon Sep 17 00:00:00 2001 From: Jos de Jong Date: Tue, 31 Oct 2023 13:42:06 +0100 Subject: [PATCH] fix: editor sometimes losing track on whether it has focus --- src/lib/components/JSONEditor.svelte | 2 +- .../components/controls/EditableDiv.svelte | 52 ++++++---- .../controls/contextmenu/ContextMenu.svelte | 20 ++-- .../components/controls/createFocusTracker.ts | 8 +- .../modals/repair/JSONRepairComponent.svelte | 4 +- .../modes/tablemode/TableMode.svelte | 36 ++++--- .../components/modes/treemode/JSONKey.svelte | 12 ++- .../components/modes/treemode/TreeMode.svelte | 97 ++++++------------- src/lib/constants.ts | 7 -- .../value/components/BooleanToggle.svelte | 2 +- .../value/components/EditableValue.svelte | 14 ++- src/lib/types.ts | 6 ++ src/lib/utils/domUtils.ts | 14 +++ 13 files changed, 127 insertions(+), 147 deletions(-) diff --git a/src/lib/components/JSONEditor.svelte b/src/lib/components/JSONEditor.svelte index 790956c1..f8a91e82 100644 --- a/src/lib/components/JSONEditor.svelte +++ b/src/lib/components/JSONEditor.svelte @@ -312,7 +312,7 @@ mode = newMode await tick() - focus() + await focus() onChangeMode(newMode) } diff --git a/src/lib/components/controls/EditableDiv.svelte b/src/lib/components/controls/EditableDiv.svelte index 63c83ebf..3d99034d 100644 --- a/src/lib/components/controls/EditableDiv.svelte +++ b/src/lib/components/controls/EditableDiv.svelte @@ -6,15 +6,15 @@ import { keyComboFromEvent } from '$lib/utils/keyBindings.js' import { createDebug } from '$lib/utils/debug.js' import { noop } from 'lodash-es' - import { UPDATE_SELECTION } from '$lib/constants.js' import type { OnFind, OnPaste } from '$lib/types' + import { UpdateSelectionAfterChange } from '$lib/types' import { classnames } from '$lib/utils/cssUtils.js' const debug = createDebug('jsoneditor:EditableDiv') export let value: string export let shortText = false - export let onChange: (newValue: string, updateSelection: string) => void + export let onChange: (newValue: string, updateSelection: UpdateSelectionAfterChange) => void export let onCancel: () => void export let onFind: OnFind export let onPaste: OnPaste = noop @@ -30,16 +30,20 @@ setDomValue(value) // focus - setTimeout(() => { - if (domValue) { - setCursorToEnd(domValue) - - // The refresh method can be used to update the classnames for example - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - domValue.refresh = handleValueInput - } - }) + if (domValue) { + setCursorToEnd(domValue) + + // The refresh method can be used to update the classnames for example + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + domValue.refresh = handleValueInput + + // The cancel method can be used to cancel editing, without firing a change + // when the contents did change in the meantime. It is the same as pressing ESC + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + domValue.cancel = handleCancel + } }) onDestroy(() => { @@ -48,7 +52,7 @@ debug('onDestroy', { closed, value, newValue }) if (!closed && newValue !== value) { - onChange(newValue, UPDATE_SELECTION.NO) + onChange(newValue, UpdateSelectionAfterChange.no) } }) @@ -78,16 +82,20 @@ valueClass = onValueClass(newValue) } + function handleCancel() { + // cancel changes (needed to prevent triggering a change onDestroy) + closed = true + + onCancel() + } + function handleValueKeyDown(event: KeyboardEvent) { event.stopPropagation() const combo = keyComboFromEvent(event) if (combo === 'Escape') { - // cancel changes (needed to prevent triggering a change onDestroy) - closed = true - - onCancel() + handleCancel() } if (combo === 'Enter' || combo === 'Tab') { @@ -95,7 +103,7 @@ closed = true const newValue = getDomValue() - onChange(newValue, UPDATE_SELECTION.NEXT_INSIDE) + onChange(newValue, UpdateSelectionAfterChange.nextInside) } if (combo === 'Ctrl+F') { @@ -133,9 +141,13 @@ if (document.hasFocus() && !closed) { closed = true if (newValue !== value) { - onChange(newValue, UPDATE_SELECTION.SELF) + onChange(newValue, UpdateSelectionAfterChange.self) } else { - onCancel() + // Note that we do not fire an onCancel here: a blur action + // is caused by the user clicking somewhere else. If we apply + // onCancel now, we would override the selection that the user + // wants by clicking somewhere else in the editor (since `blur` + // is occurring *after* `mousedown`). } } } diff --git a/src/lib/components/controls/contextmenu/ContextMenu.svelte b/src/lib/components/controls/contextmenu/ContextMenu.svelte index b1e357bd..441d420d 100644 --- a/src/lib/components/controls/contextmenu/ContextMenu.svelte +++ b/src/lib/components/controls/contextmenu/ContextMenu.svelte @@ -21,18 +21,16 @@ export let items: ContextMenuItem[] export let tip: string | undefined - let refContextMenu + let refContextMenu: HTMLDivElement onMount(() => { - setTimeout(() => { - const firstEnabledButton = [...refContextMenu.querySelectorAll('button')].find( - (button) => !button.disabled - ) + const firstEnabledButton = Array.from(refContextMenu.querySelectorAll('button')).find( + (button) => !button.disabled + ) - if (firstEnabledButton) { - firstEnabledButton.focus() - } - }) + if (firstEnabledButton) { + firstEnabledButton.focus() + } }) const directionByCombo: Record = { @@ -44,9 +42,9 @@ function handleKeyDown(event) { const combo = keyComboFromEvent(event) - const direction = directionByCombo[combo] + const direction: 'Up' | 'Down' | 'Left' | 'Right' | undefined = directionByCombo[combo] - if (typeof direction === 'string') { + if (direction && event.target) { event.preventDefault() const buttons: HTMLButtonElement[] = Array.from( diff --git a/src/lib/components/controls/createFocusTracker.ts b/src/lib/components/controls/createFocusTracker.ts index 2f0f93ff..7f48108a 100644 --- a/src/lib/components/controls/createFocusTracker.ts +++ b/src/lib/components/controls/createFocusTracker.ts @@ -42,9 +42,11 @@ export function createFocusTracker({ // The focusIn handler will cancel any pending blur timer in those cases clearTimeout(blurTimeoutHandle) blurTimeoutHandle = setTimeout(() => { - debug('blur') - focus = false - onBlur() + if (!hasFocus()) { + debug('blur') + focus = false + onBlur() + } }) as unknown as number } } diff --git a/src/lib/components/modals/repair/JSONRepairComponent.svelte b/src/lib/components/modals/repair/JSONRepairComponent.svelte index 3d469caa..0b00eac7 100644 --- a/src/lib/components/modals/repair/JSONRepairComponent.svelte +++ b/src/lib/components/modals/repair/JSONRepairComponent.svelte @@ -53,9 +53,7 @@ if (domTextArea && error) { const position = error.position != null ? error.position : 0 domTextArea.setSelectionRange(position, position) - setTimeout(() => { - domTextArea.focus() - }) + domTextArea.focus() } } diff --git a/src/lib/components/modes/tablemode/TableMode.svelte b/src/lib/components/modes/tablemode/TableMode.svelte index 142dd172..3aa90bfb 100644 --- a/src/lib/components/modes/tablemode/TableMode.svelte +++ b/src/lib/components/modes/tablemode/TableMode.svelte @@ -73,7 +73,8 @@ findParentWithNodeName, getDataPathFromTarget, getWindow, - isChildOfNodeName + isChildOfNodeName, + isEditableDivRef } from '$lib/utils/domUtils.js' import { createDebug } from '$lib/utils/debug.js' import { @@ -697,6 +698,7 @@ } export function focus() { + debug('focus') // with just .focus(), sometimes the input doesn't react on onpaste events // in Chrome when having a large document open and then doing cut/paste. // Calling both .focus() and .select() did solve this issue. @@ -710,7 +712,7 @@ scrollTop = event.target['scrollTop'] } - function handleMouseDown(event: MouseEvent) { + function handleMouseDown(event: MouseEvent & { target: HTMLDivElement }) { const path = event?.target ? getDataPathFromTarget(event.target as HTMLElement) : undefined if (path) { // when clicking inside the current selection, editing a value, do nothing @@ -726,15 +728,10 @@ event.preventDefault() } - // TODO: ugly to have two setTimeout here. Without it, hiddenInput will blur - setTimeout(() => { - setTimeout(() => { - // for example when clicking on the empty area in the main menu - if (!hasFocus && !isChildOfNodeName(event.target, 'BUTTON')) { - focus() - } - }) - }) + // for example when clicking on the empty area in the main menu + if (!isChildOfNodeName(event.target, 'BUTTON') && !event.target.isContentEditable) { + focus() + } } function createDefaultSelection(): JSONSelection | null { @@ -1017,7 +1014,7 @@ return false } - function handleContextMenuFromTableMenu(event: Event) { + function handleContextMenuFromTableMenu(event: Event & { target: HTMLDivElement }) { if (readOnly) { return } @@ -1093,9 +1090,10 @@ const { path, contents } = pastedJson // exit edit mode - updateSelection(createValueSelection(path, false)) - - await tick() + const refEditableDiv = refContents?.querySelector('.jse-editable-div') || null + if (isEditableDivRef(refEditableDiv)) { + refEditableDiv.cancel() + } // replace the value with the JSON object/array const operations: JSONPatchDocument = [ @@ -1107,6 +1105,9 @@ ] handlePatch(operations) + + // TODO: get rid of the setTimeout here + setTimeout(focus) } function handlePasteFromMenu() { @@ -1128,6 +1129,7 @@ function handleClearPastedJson() { debug('clear pasted json') pastedJson = undefined + focus() } function handleRequestRepair() { @@ -1880,10 +1882,6 @@ onClick: handleClearPastedJson } ]} - onClose={() => { - // TODO: the need for setTimeout is ugly - setTimeout(focus) - }} /> {/if} diff --git a/src/lib/components/modes/treemode/JSONKey.svelte b/src/lib/components/modes/treemode/JSONKey.svelte index 19237af2..9efcf7a7 100644 --- a/src/lib/components/modes/treemode/JSONKey.svelte +++ b/src/lib/components/modes/treemode/JSONKey.svelte @@ -11,8 +11,8 @@ import SearchResultHighlighter from './highlight/SearchResultHighlighter.svelte' import EditableDiv from '../../controls/EditableDiv.svelte' import { addNewLineSuffix } from '$lib/utils/domUtils.js' - import { UPDATE_SELECTION } from '$lib/constants.js' import type { ExtendedSearchResultItem, JSONSelection, TreeModeContext } from '$lib/types.js' + import { UpdateSelectionAfterChange } from '$lib/types.js' import type { JSONPath } from 'immutable-json-patch' import ContextMenuPointer from '../../../components/controls/contextmenu/ContextMenuPointer.svelte' import { classnames } from '$lib/utils/cssUtils.js' @@ -28,7 +28,9 @@ $: isKeySelected = selection ? isKeySelection(selection) && isEqual(selection.path, path) : false $: isEditingKey = isKeySelected && isEditingSelection(selection) - function handleKeyDoubleClick(event) { + function handleKeyDoubleClick( + event: MouseEvent & { currentTarget: EventTarget & HTMLDivElement } + ) { if (!isEditingKey && !context.readOnly) { event.preventDefault() context.onSelect(createKeySelection(path, true)) @@ -41,17 +43,17 @@ }) } - function handleChangeValue(newKey, updateSelection) { + function handleChangeValue(newKey: string, updateSelection: UpdateSelectionAfterChange) { const updatedKey = onUpdateKey(key, context.normalization.unescapeValue(newKey)) const updatedPath = initial(path).concat(updatedKey) context.onSelect( - updateSelection === UPDATE_SELECTION.NEXT_INSIDE + updateSelection === UpdateSelectionAfterChange.nextInside ? createValueSelection(updatedPath, false) : createKeySelection(updatedPath, false) ) - if (updateSelection !== UPDATE_SELECTION.SELF) { + if (updateSelection !== UpdateSelectionAfterChange.self) { context.focus() } } diff --git a/src/lib/components/modes/treemode/TreeMode.svelte b/src/lib/components/modes/treemode/TreeMode.svelte index 4af4f787..19ace6ea 100644 --- a/src/lib/components/modes/treemode/TreeMode.svelte +++ b/src/lib/components/modes/treemode/TreeMode.svelte @@ -83,7 +83,8 @@ findParentWithNodeName, getWindow, isChildOf, - isChildOfNodeName + isChildOfNodeName, + isEditableDivRef } from '$lib/utils/domUtils.js' import { convertValue, @@ -1334,7 +1335,7 @@ */ export async function scrollTo(path: JSONPath, scrollToWhenVisible = true): Promise { documentState = expandPath(json, documentState, path) - await tick() // await rerender + await tick() // await rerender (else the element we want to scroll to does not yet exist) const elem = findElement(path) @@ -1570,11 +1571,7 @@ } // set focus to the hidden input, so we can capture quick keys like Ctrl+X, Ctrl+C, Ctrl+V - setTimeout(() => { - if (!activeElementIsChildOf(refJsonEditor)) { - focus() - } - }) + focus() } function handleExpandAll() { @@ -1781,59 +1778,26 @@ if (combo === 'Ctrl+Z') { event.preventDefault() - - // TODO: find a better way to restore focus - // TODO: implement a proper TypeScript solution to check whether this is an element with blur, focus, select - const activeElement = document.activeElement as HTMLInputElement - if (activeElement && activeElement.blur && activeElement.select) { - activeElement.blur() - setTimeout(() => { - handleUndo() - setTimeout(() => activeElement?.select()) - }) - } else { - handleUndo() - } + handleUndo() } if (combo === 'Ctrl+Shift+Z') { event.preventDefault() - - // TODO: find a better way to restore focus - // TODO: implement a proper TypeScript solution to check whether this is an element with blur, focus, select - const activeElement = document.activeElement as HTMLInputElement - if (activeElement && activeElement.blur && activeElement.select) { - activeElement.blur() - setTimeout(() => { - handleRedo() - setTimeout(() => activeElement?.select()) - }) - } else { - handleRedo() - } + handleRedo() } } - function handleMouseDown(event) { + function handleMouseDown(event: MouseEvent & { target: HTMLDivElement }) { debug('handleMouseDown', event) - // TODO: ugly to have two setTimeout here. Without it, hiddenInput will blur - setTimeout(() => { - setTimeout(() => { - if (!hasFocus && !isChildOfNodeName(event.target, 'BUTTON')) { - // for example when clicking on the empty area in the main menu - focus() - - if ( - !documentState.selection && - json === undefined && - (text === '' || text === undefined) - ) { - createDefaultSelection() - } - } - }) - }) + if (!isChildOfNodeName(event.target, 'BUTTON') && !event.target.isContentEditable) { + // for example when clicking on the empty area in the main menu + focus() + + if (!documentState.selection && json === undefined && (text === '' || text === undefined)) { + createDefaultSelection() + } + } } function openContextMenu({ @@ -1966,11 +1930,13 @@ } const { path, contents } = pastedJson + pastedJson = undefined // exit edit mode - updateSelection(createValueSelection(path, false)) - - await tick() + const refEditableDiv = refContents?.querySelector('.jse-editable-div') || null + if (isEditableDivRef(refEditableDiv)) { + refEditableDiv.cancel() + } // replace the value with the JSON object/array const operations: JSONPatchDocument = [ @@ -1986,11 +1952,15 @@ state: expandRecursive(patchedJson, patchedState, path) } }) + + // TODO: get rid of the setTimeout here + setTimeout(focus) } function handleClearPastedJson() { debug('clear pasted json') pastedJson = undefined + focus() } function handleRequestRepair() { @@ -2035,17 +2005,10 @@ refHiddenInput.blur() } - // This is ugly: we need to wait until the EditableDiv has triggered onSelect, - // and have onSelect call refHiddenInput.focus(). After that we can call blur() - // to remove the focus. - // TODO: find a better solution - tick().then(() => { - setTimeout(() => { - if (refHiddenInput) { - refHiddenInput.blur() - } - }) - }) + debug('blur (outside editor)') + if (refHiddenInput) { + refHiddenInput.blur() + } } } } @@ -2240,10 +2203,6 @@ onClick: handleClearPastedJson } ]} - onClose={() => { - // TODO: the need for setTimeout is ugly - setTimeout(focus) - }} /> {/if} diff --git a/src/lib/constants.ts b/src/lib/constants.ts index 70817677..e913f453 100644 --- a/src/lib/constants.ts +++ b/src/lib/constants.ts @@ -68,13 +68,6 @@ export const JSON_STATUS_INVALID = 'invalid' export const CONTEXT_MENU_HEIGHT = (40 + 2) * 8 // px export const CONTEXT_MENU_WIDTH = 260 // px -// TODO: change UPDATE_SELECTION into an enum -export const UPDATE_SELECTION = { - NO: 'NO', - SELF: 'SELF', - NEXT_INSIDE: 'NEXT_INSIDE' -} - export const SORT_DIRECTION_NAMES = { [SortDirection.asc]: 'ascending', [SortDirection.desc]: 'descending' diff --git a/src/lib/plugins/value/components/BooleanToggle.svelte b/src/lib/plugins/value/components/BooleanToggle.svelte index 193758c6..f0cd8058 100644 --- a/src/lib/plugins/value/components/BooleanToggle.svelte +++ b/src/lib/plugins/value/components/BooleanToggle.svelte @@ -28,7 +28,7 @@ } ]) - setTimeout(focus) + focus() } diff --git a/src/lib/plugins/value/components/EditableValue.svelte b/src/lib/plugins/value/components/EditableValue.svelte index ac835981..50dc1c11 100644 --- a/src/lib/plugins/value/components/EditableValue.svelte +++ b/src/lib/plugins/value/components/EditableValue.svelte @@ -7,16 +7,16 @@ import { createValueSelection, getFocusPath } from '$lib/logic/selection.js' import { getValueClass } from '$lib/plugins/value/components/utils/getValueClass.js' import EditableDiv from '../../../components/controls/EditableDiv.svelte' - import { UPDATE_SELECTION } from '$lib/constants.js' import type { FindNextInside, JSONParser, OnFind, + OnJSONSelect, OnPasteJson, OnPatch, - OnJSONSelect, ValueNormalization } from '$lib/types.js' + import { UpdateSelectionAfterChange } from '$lib/types.js' import { isEqual } from 'lodash-es' export let path: JSONPath @@ -35,7 +35,7 @@ return enforceString ? value : stringConvert(value, parser) } - function handleChangeValue(newValue: string, updateSelection: string) { + function handleChangeValue(newValue: string, updateSelection: UpdateSelectionAfterChange) { onPatch( [ { @@ -53,7 +53,7 @@ } const selection = - updateSelection === UPDATE_SELECTION.NEXT_INSIDE + updateSelection === UpdateSelectionAfterChange.nextInside ? findNextInside(path) : createValueSelection(path, false) @@ -66,9 +66,7 @@ } ) - if (updateSelection !== UPDATE_SELECTION.SELF) { - focus() - } + focus() } function handleCancelChange() { @@ -82,7 +80,7 @@ if (isObjectOrArray(pastedJson)) { onPasteJson({ path, - contents: pastedJson + contents: pastedJson as JSONValue }) } } catch (err) { diff --git a/src/lib/types.ts b/src/lib/types.ts index ec1a64ea..7fbe2c14 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -574,6 +574,12 @@ export enum SortDirection { desc = 'desc' } +export enum UpdateSelectionAfterChange { + no = 'no', + self = 'self', + nextInside = 'nextInside' +} + export interface TableCellIndex { rowIndex: number columnIndex: number diff --git a/src/lib/utils/domUtils.ts b/src/lib/utils/domUtils.ts index c57a1b4b..292eb218 100644 --- a/src/lib/utils/domUtils.ts +++ b/src/lib/utils/domUtils.ts @@ -410,3 +410,17 @@ export function findNearestElement({ return undefined } + +export interface EditableDivElement extends HTMLDivElement { + refresh: () => void + cancel: () => void +} + +export function isEditableDivRef(element: Element | null): element is EditableDivElement { + return ( + !!element && + element.nodeName === 'DIV' && + typeof (element as unknown as Record).refresh === 'function' && + typeof (element as unknown as Record).cancel === 'function' + ) +}