Keyboar selection in list using arrow keys

[turboMaCk]

This is still work in progress (missing FF version).

I've really missed arrow keys selection so I've coded it myself. I'm opening this PR early in case you're interested to review and discuss it. If you're not interested in this feature, feel free to close this PR - I can always use my own version of extension anyway. In case you're interested I'm going to clean code a bit implement the same feature for firefox as well as include changes from code review.

[maximbaz]

Thanks @turboMaCk, I'm definitely interested in getting this merged 🙂 This feature was also requested a while ago in a comment somewhere else.

I'll be reviewing the code a bit later today or tomorrow.

[maximbaz]

I've tried it out, my immediate thoughts: I'm concerned that you are basically re-implementing the selection feature, you are writing lots of code to do this, you are coloring the selection, keeping track of highlighted item, etc... First of all, all this is already implemented by the browser itself (what you see when pressing Tab and Shift+Tab), second - it is conflicting with pressing Tab (try pressing both arrows and Tab, you will see two selections).

I have two ideas, not sure if any of them is possible, but could potentially simplify this PR a lot.

1. Add keydown listener, and when Up or Down is pressed, just initiate a Tab or Shift+Tab keypress, and let the browser do the rest of the work.
2. Similar to above, but instead of initiating Tab keypress, manipulate tabIndex property of the buttons. A bit trickier, but still simpler, just find the currently focused element and shift the index to the next one.

Both ideas will allow you to reuse most of what is already implemented by browsers, and arrows will not conflict with Tab / Shift+Tab key presses.

What do you think about these?

[maximbaz]

Had a chance to investigate a bit, here are the findings:

1. Browsers do not allow triggering Tab keypress, so this is not an option for us.
2. This will work, and it is actually relatively simple. document.activeElement returns you an element which currently has focus. You have several possible outcomes here:
   • The focus is currently on the search input box - this is the starting point. In such case, find the first button and call .focus() on it.
   • A button has focus - use the document.activeElement.nextElementSibling to find the next button:
     • If it is not null, call .focus() on it
     • If it is null, it means the last button has focus now - move the focus back to the input box.
   • Nothing has focus - don't think this is possible, but you can handle this as well, just in case, and move the focus to the input box.

[turboMaCk]

Hello, @maximbaz 👋

Thanks for super fast response. I was away until now so sorry for not keeping up with you in this.

Frankly I even didn't notice that I can use native focus with this extension since I dind't know items are buttons but it makes quite some sense. There are some advantages for using custom highlighting over native focus. For instance I like to be able to type and using sellection at the same time but probably it's not as usefull in this case sice search is performed on form submit not input (which makes sense from security perspective).

The fact that this doesn't play well with native focus is game changer indeed though and I agree it we need to fix that. In this implementation key handler is on input so on Tab you even loose that and there are other issues like ones you have already described.

I think it's pretty clear that you would prefer arrows to behave as much like native focus as possible so I'll reimplement it that way. One thing I'm not so much sure about is how will usage of DOM apis play with virtual dom of mithril. Probably the best way to find out is to try most simple solution first and see how it goes.

I'll keep you up to date. Let me know if you have so additional points.

[maximbaz]

Yeah I'd prefer to use the native focus as much as possible. Let me know if you get into issues and I'll try to help as much as I can, but I tested the things above (like document.activeElement and .focus()) in console and they did work fine.

By the way, I was toying recently with the idea to perform search on keydown instead of form submit, and even if we ignore security perspective for a minute, the current UI is simply not fit for this, everything is rather ugly, the box is jumping and changing width and height every time, and flashing between doing the searches... in the end, it's no good, I think there are too many reasons for now to stay with the "form submit" event 🙂

[turboMaCk]

I'm using id for selecting and detecting focus on input. It seemed to be better than reading other attributes (like tagName) to me.

[turboMaCk]

As you can see handler is assigned to a container of results and container of input. I did it because I didn't want to change structure without asking you first. Alternatively, we can add a new extra container to render or use native API to register the handler.

[maximbaz]

I don't mind wrapping both containers in a some kind of parent container, and assigning the onkeydown to it. But if for whatever reason it gets complicated, the current solution is also fine.

[turboMaCk]

this is using querySelector. Compatibility should be probably fine due to browser selection. https://www.w3schools.com/jsref/met_document_queryselector.asp

[turboMaCk]

I'll wait for your review before porting/integrating this to FF version.

[maximbaz]

I like it much better now @turboMaCk! And by the way, it works very well on both my Chromium and Firefox 😉 Just to make sure, you know that we have the same code for both browsers, right? After you run make js, the firefox folder is ready to be imported as temporary extension. Do test on your FF as well though, just to make sure it works for you too 🙂

Regarding to your suggestion to wrap everything in a parent container, seems like a valid idea to me. Try it out, and let me know when you are done implementing and testing, so that I can do one final test and merge everything 🙂

[turboMaCk]

Gotcha. There is one tiny issue in FireFox though. Native focus circles between input > [ list of items one by one ] > whole form. This causes 2 things:

• Arrows behave differently (never focus the whole form and goes straight to input).
• When the whole form is focused, Arrows are not working.

2nd issue should be possible to fix by reversing logic in this check to not button.item I think.

The first issue would require overloading Tab behavior which seems hacky.

[turboMaCk]

stupid class name but is fine for the view as small as this.

[maximbaz]

1. Let's not overload Tab behavior, what you implemented for arrows is good and intuitive.
2. The chance of people using both Tab and arrows is small, but if you have an idea on how to fix arrows while the entire form is focused - let's try it.

[turboMaCk]

It seems that extra thing firefox focus is not form but some node above new div.container. This means that even event listener is not firing when it's focused. Solution to that would be probably to register a listener on the whole document but that seems to me like anti-pattern in Mithril. Let me know what you think but to me, this seems to be good enough. We can always add any hack later in case users will find it confusing.

[maximbaz]

Then let's not introduce any such hacks for now. I'll test it later today, and merge if everything is okay. Thanks for this PR!

[maximbaz]

Merged in c3a6279.

What I did extra is I removed a parent ".container", because it looked like that and I don't see a need for the outermost container:

Thanks once again!

---

My testing on Firefox revealed the following troubling issue: Firefox does not respect the autofocus property, meaning that when users use hotkey to open the popup, they still need to press Tab, Tab to even get the focus to the search box. This is not related to your change, I'm just surprised that nobody reported this issue so far. Once the focus is on the search box, arrows work as expected.

Recorded as #155.

[turboMaCk]

@maximbaz I've spotted that issue as well myself just didn't want to mix it with this one. Great you've made an issue - I wanted to create one myself once this is closed. I'll subscribe to that issue and in case I'll have some time before anybody else will pick it up I'll do so myself.

Thanks for all your help with this PR - very appreciated.