Fixing keyboard shortcuts #27374

noveens · 2017-03-13T15:11:33Z

Description

I am trying to fix and introduce newer keyboard shortcuts.

Changes can be tested at : http://noveen.tk/core
Username : owncloud
password : 123

IMO we should try to implement a new page in settings named controls, so that the user can customise their shortcuts. (comments?)

Till now I have managed to fix various shortcuts plus introduce new shortcuts (I will keep adding them here, feel free to add shortcuts and I'll be happy to implement them)
These implementations include proper error messages plus testers for this branch are welcome !

Related Issue

#27359 #9740 #19442 #27580

Motivation and Context

Enhancement.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

@PVince81 , Please guide me on this one 😄

mention-bot · 2017-03-13T15:11:36Z

@noveens, thanks for your PR! By analyzing the history of the files in this pull request, we identified @eriksargent, @Kondou-ger and @DeepDiver1975 to be potential reviewers.

PVince81 · 2017-03-13T16:23:09Z

apps/files/js/keyboardshortcuts.js

+
+	function favorite() {
+		var countSelected = 0;
+		$("#fileList tr").each(function(index) {


this will not work. Try having more than 50 files in the list.
Then use the "select all" checkbox without scrolling down.
Then favorite.

It will miss the other entries. Need special handling.

This is because it doesn't render the next items as "tr" until you scroll down, for performance (pagination).

Might affect other shortcuts as well so I recommend to test this scenario with the other actions.

On another note, the selector #fileList tr.select would be way more direct

@PVince81 ,
I'm having issues trying to get focus on the selected table row while I press the DOWN arrow key beyond the screen.

can you please give me some pointers so as to scroll the screen when the "mouseOver" tr gets lower than the screen?

Ah yes, that can be quite a struggle.

You need to get the scroll container of the list and then set its 'scrollTop' value to make it scroll down. I don't remember the details.

You can find the scroll container element by inspecting the this.$container value of the FileList instance.

The most difficult part here with shortcuts is that you're trying to operate directly on the DOM instead of talking with the matching FileList instance. Currently it's not possible to get the current FileList from the files App object. Maybe this could be extended to make such API calls easier.

noveens · 2017-03-13T16:38:48Z

Also @PVince81 ,

I was thinking of implementing a controls page under personal settings,
Which would display the current list of controls and those would be editable, values of which will be stored in a database.
Does this seem good, should I start work on this?

PVince81 · 2017-03-13T16:59:29Z

I was thinking of implementing a controls page under personal settings,
Which would display the current list of controls and those would be editable, values of which will be stored in a database.

I think this is not needed as it is too much of a corner case. Keep it simple, sensible defaults.

noveens · 2017-03-14T05:47:50Z

@PVince81 ,
We should at least list the shortcuts somewhere.

They're no use unless listed somewhere.

PVince81 · 2017-03-14T08:03:13Z

List them in the documentation then. Then need to find a place to put the link in the UI to access it.

noveens · 2017-03-14T08:17:41Z

How about "general" settings under the personal tab?

noveens · 2017-03-20T01:40:10Z

@PVince81 ,
First up, sorry for so much delay, had exams in my university 😉

Until now,
I have managed to :

show the selected file at centre of screen
if select all checkbox is true, all files are made favourite not only the view ones.
apply above check while deleting files.
write the documentation 😢
give link of controls' documentation in settings

Anything else I am missing?

Additions:

use spacebar to toggle the sidebar

Thanks 😄

PVince81 · 2017-03-20T09:14:32Z

Something I'm missing now (which could be done separately) is a key to open the sidebar.
Currently the enter key opens the entry with the default action.
I'm not sure what key would be best for this, maybe spacebar ?

BUG: shift+d, shift-n, shift-r do not seem to work on my chromium browser, they don't do anything while arrow keys work
design issue: the highlight for the entry viewed in the sidebar conflicts with the arrow keys thing (to be discussed separately)
BUG: create a big list of files, then use the arrow up key to jump to the last item: it scrolls. Then use arrow down, it will select the first item but will not scroll. Only when hitting arrow down again will it scroll.
BUG: regardless which letter I push (ex: "O"), the cursor goes down as if I hit the arrow down key. Maybe there is some event mix up somewhere.

Did you solve the issue for when the list has 100 entries and only the first page is rendered ? What happens if you hit arrow up, will it render all pages first ?

noveens · 2017-03-20T11:44:19Z

@PVince81 ,
Thanks for the review 😄

I can't seem to reproduce that any key moves the cursor.
Even I thought about pressing up key while at page 1, it makes the cursor to last element in page 1, while loading the next page. (SUGGESTION : remove cycled navigation)
About chromium, the shortcuts seem to work on chrome + firefox, I'll test on chromium.

EDIT:
The shortcuts work on chromium as well.

noveens · 2017-03-20T14:37:08Z

@PVince81 , views?

PVince81 · 2017-03-22T09:36:26Z

Seems that after clearing the cache it works correctly on my Chromium 56. It did really weird shit.

I also tested what happens if I hit shortcuts while in the rename field, it seems they are not triggered which is good.

Please don't use backspace for deletion. I'd rather have backspace go to the parent folder as it's less dangerous.

noveens · 2017-03-22T11:41:40Z

@PVince81 ,
I am currently simulating a back button when pressing backspace.
Should I update it to specifically go to the parent folder (if not at root dir)?

PVince81 · 2017-03-22T14:47:31Z

I think let it go to the parent folder, not the previously visited folder.

Interestingly when I hit Backspace Chromium tells me "please use alt+left" to go to previous page. I think backspace used to be the "browser back" button shortcut. Not sure about other browsers though.

noveens · 2017-03-22T22:27:12Z

@PVince81 ,
I now call the API to change the directory to go to the parent directory rather than simulating a back button 😄
Can you have a look?

Thanks.

noveens · 2017-03-22T22:30:10Z

@tomneedham ,
can you please review? 😄

Thanks.

PVince81 · 2017-03-23T10:22:28Z

BUG: up arrow wrap with multiple pages doesn't jump to correct entry

Create 100 folders: for I in $(seq 1 100); do curl -X MKCOL -u admin:admin "http://localhost/owncloud/remote.php/webdav/$I"; done
Load the page (in Chromium)
Hit the "up" arrow

Expected: goes to the last item 100
Actual: goes to the last item of the current page which is 20

BUG: esc key doesn't close "new folder" menu on Chromium

Given that this PR is already an improvement I'd be fine merging this first and fixing the issues separately.

PVince81 · 2017-03-23T10:27:54Z

Ah, forgot to test the public link page:

Public link page

BUG: arrow keys do not seem to scroll down the container and also prevents next page to be loaded, so even though I have 100 files in the folder it wraps already around 20 (the visible entries)
BUG: public link page shows many errors about


update_view @ keyboardshortcuts.js?v=323841b…:133
select_down @ keyboardshortcuts.js?v=323841b…:197
(anonymous) @ keyboardshortcuts.js?v=323841b…:486
dispatch @ jquery.min.js:3
r.handle @ jquery.min.js:3
keyboardshortcuts.js?v=323841b…:133 Uncaught TypeError: Cannot read property 'fileList' of undefined
    at update_view (keyboardshortcuts.js?v=323841b…:133)
    at select_down (keyboardshortcuts.js?v=323841b…:197)
    at HTMLDocument.<anonymous> (keyboardshortcuts.js?v=323841b…:486)
    at HTMLDocument.dispatch (jquery.min.js:3)
    at HTMLDocument.r.handle (jquery.min.js:3)

BUG: when link share is read-only, hitting "Shift-D" still displays the folder creation popup

you might need another approach to access to fileList instance.

Other sections

For example "Favorites" section.

BUG: the arrow keys do not work there at all, probably because the keyboard module is targetting only the default list.

AFAIK there is no easy way to talk to the currently visible FileList instance.
But maybe if the keyboard stuff is rewritten as a FileList plugin then it could work.

My main worry about the current file shortcut code is that it isn't really well integrated (it's very old code from back when there was only one file list view).

PVince81 · 2017-03-23T10:30:28Z

apps/files/js/filelist.js

+				$scrollContainer = $('body');
+			}
+
+			if ($(".mouseOver").position().top >= this.$container.height() / 2) {


please never use absolute selectors, they are inefficient because they'd need to scan the whole page to find elements.

Use specific selectors, for example this.$el.find('.mouseOver')

PVince81 · 2017-03-23T10:30:49Z

apps/files/js/filelist.js

+		/**
+		 * Favorite all files in the current fileList
+		 */
+		favoriteAll: function() {


no, this belongs in the favorite plugin file, not the main list

PVince81 · 2017-03-23T10:31:56Z

apps/files/js/filelist.js

+		/**
+		 * Delete all files in the current fileList
+		 */
+		deleteAll: function() {


I'd prefer not having a keyboard shortcut that deletes ALL files. The user must first select everything manually (ctrl+a?) and then hit delete. No direct shortcut.

This is called only when the user clicks the select all button via mouse and presses DEL.

there isn't any shortcut to delete all files.

I see. Then you can try simulating a click on .delete-selected instead.

PVince81 · 2017-03-23T10:32:58Z

apps/files/js/filelist.js

+				var model = this.getModelForFile(this.files[i].name);
+				if(!model) {
+					// item not loaded due to pagination
+					this._nextPage(false);


This only renders the next page. What if the file is two pages down ?

In general I think the file models are already available and are not dependent on pagination, so you could simply remove this "if" block.

@PVince81 ,
No, the models aren't available if the element is not being displayed on the screen.
I had to introduce the check to make sure the model is made available by showing the next page.

Also, If the element is 2 pages down, it doesn't matter, the this._nextPage(false) will be called twice (since the model for 2 page down element wouldn't also be available)

Ah yes, indeed. The models won't be fully available until this big construction site is finished: #25909

will be called twice

Where does it loop ? Reading the code from top to bottom, if the second getModelForFile still returns null then you'll pass null as model to the deletion trigger.

Anyway, based on my other comment this block needs to be removed anyway as it's part of the redundant deletion code.

IMHO, the getModelForFile will never return NULL after loading the next page.

Maybe it's a side effect of the way you programmed this.

Since you're iterating over all files, then it is likely that before reaching the end of the page, the previous iteration it already loaded the next page, so all subsequent calls will still succeed.

My point is that the way nextPage is implemented is that it only fetches a single next page. The way you used it seems to work somehow due to your for loop.

Yes, I fetch the nextPage if model isn't available loading the models of all the next page elements.

PVince81 · 2017-03-23T10:34:30Z

apps/files/js/keyboardshortcuts.js

-		$("#new").addClass("active");
-		$(".popup.popupTop").toggle(true);
-		$('#new li[data-type="file"]').trigger('click');
+		$(".new").click();


Ideally this needs to be scoped to the currently visible file list.

You could try using the selector $('#app-content .viewcontainer:not(.hidden)' as a prefix to only target the currently visible file list DOM elements.

PVince81 · 2017-03-23T10:37:00Z

apps/files/js/keyboardshortcuts.js

+	function favorite() {
+		if ($("#select_all_files").is(":checked")) {
+			// selected all files
+			OCA.Files.App.fileList.favoriteAll();


OCA.Files.App.fileList is always the fileList of "All files", not the other ones.

Maybe we should store the fileList object as a data attribute of the "viewcontainer".
Basically in the FileList constructor you add this:

this.$el.data('fileList', this);

then from outside you get the currently visible view container:
var fileList = $(#app-content .viewcontainer:not(.hidden)).data('fileList').
(note: might need tweaking for public link share page)

Then in all keyboard actions you can be sure that the targetted fileList object is the current one.

…s under tabs plugin

PVince81 · 2017-11-24T08:53:24Z

@noveens there were a few comments not adressed yet. Do you intend to continue working on this ?

codecov · 2017-11-24T13:56:53Z

Codecov Report

Merging #27374 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #27374   +/-   ##
=========================================
  Coverage     62.15%   62.15%           
  Complexity    17516    17516           
=========================================
  Files          1045     1045           
  Lines         57740    57740           
=========================================
  Hits          35890    35890           
  Misses        21850    21850

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d750519...c2b9d3b. Read the comment docs.

PVince81 · 2019-02-07T15:52:46Z

@noveens any interest in bringing this to completion ?

also this here would require some UX review to make sure the shortcuts are intuitive and make sense

note that our focus is now on the files app in the Phoenix project instead so not sure if we should spend more effort here (effort being dev, review, qa, etc)

DeepDiver1975 · 2019-02-08T06:34:30Z

I vote for close

PVince81 reviewed Mar 13, 2017

View reviewed changes

PVince81 added the 2 - Developing label Mar 17, 2017

noveens added 3 - To Review app:files and removed 2 - Developing labels Mar 20, 2017

noveens requested review from DeepDiver1975 and tomneedham March 21, 2017 12:46

PVince81 reviewed Mar 23, 2017

View reviewed changes

noveens added 9 commits November 24, 2017 09:50

corner selection bug fixed

00dea2d

spacebar can toggle side-bar, select all deletion deletes all

57278ab

backspace to go back in history

261446b

backspace used to go to parent folder not simulate a back button

f34ec7f

many, literally MANY errors fixed

6d8f186

removed deletion confirmations and notifs, placed the favorite method…

26ff48c

…s under tabs plugin

DOM coding removed, code refactored

ce2f92b

selectUp and selectDown methods refactored

60ce124

functions operating on DOM seperated, optimization

c2b9d3b

DeepDiver1975 force-pushed the fixing_keyboard_shortcuts branch from 67180f9 to c2b9d3b Compare November 24, 2017 08:51

ownclouders added the review carefully! label Nov 24, 2017

ownclouders added the status/STALE label Dec 7, 2017

ownclouders added status/STALE and removed status/STALE labels Jan 30, 2018

ownclouders added status/STALE and removed status/STALE labels Feb 9, 2018

PVince81 modified the milestones: triage, maybe some day May 22, 2018

ownclouders removed the status/STALE label Jul 12, 2018

ownclouders added the status/STALE label Jul 20, 2018

ownclouders removed the status/STALE label Feb 7, 2019

PVince81 closed this Feb 8, 2019

PVince81 deleted the fixing_keyboard_shortcuts branch February 8, 2019 09:45

lock bot locked as resolved and limited conversation to collaborators Feb 9, 2020

Fixing keyboard shortcuts #27374

Fixing keyboard shortcuts #27374

Conversation

noveens commented Mar 13, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mention-bot commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noveens Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noveens commented Mar 13, 2017

PVince81 commented Mar 13, 2017

noveens commented Mar 14, 2017

PVince81 commented Mar 14, 2017

noveens commented Mar 14, 2017

noveens commented Mar 20, 2017 • edited Loading

PVince81 commented Mar 20, 2017

noveens commented Mar 20, 2017 • edited Loading

noveens commented Mar 20, 2017

PVince81 commented Mar 22, 2017 • edited by noveens Loading

noveens commented Mar 22, 2017

PVince81 commented Mar 22, 2017

noveens commented Mar 22, 2017

noveens commented Mar 22, 2017

PVince81 commented Mar 23, 2017 • edited by noveens Loading

PVince81 commented Mar 23, 2017 • edited by noveens Loading

Public link page

Other sections

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

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

noveens Mar 23, 2017 • edited Loading

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

PVince81 commented Nov 24, 2017

codecov bot commented Nov 24, 2017 • edited Loading

Codecov Report

PVince81 commented Feb 7, 2019

DeepDiver1975 commented Feb 8, 2019

noveens commented Mar 13, 2017 •

edited

Loading

noveens Mar 13, 2017 •

edited

Loading

noveens commented Mar 20, 2017 •

edited

Loading

noveens commented Mar 20, 2017 •

edited

Loading

PVince81 commented Mar 22, 2017 •

edited by noveens

Loading

PVince81 commented Mar 23, 2017 •

edited by noveens

Loading

PVince81 commented Mar 23, 2017 •

edited by noveens

Loading

noveens Mar 23, 2017 •

edited

Loading

codecov bot commented Nov 24, 2017 •

edited

Loading