Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat/add keyboard control #431

Closed
wants to merge 14 commits into from

Conversation

xindixu
Copy link

@xindixu xindixu commented May 22, 2020

Changes

closes scttcper/ngx-emoji-mart#313
Added keyboard control:

Category anchors:

ezgif com-video-to-gif (1)

  • When focusing on any emoji, Tab to change focus to the first category anchor
  • When a category is selected, Arrow Down to go to the first item in the category
  • This also means that when focusing on any emoji, Tab can no longer used to go to the next emoji
  • Same applies to search
    ezgif com-video-to-gif (2)

Emoji:

ezgif com-video-to-gif

  • Arrow Up, Arrow Down, Arrow Left, Arrow Right to go to emojis on top/down/left/right to the current emoji
  • Able to jump to next/previous category if at the last line/first row

Style:

  • emoji focus style now should look the same as emoji on hover style

Markup:

  • Change emoji layout to table/cell with a11y roles

References:

@rugk
Copy link

rugk commented May 24, 2020

Awesome PR! Thanks! 🎉
Really glad, I want to see this merged.

This PR is largely inspired by #348. Note that some of the features mentioned are not implemented or implemented differently.

Uhm, care to say which exactly?

The far I see these changes in your very nice gifs, I am very satisfied. The only thing I possibly miss is the focus style:

The focus style should be improved, so you can see what (also an accessibility improvement) is selected.

@rugk
Copy link

rugk commented May 24, 2020

Does this possibly also fix scttcper/ngx-emoji-mart#313?

Also note you can (automatically) let issues close when a PR is merged by adding some "magic" text to your PR body.

@xindixu
Copy link
Author

xindixu commented May 24, 2020

@rugk
This PR differs from #348 in these ways:

  • When you are in the input field an 🔽 (arrow down) should bring you (the focus) to the list of emojis, i.e. actually to the first emoji in that list.
  • The focus style should be improved, so you can see what (also an accessibility improvement) is selected.
  • use the keyboard keys to navigate the list of emojis in a grid-style (currently: if you use up/down arrows, it scrolls the scrollbar)
  • Just press space or enter as usual to select an emoji.
  • When you are at the top row of your emojis. 🔼 (arrow up) should bring you (the focus) to the search bar.
  • Tab navigation should obviously still be possible.
  • Tab can no longer used to select emojis. It will bring focus to the first category anchor. Users can use Tab to tab through category anchors and search bar. Users can also use arrow keys to select emojis

@xindixu
Copy link
Author

xindixu commented May 24, 2020

The focus style should be improved, so you can see what (also an accessibility improvement) is selected.

@rugk I am not sure the best way to improve this. I just used the same mouse hover style it has currently. I'm kinda reluctant to add in new styles since I think designers in massive team could make better decision than me :).
Note that developers using this library can still manually override focus style.

@xindixu
Copy link
Author

xindixu commented May 24, 2020

Does this possibly also fix TypeCtrl/ngx-emoji-mart#313?

Yup, it will. I'll add magic to close this issue. As for #348, since not all of features you mentioned is implemented, should I still close the issue?

@rugk
Copy link

rugk commented May 24, 2020

Tab can no longer used to select emojis. It will bring focus to the first category anchor.

I'm not sure whether this is a good idea, accessibility-wise. As that is the default accessibility feature. But I'm probably the wrong one to ask, whether this is good…

As for #348, since not all of features you mentioned is implemented, should I still close the issue?

I'd say yes, as long as the maintainer(s) do not say anything else.

@rugk
Copy link

rugk commented May 24, 2020

When you are at the top row of your emojis. arrow_up_small (arrow up) should bring you (the focus) to the search bar.

Even if you are at the first of all rows? E.g. this should be possible IMHO (+ user story):

  1. Search for wink
  2. Go down -> Only now notice you want to use a different emoji
  3. Press up again -> "Correct" your search term to upside [down face] e.g.

@xindixu
Copy link
Author

xindixu commented May 24, 2020

When you are at the top row of your emojis. arrow_up_small (arrow up) should bring you (the focus) to the search bar.

Even if you are at the first of all rows? E.g. this should be possible IMHO (+ user story):

I'd say that since emoji grid is separated from the search bar, based on the a11y guideline, Up should not go back to search bar since it is essentially different region. Maybe we could have search bar always in focus and thus users could type in more characters if they need to.
So it will be:

  1. Search for wink
  2. Go down -> Only now notice you want to use a different emoji
  3. Hit backspace and directly type in 'upside'

One benefit I can think of is that users don't have to press up 5 times, say, to just get back to the search bar.

Now I also realized that when a category is selected or a search is triggered, it should automatically select the first emoji, without user pressing down

I'm referring to slack emoji picker. I really like the fact that search bar is always focused so that users could just type in anything even when they are browsing emojis.
In short, the only thing that receive a focus is search bars and category anchors. Emojis are not actually "focused", they are just table cells that are "selected".

@haji-ali
Copy link

haji-ali commented May 24, 2020

This looks great! Thanks a lot for implementing it.

If I may, I would like to offer some feedback on the interface.

I think having the search bar always in focus makes more sense to me since it’s unambiguous. Letters and backspace change the search field, arrows select a particular emoji, Enter selects the emoji.
Selecting categories can be done with Tab and Shift-Tab. Clearly this makes navigating the text field impossible but since the search text is expected to be short I don’t think that should be an issue.

If the search bar is auto-focused when the interface is shown, that would allow selecting an emoji using the keyboard only with minimal keystrokes, which is ideal while writing a message.

@xindixu
Copy link
Author

xindixu commented May 25, 2020

All great ideas. I'm going to implement

  • search bar always in focus and user can type anywhere to search
  • auto select first emoji in the category when a category is selected or search result when search is triggered

@rugk
Copy link

rugk commented May 25, 2020

search bar always in focus and user can type anywhere to search

Does not this conflict with accessibility though? Don't screenreaders always read the thing that is focused?

src/components/picker/nimble-picker.js Outdated Show resolved Hide resolved
Copy link

@rugk rugk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (but I don't know React very much)

@xindixu
Copy link
Author

xindixu commented Jun 15, 2020

Could we consider merging this PR in? @EtienneLem ?

switch (e.keyCode) {
case 13:
switch (e.key) {
case 'Enter':
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because I'm not sure: Currently (with tab focus switches) also space works for selecting emojis. This here only refers to Enter.
IMHO space should also continue to work… 🤔

@rugk
Copy link

rugk commented Jun 26, 2020

I also don't know whether this is the right thing to add in this PR, but I have a use case, where I'd like more shortcuts to trigger something in the picker/with the selected emoji. In my case Ctrl+C should e.g. trigger a copy to clipboard.

As all methods we can use to get the selected/chosen emoji currently are reactive (i.e. as an dev outside the lib you only get the emoji if something happens as as a result of a click or so) this is hard to implement.
A active method that could just return the currently focused emoji could be very helpful for that. (Note this use case barely existed before, because if you click an emoji you get the reaction.)
So e.g. a getCurrentlyFocussedEmoji that likely is used internally in a similar way, may be very useful here.

Or… of course… you just expose the current keypress handler. However, IMHO, that would be less flexible as an API, because you are then – yet again – limited to key presses only.

Edit: Don't know whether that may also return if an emoji is only hovered hmm… maybe it can do both, possibly in two separate functions. 🤔

@xindixu
Copy link
Author

xindixu commented Jul 15, 2020

@rugk Sorry for the late reply. What you mentioned is a pretty cool idea. I think you could create a separate issue and PR on this. :)

@rugk
Copy link

rugk commented Jul 18, 2020

Okay, done: #444

@xindixu xindixu force-pushed the xindi/keyboard-control branch 2 times, most recently from 638c0b6 to 51dd4a9 Compare July 20, 2020 16:25
@Not-Jayden
Copy link

Any plans for this to be merged soon? Seems great.

@animify
Copy link

animify commented Aug 27, 2020

Looking forward to this too, very much needed for full keyboard navigation 💯

@xindixu
Copy link
Author

xindixu commented Sep 1, 2020

If you are interested in adding keyboard support, feel free to check out this release in our repo: https://github.com/pingboard/emoji-mart/releases/tag/v3.0.1

@castroCrea
Copy link

Hey @EtienneLem any idea when you can merge that and release ?
Thanks
And thanks @xindixu and @rugk

@hackel
Copy link

hackel commented Sep 23, 2020

Please consider also including vi-style keyboard navigation (hjkl) in addition to the arrow keys. This would make navigation even more efficient while typing, not having to move one's hands from the home row at all. I don't believe this would conflict with anything.

@AjayKumar-25
Copy link

Any tentative date of merging and releasing this ?

@EtienneLem
Copy link
Member

Sorry for the radio silence on this PR.

There are some important changes that I’m not very sold on, I’m not sure why the layout had to be changed to <table>. I’d be favorable for a more accessibility-based implementation where we can use onFocus and move the focus with keyboard shortcuts.

@xindixu
Copy link
Author

xindixu commented Oct 28, 2020

@EtienneLem The main reason why I changed to table is that: table is a markup more suitable with the aria-role grid.
role=grid implies exisitance of Arrow Up, Arrow Down, Arrow Left, Arrow Right navigations. See more: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Grid_Role

@evang522
Copy link

evang522 commented Jun 2, 2021

I would really love to see this merged in and part of a newer version. I'm planning to use this for a Chrome extension I'm developing, but in my opinion it's really not that user friendly unless keyboard navigation is possible.

@rugk
Copy link

rugk commented Jun 2, 2021

I'm planning to use this for a Chrome extension I'm developing, but in my opinion it's really not that user friendly unless keyboard navigation is possible.

I totally agree, @evang522, and I'm actually affected in the same way as you are affected.
Because I already have a Firefox extension based on this library, which could also be ported/made compatible with Chrome/ium browsers. This is tracked here: rugk/awesome-emoji-picker#54
It was just not on my highest priority so far.

@EtienneLem
Copy link
Member

Thanks a lot for your contribution. v5.0.0 has been released and this doesn’t apply anymore.

The new version has full keyboard control.

@EtienneLem EtienneLem closed this Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Anchors and emojis are not keyboard accessible
10 participants