From 158d8a58391dfdc8918854ca610df362ffefaa2b Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 22 Jul 2026 16:38:18 +0100 Subject: [PATCH] Merge pull request #35646 from overleaf/jpa-fix-input-selection [web] fix race condition of input selection and user edit GitOrigin-RevId: 2180fba1f77da2d13f216d33d16f49ee2dd58442 --- .../file-tree-create-name-input.tsx | 41 +-- .../file-tree-modal-create-file.spec.tsx | 280 +++++++++++++++++- 2 files changed, 294 insertions(+), 27 deletions(-) diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-create-name-input.tsx b/services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-create-name-input.tsx index df62a65a46..fe5db4267b 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-create-name-input.tsx +++ b/services/web/frontend/js/features/file-tree/components/file-tree-create/file-tree-create-name-input.tsx @@ -1,4 +1,4 @@ -import { useRef, useEffect, useCallback } from 'react' +import { useRef, useEffect } from 'react' import { useTranslation } from 'react-i18next' import { useFileTreeCreateName } from '../../contexts/file-tree-create-name' import { @@ -36,34 +36,38 @@ export default function FileTreeCreateNameInput({ // the value is stored in a context provider, so it's available elsewhere in the form const { name, setName, touchedName, validName } = useFileTreeCreateName() - const nameFocusedOnce = useRef(false) + const touchedNameRef = useRef(touchedName) + touchedNameRef.current = touchedName // focus the first part of the filename if needed const inputRef = useRef(null) useEffect(() => { - if (inputRef.current && focusName) { + const input = inputRef.current + if (input && focusName) { + const { selectionStart, selectionEnd } = input + // Defer the focus so it is not overridden while the modal is opening. window.requestAnimationFrame(() => { - if (inputRef.current) { - inputRef.current.focus() + const input = inputRef.current + if (!input) { + return // The input has been unmounted before the focus could be applied. } + if (touchedNameRef.current) { + return // The name has been edited already; focusing and selecting the initial name now would clobber that edit. + } + if ( + input.selectionStart !== selectionStart || + input.selectionEnd !== selectionEnd + ) { + return // The selection has been changed already (e.g. a select-all ahead of deleting the name); selecting the initial name now would clobber that change. + } + input.focus() + // Select the name part so that typing keeps the extension. + input.setSelectionRange(0, input.value.lastIndexOf('.')) }) } }, [focusName]) - const onFocus = useCallback(() => { - if (nameFocusedOnce.current) return - if (!focusName) return - window.requestAnimationFrame(() => { - if (!inputRef.current) return - nameFocusedOnce.current = true - inputRef.current.setSelectionRange( - 0, - inputRef.current.value.lastIndexOf('.') - ) - }) - }, [focusName]) - return ( {label || t('file_name')} @@ -75,7 +79,6 @@ export default function FileTreeCreateNameInput({ onChange={event => setName(event.target.value)} ref={inputRef} disabled={inFlight} - onFocus={onFocus} /> {touchedName && !validName && (
diff --git a/services/web/test/frontend/features/file-tree/components/file-tree-create/file-tree-modal-create-file.spec.tsx b/services/web/test/frontend/features/file-tree/components/file-tree-create/file-tree-modal-create-file.spec.tsx index 9bbc20ce70..506f325fad 100644 --- a/services/web/test/frontend/features/file-tree/components/file-tree-create/file-tree-modal-create-file.spec.tsx +++ b/services/web/test/frontend/features/file-tree/components/file-tree-create/file-tree-modal-create-file.spec.tsx @@ -1,10 +1,12 @@ -import { useEffect } from 'react' +import { useEffect, useState } from 'react' import FileTreeModalCreateFile from '../../../../../../frontend/js/features/file-tree/components/modals/file-tree-modal-create-file' import { useFileTreeActionable } from '../../../../../../frontend/js/features/file-tree/contexts/file-tree-actionable' import { useFileTreeData } from '../../../../../../frontend/js/shared/context/file-tree-data-context' import { EditorProviders } from '../../../../helpers/editor-providers' import { FileTreeProvider } from '../../helpers/file-tree-provider' import getMeta from '@/utils/meta' +import CommandPaletteBody from '@/features/command-palette/components/command-palette-body' +import { useCommandProvider } from '@/features/ide-react/hooks/use-command-provider' describe('', function () { it('handles invalid file names', function () { @@ -23,13 +25,6 @@ describe('', function () { cy.get('@submit').should('not.be.disabled') cy.findByRole('alert').should('not.exist') - // Wait for auto-selection of name - cy.get('@input').should($input => { - const elem = $input[0] as HTMLInputElement - expect(elem.selectionStart).to.equal(0) - expect(elem.selectionEnd).to.equal('name.tex'.lastIndexOf('.')) - }) - cy.get('@input').clear() cy.get('@submit').should('be.disabled') cy.findByRole('alert').should('contain.text', 'File name is empty') @@ -203,6 +198,239 @@ describe('', function () { }) }) + it('does not re-select the initial name when it is edited before the deferred selection runs', function () { + cy.intercept('post', '/project/*/doc', { + statusCode: 204, + }).as('createDoc') + + // capture requestAnimationFrame callbacks so the test controls when the + // deferred focus and selection of the initial name run + const requestAnimationFrameCallbacks: FrameRequestCallback[] = [] + const flushRequestAnimationFrameCallbacks = () => { + requestAnimationFrameCallbacks + .splice(0) + .forEach(callback => callback(performance.now())) + } + cy.window().then(win => { + cy.stub(win, 'requestAnimationFrame').callsFake( + (callback: FrameRequestCallback) => { + requestAnimationFrameCallbacks.push(callback) + return requestAnimationFrameCallbacks.length + } + ) + }) + + cy.mount( + + + + + + ) + + cy.findByLabelText('File Name').as('input') + // run the deferred focus, which schedules the selection of "name" + cy.then(flushRequestAnimationFrameCallbacks) + // edit the name before the deferred selection has run + cy.get('@input').clear() + cy.get('@input').type('test.tex') + // run the deferred selection; it must not select the "test" part now that + // the name has been edited + cy.then(flushRequestAnimationFrameCallbacks) + cy.get('@input').type('123') + cy.get('@input').should('have.value', 'test.tex123') + + cy.findByRole('button', { name: 'Create' }).click() + cy.wait('@createDoc') + cy.get('@createDoc').its('request.body').should('deep.equal', { + parent_folder_id: 'root-folder-id', + name: 'test.tex123', + }) + }) + + it('does not select the initial name when it is edited before the deferred focus runs', function () { + // capture requestAnimationFrame callbacks so the test controls when the + // deferred focus and selection of the initial name run + const requestAnimationFrameCallbacks: FrameRequestCallback[] = [] + const flushRequestAnimationFrameCallbacks = () => { + requestAnimationFrameCallbacks + .splice(0) + .forEach(callback => callback(performance.now())) + } + cy.window().then(win => { + cy.stub(win, 'requestAnimationFrame').callsFake( + (callback: FrameRequestCallback) => { + requestAnimationFrameCallbacks.push(callback) + return requestAnimationFrameCallbacks.length + } + ) + }) + + cy.mount( + + + + + + ) + + cy.findByLabelText('File Name').as('input') + // edit the name before the deferred focus has run + cy.get('@input').clear() + cy.get('@input').type('test.tex') + // run the deferred focus; it must not select the initial name now that + // the name has been edited + cy.then(flushRequestAnimationFrameCallbacks) + cy.get('@input').type('123') + cy.get('@input').should('have.value', 'test.tex123') + }) + + it('deletes the whole name when the deferred selection runs between select-all and delete', function () { + // capture requestAnimationFrame callbacks so the test controls when the + // deferred focus and selection of the initial name run + const requestAnimationFrameCallbacks: FrameRequestCallback[] = [] + const flushRequestAnimationFrameCallbacks = () => { + requestAnimationFrameCallbacks + .splice(0) + .forEach(callback => callback(performance.now())) + } + cy.window().then(win => { + cy.stub(win, 'requestAnimationFrame').callsFake( + (callback: FrameRequestCallback) => { + requestAnimationFrameCallbacks.push(callback) + return requestAnimationFrameCallbacks.length + } + ) + }) + + cy.mount( + + + + + + ) + + cy.findByLabelText('File Name').as('input') + // run the deferred focus and selection + cy.then(flushRequestAnimationFrameCallbacks) + // this mirrors cy.clear() -- focus, select-all and delete -- with a frame + // boundary between the select-all and the delete: a deferred selection + // scheduled on focus must not shrink the select-all to the name part, as + // the delete would then leave the extension behind + cy.get('@input').type('{selectall}') + cy.then(flushRequestAnimationFrameCallbacks) + cy.get('@input').type('{del}') + cy.get('@input').should('have.value', '') + }) + + it('deletes the whole name when the deferred initial focus runs between select-all and delete', function () { + // capture requestAnimationFrame callbacks so the test controls when the + // deferred focus and selection of the initial name run + const requestAnimationFrameCallbacks: FrameRequestCallback[] = [] + const flushRequestAnimationFrameCallbacks = () => { + requestAnimationFrameCallbacks + .splice(0) + .forEach(callback => callback(performance.now())) + } + cy.window().then(win => { + cy.stub(win, 'requestAnimationFrame').callsFake( + (callback: FrameRequestCallback) => { + requestAnimationFrameCallbacks.push(callback) + return requestAnimationFrameCallbacks.length + } + ) + }) + + cy.mount( + + + + + + ) + + // select-all before the deferred initial focus has run: it must not + // shrink the select-all to the name part, as the delete would then leave + // the extension behind + cy.findByLabelText('File Name').as('input') + cy.get('@input').type('{selectall}') + cy.then(flushRequestAnimationFrameCallbacks) + cy.get('@input').type('{del}') + cy.get('@input').should('have.value', '') + }) + + it('re-focuses the input when focus is temporarily stolen from the modal', function () { + cy.mount( + + + + + + ) + + cy.findByLabelText('File Name').as('input') + // Wait for auto-selection of name + cy.get('@input').should($input => { + const elem = $input[0] as HTMLInputElement + expect(elem.selectionStart).to.equal(0) + expect(elem.selectionEnd).to.equal('name.tex'.lastIndexOf('.')) + }) + + // Simulate focus moving outside the modal after it has opened, e.g. the + // command palette returning focus to the editor after running the + // "new file" command, or the editor stealing focus when the main doc + // loads. The focus trap of the modal pulls the focus back into the modal. + cy.window().then(win => { + const outside = win.document.createElement('button') + win.document.body.appendChild(outside) + outside.focus() + outside.remove() + }) + + // The input must be focused again with the name selected. Ideally only + // the name part (without the extension) would be selected, but the focus + // trap select()s the entire value when pulling the focus back into the + // modal, and there is no upstream option to disable that. Accept both. + cy.get('@input').should($input => { + const elem = $input[0] as HTMLInputElement + expect(elem.ownerDocument.activeElement, 'the input is focused').to.equal( + elem + ) + expect(elem.selectionStart).to.equal(0) + expect(['name.tex'.lastIndexOf('.'), 'name.tex'.length]).to.include( + elem.selectionEnd + ) + }) + }) + + it('focuses and selects the name when opened from the command palette', function () { + cy.mount( + + + + + + ) + + cy.findByRole('menuitem', { name: 'File: New file' }).click() + + // Ideally only the name part (without the extension) would be selected, + // but the focus trap of the modal select()s the entire value when pulling + // the focus back after the command palette returned it to the previously + // focused element. Which of the two wins depends on timing; accept both. + cy.findByLabelText('File Name').should($input => { + const elem = $input[0] as HTMLInputElement + expect(elem.ownerDocument.activeElement, 'the input is focused').to.equal( + elem + ) + expect(elem.selectionStart).to.equal(0) + expect(['name.tex'.lastIndexOf('.'), 'name.tex'.length]).to.include( + elem.selectionEnd + ) + }) + }) + it('imports a new file from a project', function () { getMeta('ol-ExposedSettings').hasLinkedProjectFileFeature = true getMeta('ol-ExposedSettings').hasLinkedProjectOutputFileFeature = true @@ -604,3 +832,39 @@ function OpenWithMode({ mode }: { mode: string }) { return } + +// Renders the real command palette next to the create-file modal, mirroring +// CommandPaletteRoot (the palette unmounts when hidden) and the "new_file" +// command registration from FileTreeActionButtons. +function OpenFromCommandPalette() { + const { startCreatingFile } = useFileTreeActionable() + const { fileCount } = useFileTreeData() + const [showPalette, setShowPalette] = useState(true) + + useCommandProvider( + () => [ + { + id: 'new_file', + label: 'New file', + handler: () => startCreatingFile('doc'), + }, + ], + [startCreatingFile] + ) + + if (!fileCount) { + return null + } + + return ( + <> + {showPalette && ( + setShowPalette(false)} + /> + )} + + + ) +}