17465 Fixed dropdown cursor key navigation

What does this MR do?

This MR fixes the use of cursor/arrow/enter key events with search dropdowns, allowing a user to navigate up and down the list with the arrow keys and then select their item with the enter key. It also applies some minor scroll user experience fixes, such as resetting the selected dropdown item every time it opens/closes (also stops multiple dropdowns conflicting) and forcing the dropdown scroll to scroll right to the top or bottom depending on whether they have selected the first or last item, respectively.

Are there points in the code the reviewer needs to double check?

I would like someone with GitLab experience to check over whether this would harm any unique implementations of the GitLabDropdown or SearchAutocomplete.

Why was this MR needed?

The current version has incorrectly behaving search dropdowns in the navbar, they either do not navigate using the keyboard or do not use the enter keystroke to select a highlighted item.

What are the relevant issue numbers?

Fixes #17465 (closed). Closes #20752 (closed). Closes #21014 (closed). Contributes to #20754 (moved).

Screenshots (if relevant)

17465.mp4

Changed title: WIP: Added new non-selectable selector exclusions to fix arrow key events, fixed the … → WIP: 17465 Fixed dropdown cursor key navigation

Added frontend ~12980 ~113220 labels

Milestone changed to %8.9

mentioned in merge request !4155 (closed)

Added 1 commit:

• f9454594 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 1 commit:

• b9838404 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 91 commits:

• b9838404...5804b6a0 - 90 commits from branch master
• 3cb3a2aa - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 1 commit:

• 59609951 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Unmarked this merge request as a Work In Progress

Reassigned to @jschatz1

@iamphill You recently fixed the same issue. Want to see if the tests fit the bill?

Reassigned to @iamphill

@iamphill can you review?

I didnt look through the code fully however it doesnt work fully. When you click the search bar and get the quick actions - the arrow keys dont work. Works when you search perfectly.

My main issue is that it doesn't work on other dropdowns like the filters and I think the main issue is the . selectable check, we would have to update ALL the dropdowns to have this class. Is it not safe to assume that all elements apart from the divider are selectable?

Reassigned to @lbennett

@iamphill I remember initially I had some selector logic like yours using :not([few_non-selectable_item_classes]). We decided this wasn't the best approach due to its complexity. But you're completely right in terms of other dropdowns. I was worried about this when I found out about the label dropdowns etc but I wrongly assumed that the GitLabDropdown class would always be populated using the @options.data property. So this isn't true? There is many cases where the list items are hard coded into the HAML rather than parsed as a @options.data value?

If so, I suppose we still have the same issue of going around and re-classing all the hard coded list items with a single .non-selectable class but it will be much less tedious than adding .selectable to the majority of items. This could also be added by programatically w/ jQuery but thats essentially of equal complexity as the not :not() selector as there would have to be a list of all non-selectable classes.

@lbennett yeah there is lots of different places where the dropdowns are used. Some with JS, some in HAML.

What classes aren't selectable?

@iamphill It was along the lines of li.divider, li.header but I think there were 1 or 2 more.

Would something like this work:

notSelectable = ['.divider', '.header']

$().is(":not(#{notSelectable.join(',')})")

Then notSelectable can expand to whatever.

I was thinking

nonSelectableClasses = ['li.divider', 'li.header']

listElements = $('.dropdown-menu ul')

# Add a single class to all non-selectable items
$(nonSelectableClasses.join(', '), listElements).addClass 'non-selectable'

# Use simpler selector
selectableListElements = listElements.not '.non-selectable'

But I really don't like the idea of syncing nonSelectableClasses whenever someone adds a new type of non-selectable element.

I would of thought there is only 2 anyways? But give that a go.

Hmm, I could write a test that checks that all dropdown list items are accounted for in gl_dropdown.coffee and we have a combination of nonSelectableClasses = [...] and selectableClasses = [...], if a class doesn't exist in either of them it will error. But this would be odd as it would have to grep all views as we can't just use the Jasmine fixture as it has the exact same problem of having to be manually updated when changed.

Milestone changed to %8.10

Added 345 commits:

• 59609951...4c767bab - 344 commits from branch master
• ac3624bd - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

@iamphill Those changes are in. :)

Reassigned to @iamphill

@lbennett build is failing.

Also - still not sure the .non-selectable class is going to work that well. What if the data is loaded async? The way you have done it that isn't added in. I still think the best way is by looking for NON_SELECTABLE_CLASSES rather than adding a new class.

Reassigned to @lbennett

@iamphill Will switch that out and get building.

Added 19 commits:

• ac3624bd...0115ab7f - 18 commits from branch master
• a3b022a1 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

@iamphill That works much better and should catch everything in any dropdown menu. It's funny the circles you have to go in sometimes.

There is a little issue with the search dropdown, the autocomplete dropdown appears and clicking the down arrow closes the menu before it can select an option. I remember I fixed this but must have caused a regression. Will fix shortly. :)

Oh also need to patch XSS vulns.

Added 53 commits:

• a3b022a1...c9a46263 - 52 commits from branch master
• 37dc7796 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Fixed that search issue and the XSS. Waiting for master builds to fix and see where it goes. :)

Added 37 commits:

• 37dc7796...1ade080e - 36 commits from branch master
• 187957a5 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 21 commits:

• 187957a5...ebe21acc - 20 commits from branch master
• 5a92268a - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 1 commit:

• 843ccaa5 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 34 commits:

• 843ccaa5...bef4294c - 33 commits from branch master
• d034583f - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Reassigned to @iamphill

Reassigned to @lbennett

Thank you @iamphill! Will get on it.

Added 60 commits:

• d034583f...1c2e7af6 - 59 commits from branch master
• be9779bf - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 39 commits:

• be9779bf...3041c433 - 38 commits from branch master
• 665bc59f - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 75 commits:

• 665bc59f...fc3402b7 - 74 commits from branch master
• d03ca2b3 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 1 commit:

• 039d3d48 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Added 1 commit:

• 8277a689 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …

Reassigned to @iamphill

Slack conversation relating to the most recent changes (except I used Turbolinks instead of location obj).

@lbennett Turbolinks is a good way to solve it - however it doesn't seem to work everywhere. For example: go to an issue, try to set an assignee with just the arrow keys. Notice it will send the Turobolinks.visitrather than change the assignee.

@iamphill Nice find, I'll take a look into this.

Reassigned to @lbennett

Added 1 commit:

• 31672077 - improved turbolinks.visit conditional

Reassigned to @iamphill

@lbennett 2 small comments & a failed build. But looks good to me & works well.

Can we fix the conflicts?

Added 2 commits:

• ed4fb832 - Review changes and final formatting
• b1e795f8 - Fixed test turbolink condition

Added 1 commit:

• 89c0fe85 - Fixed test turbolink condition

Added 237 commits:

• 89c0fe85...3c89a788 - 233 commits from branch master
• ac6aea15 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …
• f08c60d0 - improved turbolinks.visit conditional
• 25b8645f - Review changes and final formatting
• 7321f05e - Fixed test turbolink condition

Added 80 commits:

• 7321f05e...cfb5a76b - 76 commits from branch master
• 045bf6af - Added new non-selectable selector exclusions to fix arrow key events, fixed the …
• 298e54e2 - improved turbolinks.visit conditional
• 6081c818 - Review changes and final formatting
• db4f4065 - Fixed test turbolink condition

@iamphill @jschatz1 Good to go!

@iamphill ping

&& should be and

If we are putting the below on a new line, this probably should be as well

@lbennett There is a lot of changes in app/assets/javascripts/gl_dropdown.js.coffee that I cant see if they are relevant to this MR - they look like code style updates. This should be done in a new MR.

Reassigned to @lbennett

@iamphill Ahh, yeah, I see now that it makes reviewing incredibly hard. Sorry, I'll redo it.

@iamphill Changes are in and I removed that formatting.

Added 626 commits:

• db4f4065...1cd573ee - 621 commits from branch master
• 0332d40f - Added new non-selectable selector exclusions to fix arrow key events, fixed the …
• 0c482fa0 - improved turbolinks.visit conditional
• 064de60d - Review changes and final formatting
• a1fb47ce - Fixed test turbolink condition
• 4e6e9c1a - Further review changes and removed large ammounts of reformatting

Reassigned to @fatihacet

mentioned in issue #18939 (closed)

We got's build failures

Reassigned to @lbennett

@lbennett I assigned this back to you. Can you assign me back when the build is green.

Added 537 commits:

• 4e6e9c1a...033e5423 - 531 commits from branch master
• a924e242 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …
• 02e4c0cd - improved turbolinks.visit conditional
• c51a0fd4 - Review changes and final formatting
• 642ad44c - Fixed test turbolink condition
• b807b1c3 - Further review changes and removed large ammounts of reformatting
• ed2f8d68 - Fix failed tests

Added ~149423 label

Reassigned to @fatihacet

@fatihacet Green!

@lbennett Can you update this MR with JS/ES6.

Reassigned to @lbennett

Milestone changed to %8.11

Removed ~149423 label

mentioned in merge request !5466 (merged)

Added 124 commits:

• ed2f8d68...02d947f1 - 117 commits from branch master
• f1756d8b - Added new non-selectable selector exclusions to fix arrow key events, fixed the …
• 3f5133bb - improved turbolinks.visit conditional
• 50668902 - Review changes and final formatting
• b3a22c93 - Fixed test turbolink condition
• a8f2c747 - Further review changes and removed large ammounts of reformatting
• 5783cf1f - Fix failed tests
• b53d8780 - Moved changes across to es5 and changed spec to es6

Added 12 commits:

• b53d8780...c675bdad - 4 commits from branch master
• 7b06b294 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …
• 715f24f7 - improved turbolinks.visit conditional
• f34f89fb - Review changes and final formatting
• ebdb64bf - Fixed test turbolink condition
• 7cc21750 - Further review changes and removed large ammounts of reformatting
• d1747d8e - Fix failed tests
• 4ad38f9d - Moved changes across to es5 and changed spec to es6
• f13710cc - Improved spec, waiting on sprockets-es6 to work with rspec....

mentioned in merge request !5524 (merged)

Waiting on !5524 (merged) to unblock, shouldn't take long. :)

Added blocked label

Added 86 commits:

• f13710cc...a88a4e85 - 77 commits from branch master
• 8fc776ce - Added new non-selectable selector exclusions to fix arrow key events, fixed the …
• 632ea755 - improved turbolinks.visit conditional
• bfa9ae6a - Review changes and final formatting
• 0d567ce5 - Fixed test turbolink condition
• 64e4a4f3 - Further review changes and removed large ammounts of reformatting
• e4c6a98d - Fix failed tests
• 0660030d - Moved changes across to es5 and changed spec to es6
• 7b7326d3 - Improved spec, waiting on sprockets-es6 to work with rspec....
• efc21973 - Fixed spec and improved formatting

Added 86 commits:

• efc21973...a330b29b - 76 commits from branch master
• 4647e092 - Added new non-selectable selector exclusions to fix arrow key events, fixed the …
• 9c66ad5d - improved turbolinks.visit conditional
• 157a2848 - Review changes and final formatting
• f1df0cf4 - Fixed test turbolink condition
• ecbf93a2 - Further review changes and removed large ammounts of reformatting
• 0be67aa1 - Fix failed tests
• 5ff849ea - Moved changes across to es5 and changed spec to es6
• 8be6df5b - Improved spec, waiting on sprockets-es6 to work with rspec....
• e3c7d726 - Fixed spec and improved formatting
• bdf7accd - Fixed specs

Added 1 commit:

• bd4da47d - Fixed specs

Reassigned to @fatihacet