Feature: Support keyboard shortcuts in Vomnibar.

[libklein]

Description

This PR adds support for keyboard shortcuts to Vomnibar. The dialog now renders jump tags numbered 0-9 inside suggested elements. Pressing Ctrl + i (configurable) selects item tagged [i] and submits the choice.

The changes are non-breaking and minimal:

• Added indicator support to generateHtml() in the Suggestion class. The function renders the indicator as [${indicator}] in front of the suggestion.
• Assign indicators 0-9 when post-processing suggestions in MultiCompleter.
• Adjusted CSS to correctly render the indicator: changed li display to flex and removed obsolete position: relative.
• React to Ctrl + [0-9] keypress in VomnibarUI.
• The feature is configurable and can be turned off.

I've attached a screencast of the feature.

Testing

I've manually tested the feature in the most recent version of firefox and chrome on linux.

[philg-dev]

Seems like a pretty neat feature.

Have you tested this on a Chromium browser with multiple tabs open? Or on Firefox on Windows with multiple tabs open? I'm asking because I'm not sure if the CTRL+1~9 shortcut would interfere with native browser tab switching.

AFAIK Firefox is kinda weird in that regard. If I remember correctly, on Windows Firefox's native shortcuts are CTRL+1~9, which I think is also the case for Chromium based browsers regardless of platform. Yet, on Linux Firefox natively uses ALT+1~9 instead of CTRL for tab switching.

So as much as you claim it's "non-breaking", it might still interfere with a user's expected behavior. Did you implement a way to toggle this feature or is it just always on? Does it consume the relevant key-events to avoid native browser tab-switching?

Unfortunately it's really not that helpful that your PR is cluttered with tons and tons of formatting changes that aren't even in a separate commit from the "real" changes. So reviewing this is kind of a pain.

While skimming through I saw some seemingly "random" changes to the existing vomnibar CSS. I'm not super familiar with Vimium theming, but that seems like it might break some custom themes.

Those are just some initial thoughts I had and wanted to share for discussion - no real critique to any of your work. I didn't have time to dive deeper into this for a full review.

I'm curious to see how this will turn out!

[libklein]

Hi @philg-dev, thanks for your comment.

The PR is not in mergeable state yet - I think it needs some code cleanup and further testing. My main motivation for raising was to discuss whether a feature like this would be a worthwhile addition to Vimium.

Concerning further testing - do you have a testing workflow you usually follow? Is there a defined process?

I agree on the code clutter, let me clean up the PR and implement a feature flag. Concerning the CSS changes: It is possible to implement this CR with minimal changes to CSS at the cost of slightly increased code complexity. Let me test with some common themes to see if anything breaks with the changes.

Happy to align on this - and thanks for considering the PR.

[philg-dev]

The PR is not in mergeable state yet - I think it needs some code cleanup and further testing. My main motivation for raising was to discuss whether a feature like this would be a worthwhile addition to Vimium.

Oh I see, that's good then. I personally think it's an interesting feature, even though I don't think I have relevant use-cases for it - as in: I never felt like I'm missing a quick(er) access to whatever results the Vomnibar spits out. I only remember using a similar feature a few years ago which was implemented in the Pop!OS launcher (which I think might be just rofi under the hood).

Concerning further testing - do you have a testing workflow you usually follow? Is there a defined process?

I'm not a maintainer of Vimium, I just follow the GitHub and did some rather minor code contributions. The minimal testing I personally would expect is, that stuff works on vanilla Firefox and Chromium, ideally including regression tests of the most important Vimium features. Some people might mistake me for philc, the repo owner... unfortunately we have quite similar GitHub handles.

The stuff about browser's native shortcuts for tab-switching seems like a must in terms of testing your feature in particular, that's why I mentioned it, to raise awareness of potential drawbacks or pitfalls.

Regarding the clutter, I was also just trying to make you aware of the fact that, in the current state the PR is unnecessarily tedious to review. The smaller / on-topic your changes are, the higher the likelihood of them getting merged anytime soon. Since you kinda intended the PR as a starter for discussion: I think you can mark a PR as a "draft" version on GitHub, if I'm not mistaken. That would be a good signal for people to know that it's not ready for review and thus, people won't waste their time on looking at the code changes.

[libklein]

I've cleaned up the PR and removed whitespace changes.

The stuff about browser's native shortcuts for tab-switching seems like a must in terms of testing your feature in particular, that's why I mentioned it, to raise awareness of potential drawbacks or pitfalls.

It works as intended, that is, the event never reaches the browser. It is important to point this out as a caveat though. I've made the jump configurable.