Add basic keyboard shortcuts #567

tomasmatus · 2024-06-18T12:44:46Z

fixes: #451

For now this adds following keybindings:

alt + arrow up: go up one directory
alt + arrow down: activate selected item, enter directory
Enter: activate selected item, enter directory
ctrl + shift + L: focus files breadcrumbs to manually edit it
F2: rename selected file
shift + N: create new directory
ctrl + c: copy file/directory
ctrl + v: paste file/directory
ctrl + a: select all files

Alt + arrow left/right is already supported by cockpit files browser
history.

Add pixeltest for help menu
Merge Move global menu to the toolbar #739

Files: basic keyboard shortcuts

tomasmatus · 2024-06-18T12:49:05Z

test/check-application

+        b.wait_text(".breadcrumb-button:nth-of-type(5)", "anotherdir")
+
+        # Go back in history twice
+        # b.key("ArrowLeft", repeat=2, modifiers=1)


@jelly do you have any idea what might be happening here? the b.key(...) doesn't seem to work when running this test in chromium but everything works fine in firefox

jelly

The biggest thing which is missing (and should be added after discussion) is a way to view the keybindings. We should have a dropdownitem in the main kebab + modal for that.

Moving up / out of a directory key bindings seems a little bit obscure to me.
F2 for rename, this feels so random? Which applications have this as default?

tomasmatus · 2024-06-18T12:52:52Z

a way to view the keybindings

yes, I will add this when we are more settled with shortcuts

F2 for rename, this feels so random? Which applications have this as default?

both dolphin and nautilus

tomasmatus · 2024-06-18T12:53:25Z

src/files-card-body.jsx

+            case "N":
+                if (!e.ctrlKey && !e.altKey) {
+                    Dialogs.show(
+                        <CreateDirectoryModal currentPath={currentPath} />


ooops, looks like this slipped my mind!

tomasmatus · 2024-06-19T09:29:43Z

Turns out ctrl + a to select all files breaks using the same shortcut in the filtering text input

garrett · 2024-06-20T13:30:02Z

The biggest thing which is missing (and should be added after discussion) is a way to view the keybindings. We should have a dropdownitem in the main kebab + modal for that.

We have mockups for that in the menu and an issue for that already @ #451

What is probably good for a discussion is if we want to override default browser behavior for ctrl + l which focuses the Address Bar to "focus files breadcrumbs to manually edit it" or use the keybind that I set (L aka shift + l).

We have an issue for this already, where I suggested control-shift-L, which aligns with how browsers and file managers use control-L, but adds in the shift as to not stomp on browser behavior: #450

garrett · 2024-06-20T14:13:53Z

BTW: Mockup and explanation for keyboard shortcuts reference modal is now @ #451 (comment)

tomasmatus · 2024-06-25T11:18:52Z

Added a first round of help dialog, I'll play with the css to make keys look fancy later.

src/dialogs/keyboardShortcutsHelp.tsx

garrett · 2024-06-25T16:11:17Z

I've pushed up my changes to restyle the keyboard shortcuts.

Now, it:

looks more like the mockup visually
flows when the content is too narrow
uses headings
reports the accessibility tree properly (previously, the list wasn't being expressed properly and the headings were just text, all due to the list element having content that was not permitted)
is reformatted in the JSX so it's easier to understand what's going on, using multiple lines and indentation

garrett · 2024-06-25T16:20:48Z

F2 for rename, this feels so random? Which applications have this as default?

both dolphin and nautilus

It's also the way you rename in Explorer, in Windows.

Meanwhile, for renaming, macOS uses... enter. (Yes, I'm not kidding.) Everywhere else, including browsers on a focused area or widget, enter activates. But in Finder, it's to rename.

F2 makes more sense than enter, and it's a keyboard shortcut most people are already familiar with if they use keyboard shortcuts in file managers.

jelly

There is one big issue, when I browse to a directory where I get a permission denied or a non-existant directory then the keybindings no longer work as the eventhandlers are only added on "valid folders".

src/files-card-body.tsx

test/check-application

src/files-breadcrumbs.tsx

tomasmatus · 2024-08-06T11:34:01Z

src/header.tsx

    const onSelect = (_ev?: React.MouseEvent, itemId?: string | number) => {
        if (itemId === "hidden-toggle") {
            setShowHidden(prevShowHidden => {
                localStorage.setItem("files:showHiddenFiles", !showHidden ? "true" : "false");
                return !prevShowHidden;
            });
+        } else if (itemId === "shortcuts-help") {


dw it is tested :)

jelly · 2024-09-03T12:46:21Z

I don't believe that the location of the keyboard shortcuts help is logical here:

test/check-application

jelly · 2024-09-03T12:55:23Z

src/header.tsx

    const onSelect = (_ev?: React.MouseEvent, itemId?: string | number) => {
        if (itemId === "hidden-toggle") {
            setShowHidden(prevShowHidden => {
                localStorage.setItem("files:showHiddenFiles", !showHidden ? "true" : "false");
                return !prevShowHidden;
            });
+        } else if (itemId === "shortcuts-help") {


src/app.tsx

garrett · 2024-09-03T15:48:03Z

Agreed; the keyboard shortcuts modal is supposed to go in the main menu, not the view menu.

tomasmatus · 2024-09-04T14:03:51Z

Agreed; the keyboard shortcuts modal is supposed to go in the main menu, not the view menu.

Wasn't that removed? Where should it go then?

tomasmatus · 2024-09-04T14:20:36Z

CI red, fixing in #727

jelly · 2024-09-05T07:57:21Z

Agreed; the keyboard shortcuts modal is supposed to go in the main menu, not the view menu.

Wasn't that removed? Where should it go then?

@garrett what was the idea/ design here?

garrett · 2024-09-09T09:18:08Z

It should go into the menu, which is supposed to be in the place where the menu should be — and that was never in the sidebar. The sidebar never had a menu in any of the mockups at any point in time. The fact that the sidebar has a menu in main is a huge UX bug. (I've pointed this out several times during development, but we haven't moved it as I had an in progress mockup for some time now and haven't had the opportunity to address it properly yet.)

The menu was always supposed to be either global and/or in the toolbar. The current mockups — which I haven't had time to finish yet — have the menu in the toolbar.

The current state of the sidebarless redesign with merged menus looks like this:

But it's unfinished, so it's subject to change. And it will change. But the menu position is probably the most solid of this mockup at this point, and the menu items are probably pretty close too.

I basically made a compromise after looking at other file management apps and seeing they have a menu like this too, on Windows, macOS, and KDE. GNOME's is different, as they don't act upon the selection (and rely on a context menu only), but everything else does. Other designs for it have it come after the selected items, which is more proper in the idea of a flow, but non-GNOME file managers, word processors, and all kinds of other apps actually do sometimes change their menus and toolbars (including toolbar icons) based on selection of what comes after it in the UI, so it's OK enough for us to do this too. And it's probably what everyone other than people using GNOME would expect.

I think we'd want to also adjust the context menu to be similar too.

For now, I think the correct solution is to have the opposite of a "follow-up" and make a separate PR that moves the menu from the sidebar to the toolbar (to the right of the upload button) as a first step.

Summary: Move the menu from the sidebar to the toolbar first, then make sure the keyboard shortcuts are in the menu at the bottom.

relevant comment: cockpit-project#567 (comment)

test/check-application

relevant comment: cockpit-project#567 (comment)

relevant comment: #567 (comment)

jelly

Overall looks good, some minor fixes / suggestions and then I think it's good to land

src/app.tsx

src/files-card-body.tsx

src/header.tsx

test/check-application

src/menu.tsx

fixes: cockpit-project#451 For now this adds following keybindings: - `alt + arrow up`: go up one directory - `alt + arrow down`: activate selected item, enter directory - `Enter`: activate selected item, enter directory - `ctrl + shift + L`: focus files breadcrumbs to manually edit it - `F2`: rename selected file - `shift + N`: create new directory - `ctrl + c`: copy file/directory - `ctrl + v`: paste file/directory - `ctrl + a`: select all files Alt + arrow left/right is already supported by cockpit files browser history.

cockpituous · 2024-09-25T14:12:00Z

src/app.tsx

+        return () => {
+            document.removeEventListener("keydown", onKeyboardNav);


These 2 added lines are not executed by any test.

cockpituous · 2024-09-25T14:12:00Z

src/dialogs/keyboardShortcutsHelp.tsx

+          title={_("Keyboard shortcuts")}
+          variant={ModalVariant.large}
+          className="shortcuts-dialog"
+          onClose={() => dialogResult.resolve()}


This added line is not executed by any test.

cockpituous · 2024-09-25T14:12:00Z

src/files-card-body.tsx

+                        const selectedIdx = sortedFiles?.findIndex(file => file.name === firstSelectedName);
+                        const newIdx = selectedIdx < sortedFiles.length - 1
+                            ? selectedIdx + 1
+                            : 0;


This added line is not executed by any test.

cockpituous · 2024-09-25T14:12:00Z

src/files-card-body.tsx

+                        const selectedIdx = sortedFiles?.findIndex(file => file.name === firstSelectedName);
+                        const newIdx = selectedIdx > 0
+                            ? selectedIdx - 1
+                            : sortedFiles.length - 1;


This added line is not executed by any test.

cockpituous · 2024-09-25T14:12:01Z

src/menu.tsx

+        "-R",
+        ...clipboard,
+        path
+    ]).catch(err => addAlert(err.message, AlertVariant.danger, `${new Date().getTime()}`));


This added line is not executed by any test.

jelly

Some small comments and one follow up but let's not let this block this for much longer.

jelly · 2024-10-01T08:37:32Z

src/app.tsx

+        return () => {
+            document.removeEventListener("keydown", onKeyboardNav);
+        };
+    }, []);


The dependency array can go

jelly · 2024-10-01T08:49:43Z

test/check-application

@@ -1174,6 +1177,20 @@ class TestFiles(testlib.MachineCase):
        b.wait_not_present(".pf-v5-c-modal-box")
        b.wait_visible("[data-item='renamed-foo.txt']")

+        # Rename file using keyboard shortcut
+        b.click("[data-item='newfile']")
+        b.key("\uE032")  # F2


Hmmm any reason "F2" doesn't work?

Could work if I add it here in testlib: https://github.com/cockpit-project/cockpit/blob/dc2085e95137ced40e03c94b8b2ebcf090a8acea/test/common/testlib.py#L103

Seems worth it, let's do it :) But don't block merging on it.

jelly · 2024-10-01T08:50:46Z

test/check-application

@@ -814,6 +814,9 @@ class TestFiles(testlib.MachineCase):
        b.mouse("[data-item='delete2']", "click", ctrlKey=True)
        b.wait_visible("[data-item='delete1'].row-selected")
        b.wait_visible("[data-item='delete2'].row-selected")
+        b.focus("#files-card-parent")
+        # For strange reasons ctrlKey remains pressed after the b.mouse() above (spotted in firefox)


So we keep seeing this in the keyboard event, sounds like something we should investigate in Bidi

jelly added the release-note label Jun 18, 2024

tomasmatus commented Jun 18, 2024

View reviewed changes

jelly requested changes Jun 18, 2024

View reviewed changes

tomasmatus commented Jun 18, 2024

View reviewed changes

tomasmatus force-pushed the basic_keybinds branch from ac26b32 to 8893f60 Compare June 19, 2024 10:09

tomasmatus force-pushed the basic_keybinds branch from 8893f60 to 8ee3c5a Compare June 25, 2024 11:17

github-advanced-security bot found potential problems Jun 25, 2024

View reviewed changes

src/dialogs/keyboardShortcutsHelp.tsx Fixed Show fixed Hide fixed

tomasmatus force-pushed the basic_keybinds branch 2 times, most recently from c3d4aa3 to 1745428 Compare June 27, 2024 09:39

jelly requested changes Jun 27, 2024

View reviewed changes

src/files-card-body.tsx Show resolved Hide resolved

test/check-application Outdated Show resolved Hide resolved

src/files-breadcrumbs.tsx Outdated Show resolved Hide resolved

tomasmatus force-pushed the basic_keybinds branch 2 times, most recently from 2c25a52 to adceb56 Compare August 5, 2024 10:17

tomasmatus commented Aug 6, 2024

View reviewed changes

tomasmatus force-pushed the basic_keybinds branch 2 times, most recently from eca07b3 to 802c7dd Compare August 13, 2024 07:22

tomasmatus requested a review from jelly August 19, 2024 07:29

tomasmatus force-pushed the basic_keybinds branch 2 times, most recently from 0b2424a to 92b7601 Compare September 2, 2024 13:00

jelly reviewed Sep 3, 2024

View reviewed changes

tomasmatus force-pushed the basic_keybinds branch from 92b7601 to 3803209 Compare September 4, 2024 12:03

tomasmatus force-pushed the basic_keybinds branch from 3803209 to 575e42a Compare September 4, 2024 14:04

tomasmatus added a commit to tomasmatus/cockpit-files that referenced this pull request Sep 16, 2024

Move global menu to the toolbar

84cb34e

relevant comment: cockpit-project#567 (comment)

tomasmatus added a commit to tomasmatus/cockpit-files that referenced this pull request Sep 17, 2024

Move global menu to the toolbar

872b2b1

relevant comment: cockpit-project#567 (comment)

tomasmatus added a commit to tomasmatus/cockpit-files that referenced this pull request Sep 17, 2024

Move global menu to the toolbar

2e3e5c7

relevant comment: cockpit-project#567 (comment)

tomasmatus mentioned this pull request Sep 17, 2024

Move global menu to the toolbar #739

Merged

tomasmatus added a commit to tomasmatus/cockpit-files that referenced this pull request Sep 17, 2024

Move global menu to the toolbar

5f6f9d1

relevant comment: cockpit-project#567 (comment)

tomasmatus force-pushed the basic_keybinds branch from 575e42a to 527580d Compare September 17, 2024 12:22

jelly reviewed Sep 17, 2024

View reviewed changes

test/check-application Outdated Show resolved Hide resolved

tomasmatus added a commit to tomasmatus/cockpit-files that referenced this pull request Sep 18, 2024

Move global menu to the toolbar

0809d73

relevant comment: cockpit-project#567 (comment)

tomasmatus added a commit to tomasmatus/cockpit-files that referenced this pull request Sep 23, 2024

Move global menu to the toolbar

203e82b

relevant comment: cockpit-project#567 (comment)

tomasmatus added a commit to tomasmatus/cockpit-files that referenced this pull request Sep 24, 2024

Move global menu to the toolbar

d64b451

relevant comment: cockpit-project#567 (comment)

jelly pushed a commit that referenced this pull request Sep 24, 2024

Move global menu to the toolbar

63257cf

relevant comment: #567 (comment)

tomasmatus force-pushed the basic_keybinds branch from 8d027e5 to 84ce5a0 Compare September 24, 2024 12:29

jelly reviewed Sep 24, 2024

View reviewed changes

jelly requested changes Sep 24, 2024

View reviewed changes

src/menu.tsx Outdated Show resolved Hide resolved

tomasmatus requested a review from jelly September 25, 2024 13:09

tomasmatus force-pushed the basic_keybinds branch from 542a8d0 to 99bc362 Compare September 25, 2024 14:04

cockpituous reviewed Sep 25, 2024

View reviewed changes

jelly approved these changes Oct 1, 2024

View reviewed changes

jelly merged commit 49de6d0 into cockpit-project:main Oct 1, 2024
25 checks passed

tomasmatus mentioned this pull request Oct 1, 2024

remove empty dependency array in useInit #756

Merged

martinpitt removed the release-note label Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic keyboard shortcuts #567

Add basic keyboard shortcuts #567

tomasmatus commented Jun 18, 2024 •

edited

Loading

tomasmatus Jun 18, 2024

jelly left a comment

tomasmatus commented Jun 18, 2024

tomasmatus Jun 18, 2024

tomasmatus commented Jun 19, 2024

garrett commented Jun 20, 2024

garrett commented Jun 20, 2024

tomasmatus commented Jun 25, 2024

garrett commented Jun 25, 2024

garrett commented Jun 25, 2024

jelly left a comment

tomasmatus Aug 6, 2024

jelly commented Sep 3, 2024

jelly Sep 3, 2024

garrett commented Sep 3, 2024

tomasmatus commented Sep 4, 2024

tomasmatus commented Sep 4, 2024 •

edited

Loading

jelly commented Sep 5, 2024

garrett commented Sep 9, 2024

jelly left a comment

cockpituous Sep 25, 2024

cockpituous Sep 25, 2024

cockpituous Sep 25, 2024

cockpituous Sep 25, 2024

cockpituous Sep 25, 2024

jelly left a comment

jelly Oct 1, 2024

jelly Oct 1, 2024

tomasmatus Oct 1, 2024

jelly Oct 1, 2024

jelly Oct 1, 2024

		return () => {
		document.removeEventListener("keydown", onKeyboardNav);

Add basic keyboard shortcuts #567

Add basic keyboard shortcuts #567

Conversation

tomasmatus commented Jun 18, 2024 • edited Loading

Files: basic keyboard shortcuts

Choose a reason for hiding this comment

jelly left a comment

Choose a reason for hiding this comment

tomasmatus commented Jun 18, 2024

Choose a reason for hiding this comment

tomasmatus commented Jun 19, 2024

garrett commented Jun 20, 2024

garrett commented Jun 20, 2024

tomasmatus commented Jun 25, 2024

garrett commented Jun 25, 2024

garrett commented Jun 25, 2024

jelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Sep 3, 2024

Choose a reason for hiding this comment

garrett commented Sep 3, 2024

tomasmatus commented Sep 4, 2024

tomasmatus commented Sep 4, 2024 • edited Loading

jelly commented Sep 5, 2024

garrett commented Sep 9, 2024

jelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasmatus commented Jun 18, 2024 •

edited

Loading

tomasmatus commented Sep 4, 2024 •

edited

Loading