diff --git a/galata/test/documentation/extension_manager.test.ts-snapshots/extensions-search-documentation-linux.png b/galata/test/documentation/extension_manager.test.ts-snapshots/extensions-search-documentation-linux.png index cf6c1201f9bf..f9804f99f39b 100644 Binary files a/galata/test/documentation/extension_manager.test.ts-snapshots/extensions-search-documentation-linux.png and b/galata/test/documentation/extension_manager.test.ts-snapshots/extensions-search-documentation-linux.png differ diff --git a/galata/test/documentation/general.test.ts-snapshots/interface-left-documentation-linux.png b/galata/test/documentation/general.test.ts-snapshots/interface-left-documentation-linux.png index 0d89c9d02dc4..2ada01e72dd1 100644 Binary files a/galata/test/documentation/general.test.ts-snapshots/interface-left-documentation-linux.png and b/galata/test/documentation/general.test.ts-snapshots/interface-left-documentation-linux.png differ diff --git a/galata/test/documentation/internationalization.test.ts-snapshots/language-change-documentation-linux.png b/galata/test/documentation/internationalization.test.ts-snapshots/language-change-documentation-linux.png index c69798b90ede..ee7afd088614 100644 Binary files a/galata/test/documentation/internationalization.test.ts-snapshots/language-change-documentation-linux.png and b/galata/test/documentation/internationalization.test.ts-snapshots/language-change-documentation-linux.png differ diff --git a/packages/filebrowser-extension/schema/browser.json b/packages/filebrowser-extension/schema/browser.json index 799a4459e897..0d1357b4be34 100644 --- a/packages/filebrowser-extension/schema/browser.json +++ b/packages/filebrowser-extension/schema/browser.json @@ -157,11 +157,6 @@ "keys": ["Accel Shift F"], "selector": "body" }, - { - "command": "filebrowser:go-up", - "keys": ["Backspace"], - "selector": ".jp-DirListing-content .jp-DirListing-itemText" - }, { "command": "filebrowser:delete", "keys": ["Delete"], diff --git a/packages/filebrowser-extension/src/index.ts b/packages/filebrowser-extension/src/index.ts index 05169629bc01..202f061b19a7 100644 --- a/packages/filebrowser-extension/src/index.ts +++ b/packages/filebrowser-extension/src/index.ts @@ -365,6 +365,11 @@ const downloadPlugin: JupyterFrontEndPlugin = { Clipboard.copyToSystem(url); }); }, + isVisible: () => + // So long as this command only handles one file at time, don't show it + // if multiple files are selected. + !!tracker.currentWidget && + Array.from(tracker.currentWidget.selectedItems()).length === 1, icon: copyIcon.bindprops({ stylesheet: 'menuItem' }), label: trans.__('Copy Download Link'), mnemonic: 0 @@ -890,14 +895,8 @@ function addCommands( if (model.path === model.rootPath) { return; } - try { - await model.cd('..'); - } catch (reason) { - console.warn( - `${CommandIDs.goUp} failed to go to parent directory of ${model.path}`, - reason - ); - } + + browserForPath.goUp(); } }); @@ -1076,6 +1075,11 @@ function addCommands( return widget.rename(); } }, + isVisible: () => + // So long as this command only handles one file at time, don't show it + // if multiple files are selected. + !!tracker.currentWidget && + Array.from(tracker.currentWidget.selectedItems()).length === 1, icon: editIcon.bindprops({ stylesheet: 'menuItem' }), label: trans.__('Rename'), mnemonic: 0 @@ -1095,8 +1099,10 @@ function addCommands( Clipboard.copyToSystem(item.value.path); }, isVisible: () => + // So long as this command only handles one file at time, don't show it + // if multiple files are selected. !!tracker.currentWidget && - !tracker.currentWidget.selectedItems().next().done, + Array.from(tracker.currentWidget.selectedItems()).length === 1, icon: fileIcon.bindprops({ stylesheet: 'menuItem' }), label: trans.__('Copy Path') }); diff --git a/packages/filebrowser/src/browser.ts b/packages/filebrowser/src/browser.ts index 4123f769f3da..6fe14e6eb8b2 100644 --- a/packages/filebrowser/src/browser.ts +++ b/packages/filebrowser/src/browser.ts @@ -79,8 +79,6 @@ export class FileBrowser extends SidePanel { this._trans.__('file browser') ); - this._directoryPending = false; - // File browser widgets container this.mainPanel = new Panel(); this.mainPanel.addClass(FILE_BROWSER_PANEL_CLASS); @@ -263,63 +261,52 @@ export class FileBrowser extends SidePanel { return this.listing.paste(); } - /** - * Create a new directory - */ - createNewDirectory(): void { - if (this._directoryPending === true) { - return; + private _createNew( + future: Private.FutureNewDirectoryItem, + createOptions: Contents.ICreateOptions + ): Promise { + if (future.pending === true) { + return future.promise; } - this._directoryPending = true; - // TODO: We should provide a hook into when the - // directory is done being created. This probably - // means storing a pendingDirectory promise and - // returning that if there is already a directory - // request. - void this._manager - .newUntitled({ - path: this.model.path, - type: 'directory' - }) - .then(async model => { - await this.listing.selectItemByName(model.name); + const newItem = { + pending: true, + promise: this._manager.newUntitled(createOptions).then(async model => { + await this.listing.selectItemByName(model.name, true); await this.rename(); - this._directoryPending = false; + return model; }) + }; + Object.assign(future, newItem); + const { promise } = newItem; + promise .catch(err => { void showErrorMessage(this._trans.__('Error'), err); - this._directoryPending = false; + }) + .finally(() => { + future.pending = false; }); + return promise; + } + + /** + * Create a new directory + */ + createNewDirectory(): Promise { + return this._createNew(this._futureNewDirectory, { + path: this.model.path, + type: 'directory' + }); } /** * Create a new file */ - createNewFile(options: FileBrowser.IFileOptions): void { - if (this._filePending === true) { - return; - } - this._filePending = true; - // TODO: We should provide a hook into when the - // file is done being created. This probably - // means storing a pendingFile promise and - // returning that if there is already a file - // request. - void this._manager - .newUntitled({ - path: this.model.path, - type: 'file', - ext: options.ext - }) - .then(async model => { - await this.listing.selectItemByName(model.name); - await this.rename(); - this._filePending = false; - }) - .catch(err => { - void showErrorMessage(this._trans.__('Error'), err); - this._filePending = false; - }); + createNewFile(options: FileBrowser.IFileOptions): Promise { + return this._createNew(this._futureNewFile, { + path: this.model.path, + type: 'file', + ext: options.ext + }); } /** @@ -347,6 +334,15 @@ export class FileBrowser extends SidePanel { return this.listing.download(); } + /** + * cd .. + * + * Go up one level in the directory tree. + */ + async goUp() { + return this.listing.goUp(); + } + /** * Shut down kernels on the applicable currently selected items. * @@ -420,8 +416,14 @@ export class FileBrowser extends SidePanel { private _filenameSearcher: ReactWidget; private _manager: IDocumentManager; - private _directoryPending: boolean; - private _filePending: boolean; + private _futureNewDirectory: Private.FutureNewDirectoryItem = { + pending: false, + promise: null + }; + private _futureNewFile: Private.FutureNewDirectoryItem = { + pending: false, + promise: null + }; private _navigateToCurrentDirectory: boolean; private _showLastModifiedColumn: boolean = true; private _useFuzzyFilter: boolean = true; @@ -480,3 +482,21 @@ export namespace FileBrowser { ext: string; } } + +namespace Private { + /** + * A special type with the following properties: + * - When `pending` is `true`, the `promise` field points to a promise for a + * new directory or file. + * - When `pending` is `false`, the `promise` field has no guarantee. + */ + export type FutureNewDirectoryItem = + | { + pending: true; + promise: Promise; + } + | { + pending: false; + promise: any; + }; +} diff --git a/packages/filebrowser/src/listing.ts b/packages/filebrowser/src/listing.ts index 21dfcc02fea5..798c940b1331 100644 --- a/packages/filebrowser/src/listing.ts +++ b/packages/filebrowser/src/listing.ts @@ -317,7 +317,85 @@ export class DirListing extends Widget { * @returns A promise that resolves with the new name of the item. */ rename(): Promise { - return this._doRename(); + const selectedPaths = Object.keys(this.selection); + if (selectedPaths.length === 0) { + // Bail if nothing has been selected. + return Promise.resolve(''); + } + this._inRename = true; + const items = this._sortedItems; + let { path } = items[this._focusIndex]; + if (!this.selection[path]) { + // If the currently focused item is not selected, then choose the last + // selected item. + path = selectedPaths.slice(-1)[0]; + } + const index = ArrayExt.findFirstIndex(items, value => value.path === path); + const row = this._items[index]; + const item = items[index]; + const nameNode = this.renderer.getNameNode(row); + const original = item.name; + this._editNode.value = original; + this._selectItem(index, false); + + return Private.doRename(nameNode, this._editNode, original) + .then(newName => { + if (!newName || newName === original) { + return original; + } + if (!isValidFileName(newName)) { + void showErrorMessage( + this._trans.__('Rename Error'), + Error( + this._trans._p( + 'showErrorMessage', + '"%1" is not a valid name for a file. Names must have nonzero length, and cannot include "/", "\\", or ":"', + newName + ) + ) + ); + return original; + } + + if (this.isDisposed) { + throw new Error('File browser is disposed.'); + } + + const manager = this._manager; + const oldPath = PathExt.join(this._model.path, original); + const newPath = PathExt.join(this._model.path, newName); + const promise = renameFile(manager, oldPath, newPath); + return promise.then( + () => newName, + error => { + if (error !== 'File not renamed') { + void showErrorMessage( + this._trans._p('showErrorMessage', 'Rename Error'), + error + ); + } + return original; + } + ); + }) + .then(filename => { + if (this.isDisposed) { + throw new Error('File browser is disposed'); + } + // If nothing else has been selected, then select the renamed file. In + // other words, don't select the renamed file if the user has clicked + // away to some other file. + if ( + Object.keys(this.selection).length === 1 && + this.selection[original] + ) { + return this.selectItemByName(filename, true).then(() => filename); + } + return filename; + }) + .finally(() => { + this._inRename = false; + }); } /** @@ -420,6 +498,13 @@ export class DirListing extends Widget { if (!this.isDisposed && result.button.accept) { await this._delete(items.map(item => item.path)); } + + // Re-focus + let focusIndex = this._focusIndex; + if (focusIndex > this._sortedItems.length - items.length - 1) { + focusIndex = Math.max(0, this._focusIndex - 1); + } + this._focusItem(focusIndex); } /** @@ -611,7 +696,7 @@ export class DirListing extends Widget { * Select an item by name. * * @param name - The name of the item to select. - * @param focus - Whether to move focus the selected item. + * @param focus - Whether to move focus to the selected item. * * @returns A promise that resolves when the name is selected. */ @@ -782,16 +867,25 @@ export class DirListing extends Widget { content.appendChild(node); } - // Remove extra classes from the nodes. - nodes.forEach(node => { + nodes.forEach((node, i) => { + // Remove extra classes from the nodes. node.classList.remove(SELECTED_CLASS); node.classList.remove(RUNNING_CLASS); node.classList.remove(CUT_CLASS); + + // Uncheck each file checkbox const checkbox = renderer.getCheckboxNode(node); if (checkbox) { - // Uncheck each file checkbox checkbox.checked = false; } + + // Handle `tabIndex` + const nameNode = renderer.getNameNode(node); + if (nameNode) { + // Gotta check if the name node is there because it gets replaced by the + // edit node when editing the name of the file or directory. + nameNode.tabIndex = i === this._focusIndex ? 0 : -1; + } }); // Put the check-all checkbox in the header into the correct state @@ -810,7 +904,7 @@ export class DirListing extends Widget { checkAllCheckbox.dataset.indeterminate = String(someSelected); } - // Add extra classes to item nodes based on widget state. + // Update item nodes based on widget state. items.forEach((item, i) => { const node = nodes[i]; const ft = this._manager.registry.getFileTypeForModel(item); @@ -821,6 +915,7 @@ export class DirListing extends Widget { this.translator, this._hiddenColumns ); + if (this.selection[item.path]) { node.classList.add(SELECTED_CLASS); @@ -945,6 +1040,12 @@ export class DirListing extends Widget { } } return; + } else { + // Focus the selected file on click to ensure a couple of things: + // 1. If a user clicks on the item node, its name node will receive focus. + // 2. If a user clicks on blank space in the directory listing, the + // previously focussed item will be focussed. + this._focusItem(this._focusIndex); } } @@ -1020,11 +1121,11 @@ export class DirListing extends Widget { } this._softSelection = ''; } - // Re-focus the selected file. This is needed because nodes corresponding - // to files selected in mousedown handler will not retain the focus - // as mousedown event is always followed by a blur/focus event. + // Re-focus. This is needed because nodes corresponding to files selected in + // mousedown handler will not retain the focus as mousedown event is always + // followed by a blur/focus event. if (event.button === 0) { - this._focusSelectedFile(); + this._focusItem(this._focusIndex); } // Remove the drag listeners if necessary. @@ -1081,10 +1182,126 @@ export class DirListing extends Widget { } } + /** + * Calculate the next focus index, given the current focus index and a + * direction, keeping within the bounds of the directory listing. + * + * @param index Current focus index + * @param direction -1 (up) or 1 (down) + * @returns The next focus index, which could be the same as the current focus + * index if at the boundary. + */ + private _getNextFocusIndex(index: number, direction: number): number { + const nextIndex = index + direction; + if (nextIndex === -1 || nextIndex === this._items.length) { + // keep focus index within bounds + return index; + } else { + return nextIndex; + } + } + + /** + * Handle the up or down arrow key. + * + * @param event The keyboard event + * @param direction -1 (up) or 1 (down) + */ + private _handleArrowY(event: KeyboardEvent, direction: number) { + // We only handle the `ctrl` and `shift` modifiers. If other modifiers are + // present, then do nothing. + if (event.altKey || event.metaKey) { + return; + } + + event.stopPropagation(); + event.preventDefault(); + + const focusIndex = this._focusIndex; + let nextFocusIndex = this._getNextFocusIndex(focusIndex, direction); + + // The following if-block allows the first press of the down arrow to select + // the first (rather than the second) file/directory in the list. This is + // the situation when the page first loads or when a user changes directory. + if ( + direction > 0 && + focusIndex === 0 && + !event.ctrlKey && + !event.shiftKey && + Object.keys(this.selection).length === 0 + ) { + nextFocusIndex = 0; + } + + // Shift key indicates multi-selection. Either the user is trying to grow + // the selection, or shrink it. + if (event.shiftKey) { + const nextItem = this._sortedItems[nextFocusIndex]; + // If the next item is not selected, treat this as a normal multi-select action. + if (!this.selection[nextItem.path]) { + this._handleMultiSelect(nextFocusIndex); + } else { + // Else if the next item is selected and... + const item = this._sortedItems[focusIndex]; + const prevItem = this._sortedItems[focusIndex - direction]; + if ( + // Currently focussed item is selected and... + this.selection[item.path] && + // Previous item is boundary or unselected + (!prevItem || !this.selection[prevItem.path]) + ) { + // In other words, if the situation looks like this going up (reverse + // for going down): + // + // - [next item] selected + // - [item currently focussed] selected + // - [previous item] unselected (or boundary) + // + // Then we unselect the currently focussed item. + delete this.selection[item.path]; + } + } + } else if (!event.ctrlKey) { + // If neither the shift nor ctrl keys were used with the up/down arrow, + // then we treat it as a normal, unmodified key press and select the next + // item. + this._selectItem( + nextFocusIndex, + event.shiftKey, + false /* focus = false because we call focus method directly following this */ + ); + } + + this._focusItem(nextFocusIndex); + this.update(); + } + + /** + * cd .. + * + * Go up one level in the directory tree. + */ + async goUp() { + const model = this.model; + if (model.path === model.rootPath) { + return; + } + try { + await model.cd('..'); + } catch (reason) { + console.warn(`Failed to go to parent directory of ${model.path}`, reason); + } + } + /** * Handle the `'keydown'` event for the widget. */ protected evtKeydown(event: KeyboardEvent): void { + // Do not handle any keydown events here if in the middle of a file rename. + if (this._inRename) { + return; + } + switch (event.keyCode) { case 13: { // Enter @@ -1094,37 +1311,78 @@ export class DirListing extends Widget { } event.preventDefault(); event.stopPropagation(); + for (const item of this.selectedItems()) { + this.handleOpen(item); + } + return; + } + case 38: + // Up arrow + this._handleArrowY(event, -1); + return; + case 40: + // Down arrow + this._handleArrowY(event, 1); + return; + case 32: { + // Space + if (event.ctrlKey) { + // Follow the Windows and Ubuntu convention: you must press `ctrl` + + // `space` in order to toggle whether an item is selected. + + // However, do not handle if any other modifiers were pressed. + if (event.metaKey || event.shiftKey || event.altKey) { + return; + } - const selected = Object.keys(this.selection); - const path = selected[0]; - const items = this._sortedItems; - const i = ArrayExt.findFirstIndex(items, value => value.path === path); - if (i === -1) { + // Make sure the ctrl+space key stroke was on a valid, focussed target. + const node = this._items[this._focusIndex]; + if ( + !( + // Event must have occurred within a node whose item can be toggled. + ( + node.contains(event.target as HTMLElement) && + // That node must also contain the currently focussed element. + node.contains(document.activeElement) + ) + ) + ) { + break; + } + + event.stopPropagation(); + // Prevent default, otherwise the container will scroll. + event.preventDefault(); + + // Toggle item selected + const { path } = this._sortedItems[this._focusIndex]; + if (this.selection[path]) { + delete this.selection[path]; + } else { + this.selection[path] = true; + } + + this.update(); + // Key was handled, so return. return; } - - const item = this._sortedItems[i]; - this.handleOpen(item); break; } - case 38: // Up arrow - this.selectPrevious(event.shiftKey); - event.stopPropagation(); - event.preventDefault(); - break; - case 40: // Down arrow - this.selectNext(event.shiftKey); + case 8: + // Backspace + if (event.altKey || event.ctrlKey || event.metaKey || event.shiftKey) { + return; + } event.stopPropagation(); event.preventDefault(); - break; - default: + this.goUp(); break; } // Detects printable characters typed by the user. // Not all browsers support .key, but it discharges us from reconstructing // characters from key codes. - if (!this._inRename && event.key !== undefined && event.key.length === 1) { + if (event.key !== undefined && event.key.length === 1) { if (event.ctrlKey || event.shiftKey || event.altKey || event.metaKey) { return; } @@ -1465,11 +1723,11 @@ export class DirListing extends Widget { } else { this.selection[path] = true; } - + this._focusItem(index); // Handle multiple select. } else if (event.shiftKey) { - this._handleMultiSelect(selected, index); - + this._handleMultiSelect(index); + this._focusItem(index); // Handle a 'soft' selection } else if (path in this.selection && selected.length > 1) { this._softSelection = path; @@ -1477,75 +1735,88 @@ export class DirListing extends Widget { // Default to selecting the only the item. } else { // Select only the given item. - return this._selectItem(index, false); + return this._selectItem(index, false, true); } this.update(); } /** - * (Re-)focus on the selected file. + * (Re)-focus an item in the directory listing. * - * If index is not given, it will be inferred from the current selection; - * providing index saves on the iteration time. - */ - private _focusSelectedFile(index?: number): void { - if (typeof index === 'undefined') { - const selected = Object.keys(this.selection); - if (selected.length > 1) { - // Multiselect - do not focus on any single file - return; - } - index = ArrayExt.findFirstIndex( - this._sortedItems, - value => value.path === selected[0] - ); - } - if (index === -1) { + * @param index The index of the item node to focus + */ + private _focusItem(index: number): void { + if (this._items.length === 0) { + // Focus the top node if the folder is empty and therefore there are no + // items inside the folder to focus. + this.node.focus(); return; } - // Focus on text to make shortcuts works + this._focusIndex = index; const node = this._items[index]; - const text = DOMUtils.findElement(node, ITEM_TEXT_CLASS); - if (text) { - text.focus(); + const nameNode = this.renderer.getNameNode(node); + if (nameNode) { + // Make the filename text node focusable so that it receives keyboard + // events; text node was specifically chosen to receive shortcuts because + // it gets substituted with input element during file name edits which + // conveniently deactivates irrelevant shortcuts. + nameNode.tabIndex = 0; + nameNode.focus(); + } + } + + /** + * Are all of the items between two provided indices selected? + * + * The items at the indices are not considered. + * + * @param j Index of one item. + * @param k Index of another item. Note: may be less or greater than first + * index. + * @returns True if and only if all items between the j and k are selected. + * Returns undefined if j and k are the same. + */ + private _allSelectedBetween(j: number, k: number): boolean | void { + if (j === k) { + return; } + const [start, end] = j < k ? [j + 1, k] : [k + 1, j]; + return this._sortedItems + .slice(start, end) + .reduce((result, item) => result && this.selection[item.path], true); } /** * Handle a multiple select on a file item node. */ - private _handleMultiSelect(selected: string[], index: number): void { - // Find the "nearest selected". + private _handleMultiSelect(index: number): void { const items = this._sortedItems; - let nearestIndex = -1; - for (let i = 0; i < this._items.length; i++) { - if (i === index) { - continue; - } - const path = items[i].path; - if (selected.indexOf(path) !== -1) { - if (nearestIndex === -1) { - nearestIndex = i; - } else { - if (Math.abs(index - i) < Math.abs(nearestIndex - i)) { - nearestIndex = i; - } - } - } - } + const fromIndex = this._focusIndex; + let shouldAdd = true; - // Default to the first element (and fill down). - if (nearestIndex === -1) { - nearestIndex = 0; + const target = items[index]; + if ( + this.selection[target.path] && + this._allSelectedBetween(fromIndex, index) + ) { + shouldAdd = false; } - // Select the rows between the current and the nearest selected. - for (let i = 0; i < this._items.length; i++) { - if ( - (nearestIndex >= i && index <= i) || - (nearestIndex <= i && index >= i) - ) { + // Select (or unselect) the rows between chosen index and the last focussed. + const step = fromIndex < index ? 1 : -1; + for (let i = fromIndex; i !== index + step; i += step) { + if (shouldAdd) { + if (i === fromIndex) { + // Do not change the selection state of the starting (fromIndex) item. + continue; + } this.selection[items[i].path] = true; + } else { + if (i === index) { + // Do not unselect the target item. + continue; + } + delete this.selection[items[i].path]; } } } @@ -1576,79 +1847,6 @@ export class DirListing extends Widget { ); } - /** - * Allow the user to rename item on a given row. - */ - private _doRename(): Promise { - this._inRename = true; - const items = this._sortedItems; - const path = Object.keys(this.selection)[0]; - const index = ArrayExt.findFirstIndex(items, value => value.path === path); - const row = this._items[index]; - const item = items[index]; - const nameNode = this.renderer.getNameNode(row); - const original = item.name; - this._editNode.value = original; - this._selectItem(index, false); - - return Private.doRename(nameNode, this._editNode, original).then( - newName => { - this.node.focus(); - if (!newName || newName === original) { - this._inRename = false; - return original; - } - if (!isValidFileName(newName)) { - void showErrorMessage( - this._trans.__('Rename Error'), - Error( - this._trans._p( - 'showErrorMessage', - '"%1" is not a valid name for a file. Names must have nonzero length, and cannot include "/", "\\", or ":"', - newName - ) - ) - ); - this._inRename = false; - return original; - } - - if (this.isDisposed) { - this._inRename = false; - throw new Error('File browser is disposed.'); - } - - const manager = this._manager; - const oldPath = PathExt.join(this._model.path, original); - const newPath = PathExt.join(this._model.path, newName); - const promise = renameFile(manager, oldPath, newPath); - return promise - .catch(error => { - if (error !== 'File not renamed') { - void showErrorMessage( - this._trans._p('showErrorMessage', 'Rename Error'), - error - ); - } - this._inRename = false; - return original; - }) - .then(() => { - if (this.isDisposed) { - this._inRename = false; - throw new Error('File browser is disposed.'); - } - if (this._inRename) { - // No need to catch because `newName` will always exit. - void this.selectItemByName(newName); - } - this._inRename = false; - return newName; - }); - } - ); - } - /** * Select a given item. */ @@ -1665,8 +1863,8 @@ export class DirListing extends Widget { const path = items[index].path; this.selection[path] = true; - if (!keepExisting && focus) { - this._focusSelectedFile(index); + if (focus) { + this._focusItem(index); } this.update(); } @@ -1700,6 +1898,16 @@ export class DirListing extends Widget { this.clearSelectedItems(); // Update the sorted items. this.sort(this.sortState); + // Reset focus. + // + // Note: this code may be slightly misleading, it may suggest that the focus + // method is being called on the first item after the directory listing has + // refreshed. But actually, because the DOM update gets queued via + // `this.update()`, the focus method is called before the items refresh. The + // only reason this works is because of an implementation detail of this + // class: it reuses DOM nodes where it can, rather than destroying and + // recreating them. + this._focusItem(0); } /** @@ -1768,6 +1976,8 @@ export class DirListing extends Widget { private _inRename = false; private _isDirty = false; private _hiddenColumns = new Set(); + // _focusIndex should never be set outside the range [0, this._items.length - 1] + private _focusIndex = 0; } /** @@ -1949,7 +2159,8 @@ export namespace DirListing { header.className = HEADER_CLASS; node.appendChild(header); node.appendChild(content); - node.tabIndex = 0; + // Set to -1 to allow calling this.node.focus(). + node.tabIndex = -1; return node; } @@ -2090,12 +2301,6 @@ export namespace DirListing { node.appendChild(text); node.appendChild(modified); - // Make the text note focusable so that it receives keyboard events; - // text node was specifically chosen to receive shortcuts because - // text element gets substituted with input area during file name edits - // which conveniently deactivate irrelevant shortcuts. - text.tabIndex = 0; - if (hiddenColumns?.has?.('last_modified')) { modified.classList.add(MODIFIED_COLUMN_HIDDEN); } else { @@ -2124,11 +2329,7 @@ export namespace DirListing { // Wrap the checkbox in a label element in order to increase its hit area. const labelWrapper = document.createElement('label'); labelWrapper.classList.add(CHECKBOX_WRAPPER_CLASS); - // The individual file checkboxes are visible on hover, but the header - // check-all checkbox is always visible. - if (options?.alwaysVisible) { - labelWrapper.classList.add('jp-mod-visible'); - } + const checkbox = document.createElement('input'); checkbox.type = 'checkbox'; // Prevent the user from clicking (via mouse, keyboard, or touch) the @@ -2137,6 +2338,16 @@ export namespace DirListing { checkbox.addEventListener('click', event => { event.preventDefault(); }); + + // The individual file checkboxes are visible on hover, but the header + // check-all checkbox is always visible. + if (options?.alwaysVisible) { + labelWrapper.classList.add('jp-mod-visible'); + } else { + // Disable tabbing to all other checkboxes. + checkbox.tabIndex = -1; + } + labelWrapper.appendChild(checkbox); return labelWrapper; } @@ -2363,7 +2574,7 @@ namespace Private { edit.setSelectionRange(0, index); } - return new Promise((resolve, reject) => { + return new Promise(resolve => { edit.onblur = () => { parent.replaceChild(text, edit); resolve(edit.value); @@ -2380,20 +2591,7 @@ namespace Private { event.preventDefault(); edit.value = original; edit.blur(); - break; - case 38: // Up arrow - event.stopPropagation(); - event.preventDefault(); - if (edit.selectionStart !== edit.selectionEnd) { - edit.selectionStart = edit.selectionEnd = 0; - } - break; - case 40: // Down arrow - event.stopPropagation(); - event.preventDefault(); - if (edit.selectionStart !== edit.selectionEnd) { - edit.selectionStart = edit.selectionEnd = edit.value.length; - } + text.focus(); break; default: break; diff --git a/packages/filebrowser/style/base.css b/packages/filebrowser/style/base.css index e306f7b52c05..fdd56449f473 100644 --- a/packages/filebrowser/style/base.css +++ b/packages/filebrowser/style/base.css @@ -113,11 +113,6 @@ outline: 0; } -.jp-DirListing:focus-visible { - outline: 1px solid var(--jp-brand-color1); - outline-offset: -2px; -} - .jp-DirListing-header { flex: 0 0 auto; display: flex; @@ -273,6 +268,12 @@ user-select: none; } +/* https://css-tricks.com/copy-the-browsers-native-focus-styles/ */ +.jp-DirListing-itemText:focus { + outline: 5px auto Highlight; + outline: 5px auto -webkit-focus-ring-color; +} + .jp-DirListing-itemModified { flex: 0 0 125px; text-align: right; diff --git a/packages/filebrowser/test/browser.spec.ts b/packages/filebrowser/test/browser.spec.ts new file mode 100644 index 000000000000..2f8dcbaf4b54 --- /dev/null +++ b/packages/filebrowser/test/browser.spec.ts @@ -0,0 +1,122 @@ +// Copyright (c) Jupyter Development Team. +// Distributed under the terms of the Modified BSD License. + +import expect from 'expect'; +import { Signal } from '@lumino/signaling'; +import { Widget } from '@lumino/widgets'; +import { DocumentManager, IDocumentManager } from '@jupyterlab/docmanager'; +import { DocumentRegistry, TextModelFactory } from '@jupyterlab/docregistry'; +import { ServiceManager } from '@jupyterlab/services'; +import { Mock, signalToPromise } from '@jupyterlab/testutils'; +import { simulate } from 'simulate-event'; +import { FileBrowser, FilterFileBrowserModel } from '../src'; + +const ITEM_CLASS = 'jp-DirListing-item'; +const EDITOR_CLASS = 'jp-DirListing-editor'; + +class TestFileBrowser extends FileBrowser { + renameCalled = new Signal(this); + rename(...args: any[]) { + const result = super.rename.apply(this, args); + // Allows us to spy on onUpdateRequest. + this.renameCalled.emit(); + return result; + } +} + +describe('filebrowser/browser', () => { + let manager: IDocumentManager; + let serviceManager: ServiceManager.IManager; + let registry: DocumentRegistry; + let model: FilterFileBrowserModel; + + beforeAll(async () => { + const opener = new Mock.DocumentWidgetOpenerMock(); + + registry = new DocumentRegistry({ + textModelFactory: new TextModelFactory() + }); + serviceManager = new Mock.ServiceManagerMock(); + manager = new DocumentManager({ + registry, + opener, + manager: serviceManager + }); + + model = new FilterFileBrowserModel({ manager }); + + const contents = serviceManager.contents; + await contents.newUntitled({ type: 'directory' }); + await contents.newUntitled({ type: 'file' }); + await contents.newUntitled({ type: 'notebook' }); + }); + + describe('FileBrowser', () => { + let fileBrowser: TestFileBrowser; + + beforeEach(() => { + const options: FileBrowser.IOptions = { + model, + id: '' + }; + fileBrowser = new TestFileBrowser(options); + Widget.attach(fileBrowser, document.body); + }); + + describe('#constructor', () => { + it('should return new FileBrowser instance', () => { + expect(fileBrowser).toBeInstanceOf(FileBrowser); + }); + }); + + describe('#createNewDirectory', () => { + it('should focus newly created directory after rename', async () => { + const created = fileBrowser.createNewDirectory(); + await signalToPromise(fileBrowser.renameCalled); + const editNode = document.querySelector(`.${EDITOR_CLASS}`); + if (!editNode) { + throw new Error('Edit node not found'); + } + const itemNode = Array.from( + document.querySelectorAll(`.${ITEM_CLASS}`) + ).find(el => { + return el.contains(editNode); + }); + if (!itemNode) { + throw new Error('Item node not found'); + } + simulate(editNode, 'keydown', { + keyCode: 13, + key: 'Enter' + }); + await created; + expect(itemNode.contains(document.activeElement)).toBe(true); + }); + }); + + describe('#createNewFile', () => { + it('should focus newly created file after rename', async () => { + const created = fileBrowser.createNewFile({ ext: '.txt' }); + await signalToPromise(fileBrowser.renameCalled); + const editNode = document.querySelector(`.${EDITOR_CLASS}`); + if (!editNode) { + throw new Error('Edit node not found'); + } + const itemNode = Array.from( + document.querySelectorAll(`.${ITEM_CLASS}`) + ).find(el => { + return el.contains(editNode); + }); + if (!itemNode) { + throw new Error('Item node not found'); + } + simulate(editNode, 'keydown', { + keyCode: 13, + key: 'Enter' + }); + await created; + expect(itemNode.contains(document.activeElement)).toBe(true); + }); + }); + }); +}); diff --git a/packages/filebrowser/test/listing.spec.ts b/packages/filebrowser/test/listing.spec.ts index e8c3a29a2d48..8b9c0ff0e1bb 100644 --- a/packages/filebrowser/test/listing.spec.ts +++ b/packages/filebrowser/test/listing.spec.ts @@ -1,5 +1,5 @@ -// Copyright (c) Jupyter Development Team. -// Distributed under the terms of the Modified BSD License. +// Copyright (c) Jupyter Development Team. Distributed under the terms of the +// Modified BSD License. import expect from 'expect'; import { simulate } from 'simulate-event'; @@ -32,25 +32,18 @@ class TestDirListing extends DirListing { describe('filebrowser/listing', () => { describe('DirListing', () => { - describe('#constructor', () => { - it('should return new DirListing instance', () => { - const options = createOptionsForConstructor(); - const dirListing = new DirListing(options); - expect(dirListing).toBeInstanceOf(DirListing); - }); - }); - }); - - describe('checkboxes', () => { let dirListing: TestDirListing; beforeEach(async () => { const options = createOptionsForConstructor(); // Start with some files instead of empty before creating the DirListing. - // This makes it easier to test checking/unchecking because after the - // DirListing is created, whenever a file is added, the DirListing selects - // that file, which causes the file's checkbox to be checked. + // This makes it easier to test things, for example checking/unchecking + // because after the DirListing is created, whenever a file is added, the + // DirListing selects that file, which causes the file's checkbox to be + // checked. + await options.model.manager.newUntitled({ type: 'file' }); + await options.model.manager.newUntitled({ type: 'file' }); await options.model.manager.newUntitled({ type: 'file' }); await options.model.manager.newUntitled({ type: 'file' }); @@ -63,228 +56,610 @@ describe('filebrowser/listing', () => { await signalToPromise(dirListing.updated); }); + it('should reflect initial conditions', () => { + // Check initial conditions + const selectedItems = [...dirListing.selectedItems()]; + const sortedItems = [...dirListing.sortedItems()]; + expect(selectedItems).toHaveLength(0); + expect(sortedItems).toHaveLength(4); + }); + afterEach(() => { Widget.detach(dirListing); + jest.restoreAllMocks(); }); - describe('file/item checkbox', () => { - it('check initial conditions', async () => { - expect(Array.from(dirListing.selectedItems())).toHaveLength(0); - expect(Array.from(dirListing.sortedItems())).toHaveLength(2); + describe('#constructor', () => { + it('should return new DirListing instance', () => { + const options = createOptionsForConstructor(); + const dirListing = new DirListing(options); + expect(dirListing).toBeInstanceOf(DirListing); }); + }); - it('should be checked after item is selected', async () => { - const itemNode = dirListing.contentNode.children[0] as HTMLElement; - const checkbox = dirListing.renderer.getCheckboxNode!( - itemNode - ) as HTMLInputElement; - expect(checkbox.checked).toBe(false); + describe('#rename', () => { + it('backspace during rename does not trigger goUp method', async () => { dirListing.selectNext(); - await signalToPromise(dirListing.updated); - expect(checkbox.checked).toBe(true); + const newNamePromise = dirListing.rename(); + const goUpSpy = jest.spyOn(dirListing as any, 'goUp'); + const editNode = dirListing['_editNode']; + simulate(editNode, 'keydown', { + key: 'Backspace', + keyCode: 8 + }); + // Can input node's value be changed with simulated key events? + editNode.value = 'new_name.txt'; + simulate(editNode, 'keydown', { + key: 'Enter', + keyCode: 13 + }); + const newName = await newNamePromise; + expect(newName).toBe('new_name.txt'); + expect(goUpSpy).not.toHaveBeenCalled(); }); - it('should be unchecked after item is unselected', async () => { - const itemNode = dirListing.contentNode.children[0] as HTMLElement; - const checkbox = dirListing.renderer.getCheckboxNode!( - itemNode - ) as HTMLInputElement; + it('should focus item after rename', async () => { dirListing.selectNext(); + const newNamePromise = dirListing.rename(); + const editNode = dirListing['_editNode']; + // Give it a name that should put it at the bottom + editNode.value = 'z.txt'; + simulate(editNode, 'keydown', { + key: 'Enter', + keyCode: 13 + }); + await newNamePromise; + const sortedItems = [...dirListing.sortedItems()]; + const lastIndex = sortedItems.length - 1; + expect(sortedItems[lastIndex].name).toBe('z.txt'); + const itemNode = dirListing['_items'][lastIndex]; await signalToPromise(dirListing.updated); - expect(checkbox.checked).toBe(true); - // Selecting the next item unselects the first. + expect(itemNode.contains(document.activeElement)).toBe(true); + }); + + it('should keep focus on item after user presses escape key', async () => { dirListing.selectNext(); - await signalToPromise(dirListing.updated); - expect(checkbox.checked).toBe(false); + const newNamePromise = dirListing.rename(); + const editNode = dirListing['_editNode']; + simulate(editNode, 'keydown', { + key: 'Escape', + keyCode: 27 + }); + await newNamePromise; + const itemNode = dirListing['_items'][0]; + expect(itemNode.contains(document.activeElement)).toBe(true); }); + }); - it('should allow selecting multiple items', async () => { - const itemNodes = Array.from( - dirListing.contentNode.children - ) as HTMLElement[]; - // JSDOM doesn't render anything, which means that all the elements have - // zero dimensions, so this is needed in order for the DirListing - // mousedown handler to believe that the mousedown event is relevant. - itemNodes[0].getBoundingClientRect = (): any => ({ - left: 0, - right: 10, - top: 0, - bottom: 10 + describe('#_handleMultiSelect', () => { + it('should do nothing when to-index is same as from-index', () => { + // to-index unselected + dirListing['_focusItem'](1); + expect(Object.keys(dirListing['selection'])).toHaveLength(0); + dirListing['_handleMultiSelect'](1); + expect(Object.keys(dirListing['selection'])).toHaveLength(0); + + // to-index selected + dirListing['_selectItem'](1, false, true); + const items = [...dirListing.sortedItems()]; + expect(dirListing['selection']).toHaveProperty([items[1].path], true); + expect(Object.keys(dirListing['selection'])).toHaveLength(1); + dirListing['_handleMultiSelect'](1); + expect(dirListing['selection']).toHaveProperty([items[1].path], true); + expect(Object.keys(dirListing['selection'])).toHaveLength(1); + }); + + describe('when to-index is selected', () => { + // - to-index is 0 + // - from-index is 2 + beforeEach(() => { + dirListing['_selectItem'](0, true); + // This is outside our index range, but let's select it so we can test + // that the function only affects the items in [from-index, to-index]. + dirListing['_selectItem'](3, true); }); - itemNodes[1].getBoundingClientRect = (): any => ({ - left: 0, - right: 10, - top: 10, - bottom: 20 + + describe('when from-index and all items in-between are selected', () => { + beforeEach(() => { + dirListing['_selectItem'](1, true); + dirListing['_selectItem'](2, true); + }); + + it('should leave to-index selected and unselect from-index and items in-between', () => { + // Directory listing is like this: + // 1. selected + // 2. selected + // 3. selected, focused + // 4. selected + expect(Object.keys(dirListing['selection'])).toHaveLength(4); + dirListing['_handleMultiSelect'](0); + // Now directory should look like: + // - selected, unselected, unselected, selected + const items = [...dirListing.sortedItems()]; + expect(Object.keys(dirListing['selection'])).toHaveLength(2); + expect(dirListing['selection']).toHaveProperty( + [items[0].path], + true + ); + expect(dirListing['selection']).toHaveProperty( + [items[3].path], + true + ); + }); }); - const checkboxes = itemNodes.map(node => - dirListing.renderer.getCheckboxNode!(node) - ) as HTMLInputElement[]; - const items = Array.from(dirListing.sortedItems()); - expect(dirListing.isSelected(items[0].name)).toBe(false); - expect(dirListing.isSelected(items[1].name)).toBe(false); - simulate(checkboxes[0], 'mousedown', { - clientX: 1, - clientY: 1 + + describe('when all are selected except from-index', () => { + beforeEach(() => { + dirListing['_selectItem'](1, true); + dirListing['_focusItem'](2); + }); + + it('should leave to-index selected and unselect from-index and items in-between', () => { + // Directory listing is like this: + // 1. selected + // 2. selected + // 3. unselected, focused + // 4. selected + expect(Object.keys(dirListing['selection'])).toHaveLength(3); + dirListing['_handleMultiSelect'](0); + // Now directory should look like: + // - selected, unselected, unselected, selected + const items = [...dirListing.sortedItems()]; + expect(Object.keys(dirListing['selection'])).toHaveLength(2); + expect(dirListing['selection']).toHaveProperty( + [items[0].path], + true + ); + expect(dirListing['selection']).toHaveProperty( + [items[3].path], + true + ); + }); }); - simulate(checkboxes[1], 'mousedown', { - clientX: 1, - clientY: 11 + + describe('when from-index and some items in-between are not selected', () => { + beforeEach(() => { + dirListing['_focusItem'](2); + }); + + it('should select all in-between from- and to-index, leaving from-index unselected', () => { + // Directory listing is like this: + // 1. selected + // 2. unselected + // 3. unselected, focused + // 4. selected + expect(Object.keys(dirListing['selection'])).toHaveLength(2); + dirListing['_handleMultiSelect'](0); + // Now directory should look like: + // - selected, selected, unselected, selected + const items = [...dirListing.sortedItems()]; + expect(items).toHaveLength(4); + expect(Object.keys(dirListing['selection'])).toHaveLength(3); + expect(dirListing['selection']).not.toHaveProperty([items[2].path]); + }); }); - await signalToPromise(dirListing.updated); - expect(dirListing.isSelected(items[0].name)).toBe(true); - expect(dirListing.isSelected(items[1].name)).toBe(true); - }); - it('should reflect multiple items selected', async () => { - const itemNodes = Array.from( - dirListing.contentNode.children - ) as HTMLElement[]; - const checkboxes = itemNodes.map(node => - dirListing.renderer.getCheckboxNode!(node) - ) as HTMLInputElement[]; - expect(checkboxes[0].checked).toBe(false); - expect(checkboxes[1].checked).toBe(false); - dirListing.selectNext(); - dirListing.selectNext(true); // true = keep existing selection - await signalToPromise(dirListing.updated); - expect(checkboxes[0].checked).toBe(true); - expect(checkboxes[1].checked).toBe(true); + describe('when from-index is selected but some items in-between are not', () => { + beforeEach(() => { + dirListing['_selectItem'](2, true); + }); + + it('should select all in-between from- and to-index', () => { + // Directory listing is like this: + // 1. selected + // 2. unselected + // 3. selected, focused + // 4. selected + expect(Object.keys(dirListing['selection'])).toHaveLength(3); + dirListing['_handleMultiSelect'](0); + // Now directory should look like: + // - selected, selected, selected, selected + const items = [...dirListing.sortedItems()]; + expect(items).toHaveLength(4); + expect(Object.keys(dirListing['selection'])).toHaveLength(4); + }); + }); }); - // A double click on the item should open the item; however, a double - // click on the checkbox should only check/uncheck the box. - it('should not open item on double click', () => { - const itemNode = dirListing.contentNode.children[0] as HTMLElement; - const checkbox = dirListing.renderer.getCheckboxNode!( - itemNode - ) as HTMLInputElement; - const wasOpened = jest.fn(); - dirListing.onItemOpened.connect(wasOpened); - simulate(checkbox, 'dblclick'); - expect(wasOpened).not.toHaveBeenCalled(); - dirListing.onItemOpened.disconnect(wasOpened); + describe('when to-index is unselected', () => { + // - to-index is 2 + // - from-index is 0 + + beforeEach(() => { + // This is outside our index range, but let's select it so we can test + // that the function only affects the items in [from-index, to-index]. + dirListing['_selectItem'](3, true); + }); + + describe('when from-index and in-between items are selected', () => { + beforeEach(() => { + dirListing['_selectItem'](1, true); + dirListing['_selectItem'](0, true); + }); + + it('should select all between from- and to-index', () => { + // Directory listing is like this: + // 1. selected, focused + // 2. selected + // 3. unselected [target] + // 4. selected + expect(Object.keys(dirListing['selection'])).toHaveLength(3); + dirListing['_handleMultiSelect'](2); + // Now directory should look like: + // - selected, selected, selected, selected + const items = [...dirListing.sortedItems()]; + expect(items).toHaveLength(4); + expect(Object.keys(dirListing['selection'])).toHaveLength(4); + }); + }); + + describe('when from-index is unselected but in-between items are selected', () => { + beforeEach(() => { + dirListing['_selectItem'](1, true); + dirListing['_focusItem'](0); + }); + + it('should select all between from- and to-index', () => { + // Directory listing is like this: + // 1. unselected, focused + // 2. selected + // 3. unselected [target] + // 4. selected + expect(Object.keys(dirListing['selection'])).toHaveLength(2); + dirListing['_handleMultiSelect'](2); + // Now directory should look like: + // - unselected, selected, selected, selected + const items = [...dirListing.sortedItems()]; + expect(items).toHaveLength(4); + expect(Object.keys(dirListing['selection'])).toHaveLength(3); + expect(dirListing['selection']).not.toHaveProperty([items[0].path]); + }); + }); }); + }); - it('should not become unchecked due to right-click on selected item', async () => { - const itemNode = dirListing.contentNode.children[0] as HTMLElement; - itemNode.getBoundingClientRect = (): any => ({ - left: 0, - right: 10, - top: 0, - bottom: 10 + describe('Enter key', () => { + it('should not open an item unless it is selected', () => { + // Meaning, do not open the item that is focussed if it is not also + // selected. + dirListing['_selectItem'](0, true); + dirListing['_selectItem'](1, true); + dirListing['_focusItem'](2); + const handleOpenSpy = jest.spyOn(dirListing as any, 'handleOpen'); + const itemNode = dirListing['_items'][2]; + const nameNode = dirListing['_renderer'].getNameNode(itemNode); + simulate(nameNode, 'keydown', { + key: 'Enter', + keyCode: 13 }); - const checkbox = dirListing.renderer.getCheckboxNode!( - itemNode - ) as HTMLInputElement; - const item = dirListing.sortedItems().next(); - await dirListing.selectItemByName(item.value.name); + expect(handleOpenSpy).toHaveBeenCalledTimes(2); + const sortedItems = [...dirListing.sortedItems()]; + expect(handleOpenSpy).toHaveBeenCalledWith(sortedItems[0]); + expect(handleOpenSpy).toHaveBeenCalledWith(sortedItems[1]); + expect(handleOpenSpy).not.toHaveBeenCalledWith(sortedItems[2]); + }); + }); + + describe('ArrowDown key', () => { + let dirListing: TestDirListing; + beforeEach(async () => { + const options = createOptionsForConstructor(); + + // Start with some files instead of empty before creating the DirListing. + // This makes it easier to test checking/unchecking because after the + // DirListing is created, whenever a file is added, the DirListing selects + // that file, which causes the file's checkbox to be checked. + await options.model.manager.newUntitled({ type: 'file' }); + await options.model.manager.newUntitled({ type: 'file' }); + await options.model.manager.newUntitled({ type: 'file' }); + + // Create the widget and mount it to the DOM. + dirListing = new TestDirListing(options); + Widget.attach(dirListing, document.body); + + // Wait for the widget to update its internal DOM state before running + // tests. await signalToPromise(dirListing.updated); - expect(checkbox.checked).toBe(true); - expect(dirListing.isSelected(item.value.name)).toBe(true); - simulate(checkbox, 'mousedown', { - clientX: 1, - clientY: 1, - button: 2 + }); + + it('should select first item when nothing is selected', async () => { + simulate(dirListing.node, 'keydown', { + key: 'ArrowDown', + keyCode: 40 }); await signalToPromise(dirListing.updated); - // Item is still selected and checkbox is still checked after - // right-click. - expect(dirListing.isSelected(item.value.name)).toBe(true); - expect(checkbox.checked).toBe(true); + const sortedItems = [...dirListing.sortedItems()]; + const selectedItems = [...dirListing.selectedItems()]; + expect(selectedItems).toHaveLength(1); + expect(selectedItems[0]).toBe(sortedItems[0]); }); - // This essentially tests that preventDefault has been called on the click - // handler (which also handles keyboard and touch "clicks" in addition to - // mouse clicks). In other words, only the DirListing should check/uncheck - // the checkbox, not the browser's built-in default handler for the click. - it('should not get checked by the default action of a click', () => { - const itemNode = dirListing.contentNode.children[0] as HTMLElement; - const checkbox = dirListing.renderer.getCheckboxNode!( - itemNode - ) as HTMLInputElement; - expect(checkbox.checked).toBe(false); - simulate(checkbox, 'click', { bubbles: false }); - expect(checkbox.checked).toBe(false); + it('should select second item once first item is selected', async () => { + dirListing['_selectItem'](0, false); + simulate(dirListing.node, 'keydown', { + key: 'ArrowDown', + keyCode: 40 + }); + await signalToPromise(dirListing.updated); + const sortedItems = [...dirListing.sortedItems()]; + const selectedItems = [...dirListing.selectedItems()]; + expect(selectedItems).toHaveLength(1); + expect(selectedItems[0]).toBe(sortedItems[1]); }); - }); - describe('check-all checkbox', () => { - it('should be unchecked when the current directory is empty', async () => { - const { path } = await dirListing.model.manager.newUntitled({ - type: 'directory' + describe('when pressing shift key and next item is selected', () => { + it('should unselect if current item is selected and previous is unselected', async () => { + dirListing['_selectItem'](2, true); + dirListing['_selectItem'](1, true); + // This should be the state: + // - unselected + // - selected, focussed + // - selected + await signalToPromise(dirListing.updated); + simulate(dirListing.node, 'keydown', { + key: 'ArrowDown', + keyCode: 40, + shiftKey: true + }); + await signalToPromise(dirListing.updated); + // Now it should be: + // - unselected + // - unselected + // - selected, focussed + const sortedItems = [...dirListing.sortedItems()]; + const selectedItems = [...dirListing.selectedItems()]; + expect(selectedItems).toHaveLength(1); + expect(selectedItems[0]).toBe(sortedItems[2]); + }); + + it('should leave selected otherwise', async () => { + dirListing['_selectItem'](0, true); + dirListing['_selectItem'](2, true); + dirListing['_selectItem'](1, true); + // This should be the state: + // - selected + // - selected, focussed + // - selected + await signalToPromise(dirListing.updated); + simulate(dirListing.node, 'keydown', { + key: 'ArrowDown', + keyCode: 40, + shiftKey: true + }); + await signalToPromise(dirListing.updated); + // Now it should be: + // - selected + // - selected + // - selected, focussed + const sortedItems = [...dirListing.sortedItems()]; + const selectedItems = [...dirListing.selectedItems()]; + expect(selectedItems).toHaveLength(3); + expect(sortedItems).toHaveLength(3); }); - await dirListing.model.cd(path); - await signalToPromise(dirListing.updated); - const headerCheckbox = dirListing.renderer.getCheckboxNode!( - dirListing.headerNode - ) as HTMLInputElement; - expect(headerCheckbox.checked).toBe(false); - expect(headerCheckbox!.indeterminate).toBe(false); }); + }); - describe('when previously unchecked', () => { - it('check initial conditions', () => { - const headerCheckbox = dirListing.renderer.getCheckboxNode!( - dirListing.headerNode + describe('checkboxes', () => { + describe('file/item checkbox', () => { + it('should be checked after item is selected', async () => { + const itemNode = dirListing.contentNode.children[0] as HTMLElement; + const checkbox = dirListing.renderer.getCheckboxNode!( + itemNode ) as HTMLInputElement; - expect(headerCheckbox.checked).toBe(false); - expect(headerCheckbox!.indeterminate).toBe(false); - expect(Array.from(dirListing.selectedItems())).toHaveLength(0); + expect(checkbox.checked).toBe(false); + dirListing.selectNext(); + await signalToPromise(dirListing.updated); + expect(checkbox.checked).toBe(true); }); - it('should check all', async () => { - const headerCheckbox = dirListing.renderer.getCheckboxNode!( - dirListing.headerNode + + it('should be unchecked after item is unselected', async () => { + const itemNode = dirListing.contentNode.children[0] as HTMLElement; + const checkbox = dirListing.renderer.getCheckboxNode!( + itemNode ) as HTMLInputElement; - simulate(headerCheckbox, 'click'); + dirListing.selectNext(); await signalToPromise(dirListing.updated); - expect(Array.from(dirListing.selectedItems())).toHaveLength(2); + expect(checkbox.checked).toBe(true); + // Selecting the next item unselects the first. + dirListing.selectNext(); + await signalToPromise(dirListing.updated); + expect(checkbox.checked).toBe(false); }); - }); - describe('when previously indeterminate', () => { - beforeEach(async () => { + it('should allow selecting multiple items', async () => { + const itemNodes = Array.from( + dirListing.contentNode.children + ) as HTMLElement[]; + // JSDOM doesn't render anything, which means that all the elements have + // zero dimensions, so this is needed in order for the DirListing + // mousedown handler to believe that the mousedown event is relevant. + itemNodes[0].getBoundingClientRect = (): any => ({ + left: 0, + right: 10, + top: 0, + bottom: 10 + }); + itemNodes[1].getBoundingClientRect = (): any => ({ + left: 0, + right: 10, + top: 10, + bottom: 20 + }); + const checkboxes = itemNodes.map(node => + dirListing.renderer.getCheckboxNode!(node) + ) as HTMLInputElement[]; + const items = Array.from(dirListing.sortedItems()); + expect(dirListing.isSelected(items[0].name)).toBe(false); + expect(dirListing.isSelected(items[1].name)).toBe(false); + simulate(checkboxes[0], 'mousedown', { + clientX: 1, + clientY: 1 + }); + simulate(checkboxes[1], 'mousedown', { + clientX: 1, + clientY: 11 + }); + await signalToPromise(dirListing.updated); + expect(dirListing.isSelected(items[0].name)).toBe(true); + expect(dirListing.isSelected(items[1].name)).toBe(true); + }); + + it('should reflect multiple items selected', async () => { + const itemNodes = Array.from( + dirListing.contentNode.children + ) as HTMLElement[]; + const checkboxes = itemNodes.map(node => + dirListing.renderer.getCheckboxNode!(node) + ) as HTMLInputElement[]; + expect(checkboxes[0].checked).toBe(false); + expect(checkboxes[1].checked).toBe(false); dirListing.selectNext(); + dirListing.selectNext(true); // true = keep existing selection await signalToPromise(dirListing.updated); + expect(checkboxes[0].checked).toBe(true); + expect(checkboxes[1].checked).toBe(true); }); - it('check initial conditions', () => { - const headerCheckbox = dirListing.renderer.getCheckboxNode!( - dirListing.headerNode + + // A double click on the item should open the item; however, a double + // click on the checkbox should only check/uncheck the box. + it('should not open item on double click', () => { + const itemNode = dirListing.contentNode.children[0] as HTMLElement; + const checkbox = dirListing.renderer.getCheckboxNode!( + itemNode ) as HTMLInputElement; - expect(headerCheckbox.indeterminate).toBe(true); - expect(Array.from(dirListing.selectedItems())).toHaveLength(1); + const wasOpened = jest.fn(); + dirListing.onItemOpened.connect(wasOpened); + simulate(checkbox, 'dblclick'); + expect(wasOpened).not.toHaveBeenCalled(); + dirListing.onItemOpened.disconnect(wasOpened); }); - it('should uncheck all', async () => { - const headerCheckbox = dirListing.renderer.getCheckboxNode!( - dirListing.headerNode + + it('should not become unchecked due to right-click on selected item', async () => { + const itemNode = dirListing.contentNode.children[0] as HTMLElement; + itemNode.getBoundingClientRect = (): any => ({ + left: 0, + right: 10, + top: 0, + bottom: 10 + }); + const checkbox = dirListing.renderer.getCheckboxNode!( + itemNode ) as HTMLInputElement; - simulate(headerCheckbox, 'click'); + const item = dirListing.sortedItems().next(); + await dirListing.selectItemByName(item.value.name); await signalToPromise(dirListing.updated); - expect(Array.from(dirListing.selectedItems())).toHaveLength(0); + expect(checkbox.checked).toBe(true); + expect(dirListing.isSelected(item.value.name)).toBe(true); + simulate(checkbox, 'mousedown', { + clientX: 1, + clientY: 1, + button: 2 + }); + await signalToPromise(dirListing.updated); + // Item is still selected and checkbox is still checked after + // right-click. + expect(dirListing.isSelected(item.value.name)).toBe(true); + expect(checkbox.checked).toBe(true); + }); + + // This essentially tests that preventDefault has been called on the click + // handler (which also handles keyboard and touch "clicks" in addition to + // mouse clicks). In other words, only the DirListing should check/uncheck + // the checkbox, not the browser's built-in default handler for the click. + it('should not get checked by the default action of a click', () => { + const itemNode = dirListing.contentNode.children[0] as HTMLElement; + const checkbox = dirListing.renderer.getCheckboxNode!( + itemNode + ) as HTMLInputElement; + expect(checkbox.checked).toBe(false); + simulate(checkbox, 'click', { bubbles: false }); + expect(checkbox.checked).toBe(false); }); }); - describe('when previously checked', () => { - beforeEach(async () => { - dirListing.selectNext(true); - dirListing.selectNext(true); + describe('check-all checkbox', () => { + it('should be unchecked when the current directory is empty', async () => { + const { path } = await dirListing.model.manager.newUntitled({ + type: 'directory' + }); + await dirListing.model.cd(path); await signalToPromise(dirListing.updated); - }); - it('check initial conditions', () => { const headerCheckbox = dirListing.renderer.getCheckboxNode!( dirListing.headerNode ) as HTMLInputElement; - expect(headerCheckbox.checked).toBe(true); - expect(headerCheckbox.indeterminate).toBe(false); - expect(Array.from(dirListing.selectedItems())).toHaveLength(2); + expect(headerCheckbox.checked).toBe(false); + expect(headerCheckbox!.indeterminate).toBe(false); }); - it('should uncheck all', async () => { - const headerCheckbox = dirListing.renderer.getCheckboxNode!( - dirListing.headerNode - ) as HTMLInputElement; - simulate(headerCheckbox, 'click'); - await signalToPromise(dirListing.updated); - expect(Array.from(dirListing.selectedItems())).toHaveLength(0); + + describe('when previously unchecked', () => { + const expectInitialConditions = () => { + const headerCheckbox = dirListing.renderer.getCheckboxNode!( + dirListing.headerNode + ) as HTMLInputElement; + expect(headerCheckbox.checked).toBe(false); + expect(headerCheckbox!.indeterminate).toBe(false); + expect(Array.from(dirListing.selectedItems())).toHaveLength(0); + }; + it('should check all', async () => { + expectInitialConditions(); + const headerCheckbox = dirListing.renderer.getCheckboxNode!( + dirListing.headerNode + ) as HTMLInputElement; + simulate(headerCheckbox, 'click'); + await signalToPromise(dirListing.updated); + expect(Array.from(dirListing.selectedItems())).toHaveLength(4); + }); + }); + + describe('when previously indeterminate', () => { + beforeEach(async () => { + dirListing.selectNext(); + await signalToPromise(dirListing.updated); + }); + const expectInitialConditions = () => { + const headerCheckbox = dirListing.renderer.getCheckboxNode!( + dirListing.headerNode + ) as HTMLInputElement; + expect(headerCheckbox.indeterminate).toBe(true); + expect(Array.from(dirListing.selectedItems())).toHaveLength(1); + }; + it('should uncheck all', async () => { + expectInitialConditions(); + const headerCheckbox = dirListing.renderer.getCheckboxNode!( + dirListing.headerNode + ) as HTMLInputElement; + simulate(headerCheckbox, 'click'); + await signalToPromise(dirListing.updated); + expect(Array.from(dirListing.selectedItems())).toHaveLength(0); + }); + }); + + describe('when previously checked', () => { + beforeEach(async () => { + // Select/check all items + dirListing.selectNext(true); + dirListing.selectNext(true); + dirListing.selectNext(true); + dirListing.selectNext(true); + await signalToPromise(dirListing.updated); + }); + const expectInitialConditions = () => { + const headerCheckbox = dirListing.renderer.getCheckboxNode!( + dirListing.headerNode + ) as HTMLInputElement; + expect(headerCheckbox.checked).toBe(true); + expect(headerCheckbox.indeterminate).toBe(false); + expect(Array.from(dirListing.selectedItems())).toHaveLength(4); + }; + it('should uncheck all', async () => { + expectInitialConditions(); + const headerCheckbox = dirListing.renderer.getCheckboxNode!( + dirListing.headerNode + ) as HTMLInputElement; + simulate(headerCheckbox, 'click'); + await signalToPromise(dirListing.updated); + expect(Array.from(dirListing.selectedItems())).toHaveLength(0); + }); }); }); });