Tiny Tiny RSS: Community

Keyboard shortcuts stop working after clicking buttons


#1
  1. Click on a button in the toolbar (and click again to close the popup menu), or click any of the +/- icons to expand/fold a feed category, or (double-)click anywhere on the toolbar.
  2. Try to use any keyboard shortcut, such as “g f”

You will find all (default) keyboard shortcuts have stopped working.

You can only start using the shortcuts again after clicking inside div#headlines-wrap-inner, or on a feed/feed category.

Tested with commit 656475ec78 (while plugins were disabled) on Waterfox 56, Ungoogled-Chromium 73 and Firefox 65. No messages are found in the browser console when keyboard shortcuts fail to respond.

(Red Hat, PHP 7.2.13, MySQL)


#2

possibly related to https://git.tt-rss.org/fox/tt-rss/pulls/110 - try posting in that PR thread

+ i’ll try take a look at this tomorrow


#3

this is likely unrelated to the aforementioned hotkey overhaul pull, it’s because of this:

this is how it always worked. otherwise you’d get into serious trouble with hotkeys triggering when you’re trying to input text.

this check needs to be further limited to only apply to specific subsets of input elements, going by class or type. i.e. include input type text, search (more?), and maybe a few more dojo-related things (.dijitInputInner, more?). i’ll need to think this over.


#4

Thanks for checking. I thought at first it would be that, but neither the Menu button, nor the expand/collapse icons are INPUT or TEXTAREA elements, nor does the mouse click event propagate to one.

Some buttons in the toolbar do have an INPUT element, but it doesn’t seem that’s what is clicked, or what the click event propagates to.

So, I changed that to:

if (event.target.nodeName == “INPUT” || event.target.nodeName == “TEXTAREA”) {
console.log(“ABORTING keyboard shortcut evaluation!”);
return;
}

and upon testing, the text was NOT written to the console. Seems like that isn’t the culprit.

(Fancy double quotes added to code by Discourse, there’s probably a tag for coding, but doesn’t matter for this.)

What seems to be the issue is that only a keydown event is triggered, after which

if (event.type == “keydown” && event.which != 27 && (event.which < 37 || event.which > 40)) return;

prevents the handling.


#5

then its the above PR after all

i’m starting to think that this maybe should be reverted


#6

Yeah, I’m guessing this keyboard shortcuts overhaul introduces issues that I at least have never had before in >5 years of use…

Perhaps there’s a way to not use keypress, only keydown, while keeping the rest of the code intact, but my initial test with that was unsuccessful.


#7

if keypress event is eaten when we have those elements focused and there’s really no way around it, we’ll have to revert this and go back to keydown. shame, this new way introduced a few problems, but overall it was kinda less hack-y.

e: i’ve pinged the PR thread, maybe suraia will have some ideas.


#8

I agree. On the surface, it looks elegant. It just doesn’t work, though :stuck_out_tongue:


#9

Sorry, took me a while to dig through the Dojo/Dijit nightmare.

I have created PR 114 (https://git.tt-rss.org/fox/tt-rss/pulls/114), which should hopefully fix these issues.
Please let me know whether it works for you.


#10

thanks for your efforts, i’ll take a look a bit later


#11

i wonder though, dijit probably does that to provide keyboard navigation. wouldn’t merging that PR break it? although i’m not sure if anyone really needs or expects to tab through dijit controls anyway.

also we have many more toolbars, for example in preferences. clicking on those would also break hotkeys? adding those dummy methods to each and every toolbar, forever, doesn’t sound like the best idea.

e: in all honesty i’m having doubts over this redesign

  • breaks all non-QWERTY hotkeys altogether
  • this toolbar/button focus shit needing workaround or a custom toolbar class with mixins baked in
  • keyboard navigation in dojo controls broken (?)

as opposed to

  • some QWERTY-alike layouts are broken

i’m not entirely convinced we’re moving forward here


#12

As far as I could find out, Dijit uses a weird mix of the navkey mixin plus other methods within the different classes.

To be honest, I have not checked whether tabbing ever worked. Using the up/down arrow keys did not work in the tree and instead changed the articles. Using the left/right arrows only worked if you clicked the expanders before and was used to expand/collapse the categories. Using the keyboard still works if you click on a button within the toolbar, just not the toolbar itself.

I first wanted to get feedback before touching the other toolbars. I have a local change that creates a new fox.Toolbar class, that overrides the appropriate methods. This way, we do not have to change all toolbars (well, except for the name of course).

I assume by non-QWERTY you mean cyrillic keyboards etc.? Because I have a QWERTZ layout and (complex) hotkeys finally work for me.

As far as I can tell, keydown suffers from very similar problems. If a Dijit class uses the arrow keys, their keydown propagation will also be stopped.

As mentioned above, at least within the feed tree, they also did not work before.

If you want, I can give the other toolbars etc. a go. If you prefer the old approach, no worries, even though I have gotten used to being able to use the hotkeys. :slight_smile:


#13

Would it break on my Dvorak keyboard?

image


#14

I just tested your pull request.
After clicking on…

  • an expand/collapse button in freed tree, hotkeys now work
  • an empty region in the toolbar, hotkeys now work
  • the Menu button in toolbar, hotkeys now work
  • any drop-down button in the toolbar (and choosing a menu item, or closing the menu by clicking the button again), hotkeys still won’t work

I am not familiar with the issues with other keyboard layouts, but it seems plausible that going back to the old method will introduce a shitstorm of user complaints if the keyboard shortcuts suddenly stop working altogether. So, even if the hacks to the current implementation are not exactly elegant, I would imagine it would prevent some annoyances.


#15

well i think in the end it probably doesn’t matter if we break dijit-native keyboard stuff any further or not, tt-rss broke it left and right for years with global hotkeys and nobody complained. :shrug:

very nice, that’s exactly what I meant. can you resubmit with it included so we would just skip the awkward inline mixins altogether?

i don’t think there’s going to be a shitstorm either way because in my opinion a minority of users is even aware of hotkeys. a subset of those (Germans and others (?) with QWERTZ layout) had them broken for years and nobody complained (much).

anyway, my main problem with this PR was adding inline methods to every toolbar, but with custom toolbar class that wouldn’t be an issue, although i’d ask @suraia to check this:


#16

The new approach will not work with keyboards where the letter on the key does not necessarily correspond to the generated character. For example, pressing “a” on a Cyrillic or Chinese keyboard might generate another character. Everything else should work as is.


#17

Will do. :slight_smile: Will probably need a day or two to finish everything up.

This is actually by design because buttons stay focused after closing them (so you can open them again using Space or Enter). I kept this behavior as this is also how other GUIs usually work. I can try to change this, though, if you prefer.


#18

Ah - I’m OK if the only hotkeys I use are a, m and any digit then.

Gotcha…


#19

This is just my take on it, but I don’t think any focus other than textarea/input fields justify breaking hotkeys, especially when considering the navigation hotkeys (‘g a’, ‘g s’ etc.). I really think consistency here should have priority. Apart from that, I don’t think anyone clicks on a button and then uses the arrow keys to go through a pop-up menu instigated by that button.

So, I would indeed advocate that tt-rss hotkeys should prevail even after those toolbar buttons have gained focus.


#20

this wouldn’t work with basic html buttons, i think, so we might as well override this for dijit controls

then again i don’t think tt-rss has any hotkeys with space, enter or tab so maybe allow specifically those? not sure about this tbh