-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make file browser respond to focussed elements #13577
Make file browser respond to focussed elements #13577
Conversation
Thanks for making a pull request to jupyterlab! |
149cd9f
to
6e7d068
Compare
6e7d068
to
1879719
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done self-reviewing!
@@ -346,6 +346,11 @@ const downloadPlugin: JupyterFrontEndPlugin<void> = { | |||
Clipboard.copyToSystem(url); | |||
}); | |||
}, | |||
isVisible: () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "copy download link" command does not accept multiple files, so do not show it in the (right-click) context menu when multiple files are selected.
@@ -1111,6 +1110,11 @@ function addCommands( | |||
return widget.rename(); | |||
} | |||
}, | |||
isVisible: () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no batch, multi-file "rename" command, so do not show it in the (right-click) context menu when multiple files are selected.
@@ -1130,8 +1134,10 @@ function addCommands( | |||
Clipboard.copyToSystem(item.value.path); | |||
}, | |||
isVisible: () => | |||
// So long as this command only handles one file at time, don't show it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "copy path" command does not support multiple files, so do not show it in the (right-click) context menu when multiple files are selected.
Note: in Windows, when multiple files are selected, the copy-path command puts each path on a newline in the clipboard.
packages/filebrowser/src/browser.ts
Outdated
* Create a new directory | ||
*/ | ||
createNewDirectory(): Promise<Contents.IModel> { | ||
return this._createNew(this._futureNewDirectory, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This re-write allows consuming code to know when the operation is done.
Note that my implementation of "done" means the new directory or file has both been created and renamed by the user.
Rewriting it this way both allowed me to better test these methods as well as remove the TODO comment inside the methods.
* | ||
* Go up one level in the directory tree. | ||
*/ | ||
async goUp() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.listing
is a protected member so I can't do browserForPath.listing.goUp()
in the execute
function of the "filebrower:go-up" command, I need to expose the method this way and call browserForPath.goUp()
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now handled in onUpdateRequest
. All item nodes except for the currently focussed one are given tabIndex = -1.
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why these up/down arrow handlers on edit were ever needed. This is the default behavior of an input field in Chrome.
I have a suspicion that this code was needed at some point, but then something was refactored that made this code no longer necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also the default behavior on Firefox (using Debian OS). Could you test it on Safary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Tested on Safari. Works the same.
@@ -113,11 +113,6 @@ | |||
outline: 0; | |||
} | |||
|
|||
.jp-DirListing:focus-visible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR removes the tabIndex = 0
on this element, so these styles are no longer needed.
@@ -273,6 +268,12 @@ | |||
user-select: none; | |||
} | |||
|
|||
/* https://css-tricks.com/copy-the-browsers-native-focus-styles/ */ | |||
.jp-DirListing-itemText:focus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some people might not like it, but I want the focus ring to be always visible, whether the user selects a file via mouse or via keyboard.
Part of the reason why is that I think it can help the user make sense of the multi-select, which uses the currently focussed item as one of the endpoints of the multi-select.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
labelWrapper.classList.add('jp-mod-visible'); | ||
} else { | ||
// Disable tabbing to all other checkboxes. | ||
checkbox.tabIndex = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unnecessary to allow the user to tab to the checkbox next to a file because the state of the checkbox is controlled by the state of the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank @gabalafou
Here is a first quick review - I did not test it yet.
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also the default behavior on Firefox (using Debian OS). Could you test it on Safary?
@@ -273,6 +268,12 @@ | |||
user-select: none; | |||
} | |||
|
|||
/* https://css-tricks.com/copy-the-browsers-native-focus-styles/ */ | |||
.jp-DirListing-itemText:focus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
packages/filebrowser/style/base.css
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather choose a CSS theme property than Highlight
(from the system-color) because the background of the highlight will not use the system-color and therefore can lake contrast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will do.
packages/filebrowser/style/base.css
Outdated
/* 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the same about that specific webkit property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
// Copyright (c) Jupyter Development Team. Distributed under the terms of the | ||
// Modified BSD License. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not touch the license header
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch. That was my editor. Should be fixed now.
packages/filebrowser/src/browser.ts
Outdated
createNewDirectory(): void { | ||
if (this._directoryPending === true) { | ||
return; | ||
private _createNew( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weird pattern. I would suggest you use a pattern like that:
private async _createNew(options): Promise<Contents.IModel> {
// ...
}
async createNewDirectory(): Promise<Contents.IModel> {
if(this._directoryPending) {
return this._directoryPending;
}
this._directoryPending = this._createNew(...);
const model = await this._directoryPending;
// Reset directory pending
this._directoryPending = null;
return model;
}
private _directoryPending: Promise<Contents.IModel> | null = null;
A general comment, I would advice to use async/await/try..catch pattern rather than then/catch to ease code reading and modernize it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe. Agreed. I thought there was some reason for doing it the way I did but now I can't remember why, so I'll implement it the way you suggest.
"command": "filebrowser:toggle-main", | ||
"keys": ["Accel Shift F"], | ||
"selector": "body" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the Backspace
shortcut a WCAG recommendation? Is it something users may want to customize?
That shortcuts is defined in packages/filebrowser-extension/schema/widget.json
. So it should be needed here.
"command": "filebrowser:toggle-main", | |
"keys": ["Accel Shift F"], | |
"selector": "body" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a merge conflict here. I must not have resolved it correctly. The only diff should be removing the Backspace shortcut from this JSON file. I'll fix this.
packages/filebrowser/src/listing.ts
Outdated
const promise = renameFile(manager, oldPath, newPath); | ||
return promise.then( | ||
() => newName, | ||
error => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your reasoning makes sense.
Would you mind switching to async/await syntax to ease code readability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review comments @fcollonval! I believe I addressed all of them. If I missed anything, it wasn't intentional, please bring it to my attention again.
I changed the CSS rule that controls the focus ring of the directory items to use theme variables and checked it in light-mode across a variety of browsers (Chrome, Safari, and Firefox on MacOS). I also checked dark-mode on MacOS Chrome. I probably need @isabela-pf or someone more familiar than me with the graphic design conventions of JupyterLab to give that a look and okay it before we merge this in.
"command": "filebrowser:toggle-main", | ||
"keys": ["Accel Shift F"], | ||
"selector": "body" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a merge conflict here. I must not have resolved it correctly. The only diff should be removing the Backspace shortcut from this JSON file. I'll fix this.
packages/filebrowser/src/listing.ts
Outdated
const promise = renameFile(manager, oldPath, newPath); | ||
return promise.then( | ||
() => newName, | ||
error => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Happy to rewrite using async/await. The only reason I didn't do it to begin with is that this file does not have a lot of unit tests so I've tried to change it as little as possible—just enough to achieve what I need, for fear of accidentally breaking something.
But I will gladly convert this and some of the other code that this PR touches to use async-await syntax.
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Tested on Safari. Works the same.
packages/filebrowser/src/browser.ts
Outdated
createNewDirectory(): void { | ||
if (this._directoryPending === true) { | ||
return; | ||
private _createNew( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe. Agreed. I thought there was some reason for doing it the way I did but now I can't remember why, so I'll implement it the way you suggest.
packages/filebrowser/style/base.css
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will do.
packages/filebrowser/style/base.css
Outdated
/* 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
// Copyright (c) Jupyter Development Team. Distributed under the terms of the | ||
// Modified BSD License. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch. That was my editor. Should be fixed now.
Thanks @fcollonval! I pushed two commits that should hopefully fix the test timeout and linter error. My biggest concern before merging this PR is me moving |
I don't think the UI and Visual Regression Tests are related but I'm not quite sure how to check... |
I don't have strong feeling about this. It seems that Regarding UI tests, they are currently broken on master. |
@fcollonval I opened an issue to keep track of the bug you mentioned in your review and I will try to submit a fix in the near future: |
I do not have strong feelings on hard-coding
I believe we can just adjust the selector for the keybinding and it would just work. Is there any other reason for hard-coding it? |
No other reason. I just don't know how to do it. 😑 |
But I can give it another think and get back to you. |
I'm not finding any problems! I think this PR is well-scoped and I think everything happening with focus in the file browser makes sense to me. Just to note some positives:
I am curious about the ways that the focus styling are different in the file browser versus other areas of the UI, but I do think that is outside the scope of the PR. Just noting it for follow up on my part. A final note from me: |
Thanks @isabela-pf! I want to follow up on your note about I would say that the keyboard-only, discontiguous multi-select feature is more aimed at Windows and Ubuntu users than Mac users, but all the same, I would like things to mostly work the same across operating systems, including older versions of Mac OS. |
Also, to your note about focus styling, I would add that in my experience, JupyterLab's focus styling is not terribly consistent, and that this is an area that could use some polishing, but yes, I agree that it's a bigger scope than this PR. I wonder if a good first step towards consistency would be to add some variables for outline styles that could be reused across the app. I have the variables.css file in mind. (That one links to the light theme version, but there is also a dark theme version of the file.) I imagine adding a section like: /* Outline (focus ring) */
--jp-outline-color: white;
--jp-outline-style: solid;
--jp-outline-width: 1px; And for the dark theme version: /* Outline (focus ring) */
--jp-outline-color: #111;
--jp-outline-style: solid;
--jp-outline-width: 1px; I'm not sure about those values, but... something like that to start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krassowski, I'm happy to report that I was able to keep the backspace as a user-configurable shortcut.
I think this PR is ready for merge now. 😄
{ | ||
"command": "filebrowser:go-up", | ||
"keys": ["Backspace"], | ||
"selector": ".jp-DirListing:focus" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this seems to do the trick: just add another selector to handle the case when the directory is empty and leave the other selector alone. I wish I could leave a comment in this JSON file.
This :focus
selector works for when the directory is empty because the only node (within the directory listing widget) that can receive focus at that point is the directory listing node itself. We cannot use the selector without :focus
though because then it would get triggered if the user were in the middle of renaming a file, makes a typo, and then presses backspace to delete the typo.
I tested this change three ways:
- Opening an empty folder then pressing backspace
- Opening a non-empty folder then pressing backspace
- Renaming a file and pressing backspace while editing the file name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks the patience @gabalafou
Waiting for a second run of js-filebrowser to be sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gabalafou
References
PR #13415 made focus visible, which as @krassowski pointed out:
In order to understand this PR, it's important to understand that "selection" and "focus" are not the same thing. The easiest way to understand the difference between the two is to play with the file browser in Windows or Ubuntu. If you have Windows or Ubuntu, and you open the file browser, you can move focus independent of selection by pressing
ctrl
+ arrow key. Focus is visually indicated with a focus ring, while selection is indicated with a highlight, or background color. This PR basically adopts the focus-versus-selection patterns in Windows and Ubuntu for the JupyterLab file browser.Code changes
Most of the code changes in this PR are to the
DirListing
class in the@jupyterlab/filebrowser
package. The main change is the addition of a new private state variable_focusIndex
, which keeps track of where the focus should be in the directory listing.Previously, every file or directory in the file browser had a tab index of 0. I changed it so that there should only ever be one file item node at a time that can be focussed with the tab key. This makes navigating the JupyterLab UI easier because the user won't have to tab through all of the items in the file browser to get to another part of the UI.
I use the focus index in a few places to simplify
DirListing
methods. For example, the multi select method used to iterate through the directory listing to find the selected item nearest to the one clicked, but now it simply looks up the item at the focus index.There are a number of other code changes to fix issues I uncovered with the file browser keyboard shortcuts. For example, I moved the
Backspace
keyboard shortcut out of the JSON schema file and into theDirListing
class because it wasn't working when theDirListing
was empty, and I couldn't think of a better way to fix it.Test coverage
I added tests for about half of the code changes I made. Before adding more tests, I wanted to get this PR up for review, since changes requested in review could change the test code.
User-facing changes
CTRL
+Space
now selects/unselects files.Backspace
now works to go up even when folder is empty.CTRL
+ up (or down) arrow key moves the focus. ThenCTRL
+Space
toggles selection.esc
key).Enter
keyboard shortcut can now open multiple files instead of just one.Backwards-incompatible changes
None, as far as I'm aware.