From f4bd157b02bc5ea3d1756145dd4a0399ca75bfe4 Mon Sep 17 00:00:00 2001 From: Alf Eaton Date: Thu, 12 Oct 2023 09:38:31 +0100 Subject: [PATCH] Improve context menu behaviour in the file tree (#15142) * Tidy up menu button * Unselect all items when clicking in the file tree root * Close the context menu when selecting a new item * Avoid selecting multiple items with Ctrl+click on macOS GitOrigin-RevId: b67a724909668ec13d7a6d09ffc31574cb42238f --- .../file-tree/components/file-tree-inner.tsx | 5 ++- .../file-tree-item/file-tree-item-menu.jsx | 30 +++++--------- .../contexts/file-tree-selectable.jsx | 26 +++++++++++-- .../js/infrastructure/without-propagation.js | 6 --- .../components/file-tree-root.test.jsx | 17 ++++++-- .../file-tree/flows/context-menu.test.jsx | 39 +++++++++++++++++++ 6 files changed, 87 insertions(+), 36 deletions(-) delete mode 100644 services/web/frontend/js/infrastructure/without-propagation.js diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-inner.tsx b/services/web/frontend/js/features/file-tree/components/file-tree-inner.tsx index 3de53a4817..4eb34ace23 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-inner.tsx +++ b/services/web/frontend/js/features/file-tree/components/file-tree-inner.tsx @@ -1,4 +1,5 @@ import { useFileTreeSelectable } from '../contexts/file-tree-selectable' +import { useCallback } from 'react' type FileTreeInnerProps = { children: React.ReactNode @@ -7,9 +8,9 @@ type FileTreeInnerProps = { function FileTreeInner({ children }: FileTreeInnerProps) { const { setIsRootFolderSelected } = useFileTreeSelectable() - const handleFileTreeClick = () => { + const handleFileTreeClick = useCallback(() => { setIsRootFolderSelected(true) - } + }, [setIsRootFolderSelected]) return ( // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-item/file-tree-item-menu.jsx b/services/web/frontend/js/features/file-tree/components/file-tree-item/file-tree-item-menu.jsx index 106e55ef61..32afe6d251 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-item/file-tree-item-menu.jsx +++ b/services/web/frontend/js/features/file-tree/components/file-tree-item/file-tree-item-menu.jsx @@ -1,23 +1,20 @@ -import { useState } from 'react' -import { findDOMNode } from 'react-dom' +import { useRef } from 'react' import PropTypes from 'prop-types' import { useTranslation } from 'react-i18next' -import withoutPropagation from '../../../../infrastructure/without-propagation' -import { Button } from 'react-bootstrap' import Icon from '../../../../shared/components/icon' import { useFileTreeMainContext } from '../../contexts/file-tree-main' function FileTreeItemMenu({ id }) { const { t } = useTranslation() - const { contextMenuCoords, setContextMenuCoords } = useFileTreeMainContext() - const [dropdownTarget, setDropdownTarget] = useState() + const menuButtonRef = useRef() - function handleClick(_ev) { - const target = dropdownTarget.getBoundingClientRect() + function handleClick(event) { + event.stopPropagation() if (!contextMenuCoords) { + const target = menuButtonRef.current.getBoundingClientRect() setContextMenuCoords({ top: target.top + target.height / 2, left: target.right, @@ -27,25 +24,16 @@ function FileTreeItemMenu({ id }) { } } - const menuButtonRef = component => { - if (component) { - // eslint-disable-next-line react/no-find-dom-node - setDropdownTarget(findDOMNode(component)) - } - } - return (
- +
) } diff --git a/services/web/frontend/js/features/file-tree/contexts/file-tree-selectable.jsx b/services/web/frontend/js/features/file-tree/contexts/file-tree-selectable.jsx index b86a7b86e5..0f2de76b30 100644 --- a/services/web/frontend/js/features/file-tree/contexts/file-tree-selectable.jsx +++ b/services/web/frontend/js/features/file-tree/contexts/file-tree-selectable.jsx @@ -18,6 +18,7 @@ import { useEditorContext } from '../../../shared/context/editor-context' import { useLayoutContext } from '../../../shared/context/layout-context' import usePersistedState from '../../../shared/hooks/use-persisted-state' import usePreviousValue from '../../../shared/hooks/use-previous-value' +import { useFileTreeMainContext } from '@/features/file-tree/contexts/file-tree-main' const FileTreeSelectableContext = createContext() @@ -31,7 +32,7 @@ function fileTreeSelectableReadWriteReducer(selectedEntityIds, action) { switch (action.type) { case ACTION_TYPES.SELECT: { // reset selection - return new Set([action.id]) + return new Set(Array.isArray(action.id) ? action.id : [action.id]) } case ACTION_TYPES.MULTI_SELECT: { @@ -207,8 +208,11 @@ const editorContextPropTypes = { permissionsLevel: PropTypes.oneOf(['readOnly', 'readAndWrite', 'owner']), } +const isMac = /Mac/.test(window.navigator?.platform) + export function useSelectableEntity(id, isFile) { const { view, setView } = useLayoutContext(layoutContextPropTypes) + const { setContextMenuCoords } = useFileTreeMainContext() const { selectedEntityIds, selectOrMultiSelectEntity, @@ -221,18 +225,32 @@ export function useSelectableEntity(id, isFile) { const handleEvent = useCallback( ev => { ev.stopPropagation() + // use Command (macOS) or Ctrl (other OS) to select multiple items, + // as long as the root folder wasn't selected + const multiSelect = + !isRootFolderSelected && (isMac ? ev.metaKey : ev.ctrlKey) setIsRootFolderSelected(false) - selectOrMultiSelectEntity(id, ev.ctrlKey || ev.metaKey) + selectOrMultiSelectEntity(id, multiSelect) setView(isFile ? 'file' : 'editor') }, - [id, setIsRootFolderSelected, selectOrMultiSelectEntity, setView, isFile] + [ + id, + isRootFolderSelected, + setIsRootFolderSelected, + selectOrMultiSelectEntity, + setView, + isFile, + ] ) const handleClick = useCallback( ev => { handleEvent(ev) + if (!ev.ctrlKey && !ev.metaKey) { + setContextMenuCoords(null) + } }, - [handleEvent] + [handleEvent, setContextMenuCoords] ) const handleKeyPress = useCallback( diff --git a/services/web/frontend/js/infrastructure/without-propagation.js b/services/web/frontend/js/infrastructure/without-propagation.js deleted file mode 100644 index 5b03c82f01..0000000000 --- a/services/web/frontend/js/infrastructure/without-propagation.js +++ /dev/null @@ -1,6 +0,0 @@ -export default function withoutPropagation(callback) { - return ev => { - ev.stopPropagation() - if (callback) callback(ev) - } -} diff --git a/services/web/test/frontend/features/file-tree/components/file-tree-root.test.jsx b/services/web/test/frontend/features/file-tree/components/file-tree-root.test.jsx index 7932953da1..2935ad4de0 100644 --- a/services/web/test/frontend/features/file-tree/components/file-tree-root.test.jsx +++ b/services/web/test/frontend/features/file-tree/components/file-tree-root.test.jsx @@ -347,9 +347,9 @@ describe('', function () { ) // select the sub file - const mainDoc = screen.getByRole('treeitem', { name: 'sub.tex' }) - fireEvent.click(mainDoc) - expect(mainDoc.getAttribute('aria-selected')).to.equal('true') + const subDoc = screen.getByRole('treeitem', { name: 'sub.tex' }) + fireEvent.click(subDoc) + expect(subDoc.getAttribute('aria-selected')).to.equal('true') // click on empty area fireEvent.click(screen.getByTestId('file-tree-inner')) @@ -394,5 +394,16 @@ describe('', function () { expect(newItem.parentNode).to.equal(rootEl) }) + + it('starts a new selection', function () { + const subDoc = screen.getByRole('treeitem', { name: 'sub.tex' }) + expect(subDoc.getAttribute('aria-selected')).to.equal('false') + + const mainDoc = screen.getByRole('treeitem', { name: 'main.tex' }) + fireEvent.click(mainDoc, { ctrlKey: true }) + expect(mainDoc.getAttribute('aria-selected')).to.equal('true') + + expect(subDoc.getAttribute('aria-selected')).to.equal('false') + }) }) }) diff --git a/services/web/test/frontend/features/file-tree/flows/context-menu.test.jsx b/services/web/test/frontend/features/file-tree/flows/context-menu.test.jsx index 4710b5847c..3e424210fb 100644 --- a/services/web/test/frontend/features/file-tree/flows/context-menu.test.jsx +++ b/services/web/test/frontend/features/file-tree/flows/context-menu.test.jsx @@ -59,6 +59,45 @@ describe('FileTree Context Menu Flow', function () { screen.getByRole('menu') }) + it('closes when a new selection is started', async function () { + const rootFolder = [ + { + _id: 'root-folder-id', + name: 'rootFolder', + docs: [ + { _id: '456def', name: 'main.tex' }, + { _id: '456def', name: 'foo.tex' }, + ], + folders: [], + fileRefs: [], + }, + ] + renderWithEditorContext( + null} + setRefProviderEnabled={() => null} + setStartedFreeTrial={() => null} + onSelect={onSelect} + onInit={onInit} + isConnected + />, + { + rootFolder, + projectId: '123abc', + rootDocId: '456def', + } + ) + const treeitem = screen.getByRole('button', { name: 'main.tex' }) + expect(screen.queryByRole('menu')).to.be.null + + fireEvent.contextMenu(treeitem) + screen.getByRole('menu') + + screen.getByRole('button', { name: 'foo.tex' }).click() + expect(screen.queryByRole('menu')).to.be.null + }) + it("doesn't open in read only mode", async function () { const rootFolder = [ {