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

Make clickable elements reachable by keyboard users #7822

Closed
wants to merge 2 commits into from

Conversation

DuaelFr
Copy link

@DuaelFr DuaelFr commented Oct 1, 2020

As seen in #6090 and more specifically in #5992, #5308 and older/closed issues, a lot of elements of Jitsi are clickable but never reachable using keyboard navigation. This PR aims to improve this by adding the role and tabindex elements each time an HTML element that is not reachable on its own (buttons, links) contains an onClick attribute or converting them to a button when it's not too difficult.

I can see it could be factorized somehow but I'm not as confident with React as I could be with another language so, please, if this is an issue, give me some tips to improve this PR so it can be commited soon.

@DuaelFr
Copy link
Author

DuaelFr commented Oct 1, 2020

This PR should also fix #7653

@DuaelFr
Copy link
Author

DuaelFr commented Oct 1, 2020

This PR cover some points reported in #7537

@saghul
Copy link
Member

saghul commented Oct 1, 2020

@muscat1 can you please review this?

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@DuaelFr
Copy link
Author

DuaelFr commented Oct 1, 2020

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

I did that a few hours ago :)
(Yes, I'm talking to a bot)

@saghul
Copy link
Member

saghul commented Oct 1, 2020

Sorry about that, it's just a robot :-)

@muscat1
Copy link
Member

muscat1 commented Oct 2, 2020

LGTM! Just a few observations after some manual testing:

First of all, this seems to make clickable elements visible when accessing them via keyboard, however they are not necessarily functional since most of these do not have an onKeyDown listener set up. I'm not entirely sure if this is part of this PR's scope, but I suspect it might be. We've (relatively recently) merged something similar WRTO tooblar buttons here: #6070 If this is not the case, feel free to disregard this observation.

Finally, and this is purely a technical aspect, from the way things are currently implemented, dialogs close when hitting the enter key, which might come in conflict with the point I've made above since trying to use dialog buttons via keyboard will just close the dialog. Not sure how to proceed with this one, maybe remove the enter key dialog behaviour entirely and rely on navigation to the close button and triggering that. WDYT @saghul ?

@saghul
Copy link
Member

saghul commented Oct 2, 2020

Not sure how to proceed with this one, maybe remove the enter key dialog behaviour entirely and rely on navigation to the close button and triggering that.

This makes sense to me.

@DuaelFr
Copy link
Author

DuaelFr commented Oct 8, 2020

Fixed conflict.

@muscat1 @saghul Do you expect me to remove this enter key behavior in this PR?

@muscat1
Copy link
Member

muscat1 commented Oct 12, 2020

@DuaelFr I was thinking that maybe we should be adding a similar behavior for the elements that are missing it. Imo, it seems a bit misleading if a user can navigate to an element via keyboard but can't actually interact with said element. Wdyt?

@DuaelFr
Copy link
Author

DuaelFr commented Oct 12, 2020

@muscat1 I'm sorry but I don't understand (maybe that's because I'm not a native english speaker). I thought I added this behavior to all elements that had an onClick attribute. That was my intent. Have I missed some of them?

@stale
Copy link

stale bot commented Jan 10, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issue won't be fixed label Jan 10, 2021
@muscat1
Copy link
Member

muscat1 commented Jan 11, 2021

Hey, for some reason this PR fell through my mind's cracks and I completely lost track of it :') If this is still of interest, I remember that when I was testing it, I was indeed able to navigate to all buttons (the point of the PR) but I was unable to interact with them through the keyboard. (I couldn't "click" them through a key) If we want the interface to be accessible, I think we should be able to do that, don't you?

It's entirely possible that I tested this wrong so please let me know if that's the case.

@saghul tagging you as more of a reminder as well, because you might also disagree with my opinion :D

@stale stale bot removed the wontfix Issue won't be fixed label Jan 11, 2021
@DuaelFr
Copy link
Author

DuaelFr commented Jan 12, 2021

@muscat1 would you mind trying to give me some steps to reproduce the issue, please? I'd like to understand how you proceeded so I can try to fix the problem and update the PR.

@muscat1
Copy link
Member

muscat1 commented Jan 12, 2021

Again, I might be in the wrong here, but they way I reproduced this was I cloned your repo and ran the project with your changes via make dev, I then joined a conference and started trying to access various conference features by only using the keyboard.

I'll attach some screenshots below with some buttons that were reachable, but not actionable via keyboard only. (Please tell me if this was not your initial intent with the PR so I don't overcomplicate things)

In the prejoin screen you could reach and trigger the video and audio mute/unmute, but several other functions were only reachable:

Screenshot 2021-01-12 at 11 49 12

After joining the conference, I can only reach, but not trigger the 'invite more people' button:

Screenshot 2021-01-12 at 11 56 57

While in the conference, I can reach and trigger the display of the overflow menu from the toolbar, but then none of the buttons are actionable, only reachable:

Screenshot 2021-01-12 at 11 57 24

And finally, there's the entire dialog discussion where, if you open any dialog and try to trigger a button with the Enter key, it'll just close the dialog.

Feel free to correct me on any wrong assumptions and also share your thoughts regarding my 'investigation' as I might've gone overboard :)

@DuaelFr
Copy link
Author

DuaelFr commented Jan 12, 2021

Thanks a lot for your explanations.
You are right, I expected to make those buttons reachable and actionable.
Putting the dialog window problem aside for now, I'll try to see if I can understand why those buttons are not actionable (I'm not a React dev so I'll do my best but if someone has some insight, they'll be very welcome).

@muscat1
Copy link
Member

muscat1 commented Jan 12, 2021

I've mentioned it before but some time ago we merged a PR that achieves that for the toolbar buttons, maybe that can provide some inspiration. You can find it here: #6070

@MarcoZehe
Copy link
Contributor

Is this still going to be worked on? Or was this fixed by #8423 by chance? Is there any instance newer than https://meet.jit.si where one could test current Master without having to install all stuff oneself?

@DuaelFr
Copy link
Author

DuaelFr commented Mar 2, 2021

I wish I had more free time to work on this but for now I have to focus on paid work. This is on top of my contrib todo list.
I've seen some changes in the master branch so I'll need to rebase my commits and handle the conflicts so it might need some extra work :/

@damencho
Copy link
Member

damencho commented Mar 2, 2021

Is this still going to be worked on? Or was this fixed by #8423 by chance? Is there any instance newer than https://meet.jit.si where one could test current Master without having to install all stuff oneself?

All debian packages while being tested are first installed on alpha.jitsi.net, so latest from unstable cal always be tested there.

@DuaelFr
Copy link
Author

DuaelFr commented Mar 2, 2021

I did a really quick test run on alpha.jitsi.net and I can confirm that some actions are still unreachable by keyboard (but it looks better already, congrats!)

@stale
Copy link

stale bot commented Jun 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issue won't be fixed label Jun 11, 2021
@DuaelFr
Copy link
Author

DuaelFr commented Jun 11, 2021

I'm so sorry I cannot spend much time on this one. As the app has evolved since my first commit, there is work to do to test keyboard navigation on the current version and only fix what needs to be fixed.
Please don't close it stale bot!

@stale stale bot removed the wontfix Issue won't be fixed label Jun 11, 2021
@muscat1
Copy link
Member

muscat1 commented Jun 11, 2021

Hey there @DuaelFr ! We have recently (yesterday) merged this humongous PR (#8921) that helps in bringing us in line with accessibility standards. If you see that this PR is still needed, feel free to keep it open, otherwise close it at your convenience.

@stale
Copy link

stale bot commented Jan 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issue won't be fixed label Jan 8, 2022
@stale stale bot closed this Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix Issue won't be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants