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.
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.
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.
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.
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.
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
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.
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.
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:
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.
Will do. 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.
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.
