toolbox-team / reddit-moderator-toolbox

Moderator toolbox for reddit extension development.
https://www.reddit.com/r/toolbox
Apache License 2.0
114 stars 38 forks source link

Feature Request: Keyboard navigation #88

Open CelineHagbard opened 7 years ago

CelineHagbard commented 7 years ago

Feature Request:

I'm in the process of adding keyboard support for common mod actions in RES on a per-comment/post basis in the modqueue, such as [remove], [approve], but also [context]. For the context option, I've bound the key-press to fire a click event on the context link, so if the user has toolbox's context popup enabled, a key-press opens the popup.

My issue is that there appears to be no way to close the context popup from the keyboard. Is it possible to add an event listener to close the popup on Esc keyDown? I don't mind making the changes, but I'd like to know how it should be handled. Should an Esc keyDown close ALL open popups (e.g. if a context and usernotes popup is up), the most recently created popup, or something else?

I'm thinking it makes the most sense to add a 'keyDown' listener to $('body') which then fires a 'click' event on every link matching $('.mod-toolbox .tb-popup .tb-popup-header .buttons a.close'). So pressing Esc would close out every TBui popup.

Reddit Thread:

https://www.reddit.com/r/toolbox/comments/6v20yg/feature_request_close_popup_with_esc_key/

creesch commented 7 years ago

I have changed the title to reflect what actually needs to be done. In more detail:

We don't currently have a framework for keyboard navigation in toolbox. I'd rather not add it on a per case basis as that will lead to code all over the place.

What needs to be done is the following:

After the above has been done we can look at implementing your request.

To complicate things, this will have to wait as this will largely depend on the DOM context. Reddit is in the process of a technical redesign where the frontend stack will be react based (similar to new modmail and the new profiles) which will probably change the way that is the best way to tackle this.

CelineHagbard commented 7 years ago

Okay. That all makes a ton of sense. I think you're right in that it doesn't make a whole lot of sense to implement a keyboard nav module or utility at this point if the frontend change is coming soon. When that does happen, I'd be interested in working on it, though. Being able to use removal reasons, mod macros, usernotes, etc. from the keyboard would be great.

As a temporary solution, what would you think about setting the focus to the close button when the context popup is created, so the user could press enter to close the popup? Currently, when the context link is clicked by mouse, the focus remains on the context link itself, so I don't think this should break anything. (When I programmatically fire the click event with RES, the focus changes to body). I think this would be as simple as adding

$contextPopup.find('tb-popup-header .buttons a.close').focus();

after line 252 of queuetools.js.

creesch commented 7 years ago

When that does happen, I'd be interested in working on it, though. Being able to use removal reasons, mod macros, usernotes, etc. from the keyboard would be great.

That would be great! I'll let you know when we know more in that regard and are also allowed to tell more about it.

As far as the temp solution goes, that would work. Though if at all possible (not sure it is) see if it works a bit earlier in the UI utility, Somewhere around line 287 in tbui.js.

So $popup.find('tb-popup-header .buttons a.close').focus();

I am not sure if it is possible to do it at that point (as it isn't technically part of the DOM there), but it would effectively make it work for all instances where toolbox opens a popup.

CelineHagbard commented 7 years ago

You were right, it wasn't working because it wasn't part of the DOM yet. My solution wasn't working either for what I suspect was the same reason, so I added this to line 287 in tbui.js instead:

    // Give focus to close button so popup can be closed with `Enter`
    window.setTimeout( function() {
        $('.mod-toolbox .tb-popup .tb-popup-header .buttons a.close').last().focus();
    }, 0);

The setTimeout was necessary to wait for the popup to be added to the DOM. This selects and focuses the last close button to be created, and works for the other popups like history and notes. It works on Chrome and Nightly. Do you guys run integration tests or what are the next steps I'd need to do?

It doesn't return focus to the previously created popup, but I can live with that until we can work on the full keyboard nav when the new frontend comes out. I think that would be as simple as running this snippet of code again from the close button event handler, but that would need to be done in each popup type. I don't think it makes sense to do at this point.

eritbh commented 7 years ago

If this issue is for general keynav stuff, should we merge #267 into this for the future?

creesch commented 7 years ago

Possibly, though I don't really think so. Forms submitting on enter is not something specific to keyboard shortcuts but rather something they should do regardless as that is what people expect (with the exception of textareas).

It is definitely related though but still a separate issue as far as I am concerned.