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

Accessibility fixes for header and global elements #10310

Merged
merged 12 commits into from
Jul 24, 2018

Conversation

jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Jul 20, 2018

Fix #9994 and fix #10008 🎉

  • Settings icon / avatar: fix opening on enter
  • Contacts: fix opening on enter
  • Notification icon: fix keyboard tab feedback and opening on enter: Fix header icon accessibility notifications#146
  • Fix Upload in +Menu not launching when using keyboard

cc @nextcloud/accessibility :)

@jancborchardt jancborchardt added this to the Nextcloud 14 milestone Jul 20, 2018
@jancborchardt jancborchardt changed the title [Wor k Accessibility fixes for header and global elements [Work in progress] Accessibility fixes for header and global elements Jul 20, 2018
@jancborchardt
Copy link
Member Author

Could use some help with JS here on how to do the trigger using enter so that it works for all .menutoggle elements and we don’t need to change the markup. @nextcloud/javascript or what do you think?

Similar for the Unable to open file or folder with keyboard #10008 issue where pressing enter on a folder/file opens the sidebar instead of opening the folder/file itself.

jancborchardt and others added 8 commits July 20, 2018 23:19
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt jancborchardt force-pushed the accessibility-header branch from ad7181d to b7bf2f7 Compare July 20, 2018 21:22
@jancborchardt jancborchardt changed the title [Work in progress] Accessibility fixes for header and global elements Accessibility fixes for header and global elements Jul 20, 2018
@jancborchardt jancborchardt added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 20, 2018
@jancborchardt
Copy link
Member Author

This is now ready to review, together with the related Notifications pull request at nextcloud/notifications#146

Please check @nextcloud/accessibility @nextcloud/designers :)

Copy link
Contributor

@tyrylu tyrylu left a comment

Choose a reason for hiding this comment

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

From the point of view of the markup and script changes, it looks alright and useful, so thank you.


// Trigger upload action also with keyboard navigation on enter
this.$el.find('[for="file_upload_start"]').on('keypress', function() {
$('#file_upload_start').trigger('click');
Copy link
Contributor

@kevgathuku kevgathuku Jul 21, 2018

Choose a reason for hiding this comment

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

@jancborchardt Right now this is triggered for every keypress.
Ideally, only the Enter and Spacebar keys should activate the functionality.
A potential fix would be something like this:

this.$el.find('[for="file_upload_start"]').on("keypress", function(event) {
  if (event.key === " " || event.key === "Enter") {
    $("#file_upload_start").trigger("click");
  }
});

Copy link
Member

Choose a reason for hiding this comment

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

@jancborchardt yup, we forgot this one! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevgathuku ah right! Do you want to add a commit to the pull request? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@jancborchardt sure. I'll add it

@skjnldsv
Copy link
Member

skjnldsv commented Jul 21, 2018

Fix #9994 too?
Arf, was already there :)

this.$el.find('[for="file_upload_start"]').on('keypress', function() {
$('#file_upload_start').trigger('click');
this.$el.find('[for="file_upload_start"]').on('keyup', function(event) {
if (event.key === " " || event.key === "Enter") {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your commit! 🎉

Why not use the codeKey?
Is there a special reason for allowing the " " ? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @skjnldsv
Thanks 👍

Why not use the codeKey?

The keyCode is deprecated and event.key is preferred instead.

Is there a special reason for allowing the " " ? :)

For this instance, the behavior is more like a button, which is usually activated using either the Spacebar (" ") or Enter keys. I'm thinking it would be better to stick to this convention.

Let me know whether this makes sense 😄

Copy link
Member

Choose a reason for hiding this comment

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

Nice! Thanks for the tip, I did not know it was deprecated :)

Can you also move the edits we dit Jan and I from keyCode to key?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure! I'll make the edits.

core/js/js.js Outdated
// prevent the link event (append anchor to URL)
event.preventDefault();

// allow enter key as a trigger
if (event.keyCode && event.keyCode !== 13) {
Copy link
Member

Choose a reason for hiding this comment

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

We should key instead

core/js/js.js Outdated
$(document).on('mouseup.closemenus', function(event) {

// allow enter as a trigger
// if (event.keyCode && event.keyCode !== 13) {
Copy link
Member

Choose a reason for hiding this comment

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

We should key instead

@kevgathuku kevgathuku force-pushed the accessibility-header branch from 7df461d to d2e9e22 Compare July 23, 2018 08:21
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 23, 2018
@MorrisJobke
Copy link
Member

CI is here: https://drone.nextcloud.com/nextcloud/server/9129

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jul 23, 2018
@MorrisJobke MorrisJobke mentioned this pull request Jul 24, 2018
21 tasks
jancborchardt and others added 4 commits July 24, 2018 18:58
Before, the file or folder was opened when clicking on the name span,
but not when clicking on the link that contains the name; clicking on
the link highlighted the file and opened the sidebar, just like clicking
on the file size or date. Now clicking on the link opens the file or
folder, so the unit tests that tested clicks on the link were changed to
test clicking on the file size instead.

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Kevin Ndung'u <kevgathuku@gmail.com>
Signed-off-by: Kevin Ndung'u <kevgathuku@gmail.com>
@danxuliu danxuliu force-pushed the accessibility-header branch from d2e9e22 to fe25092 Compare July 24, 2018 17:03
@danxuliu
Copy link
Member

I have amended the offending commit and fixed the JavaScript unit tests ;-)

@MorrisJobke MorrisJobke merged commit a96137e into master Jul 24, 2018
@MorrisJobke MorrisJobke deleted the accessibility-header branch July 24, 2018 22:07
@jancborchardt
Copy link
Member Author

Thanks @skjnldsv @kevgathuku @danxuliu for the help! :) This is another great fix for better accessibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug design Design, UI, UX, etc. feature: accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to open file or folder with keyboard Keyboard accessibility
6 participants