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

Try: Make gallery keyboardable #11205

Merged
merged 2 commits into from Nov 2, 2018

Conversation

Projects
None yet
6 participants
@jasmussen
Contributor

jasmussen commented Oct 29, 2018

The intent of this PR is to fix #3836, but for now this is just the barebones initial work.

I would like your thoughts on the best approach to take before I optimize too far in one direction. As such, this PR does the bare minimum to get tabbing working for the gallery, and then we can go from there. For example, this PR simply adds a tabIndex="0" and an onKeyDown handler to the image, to make it accessible. It might be a better solution to wrap the image itself in a button, but this might regress editor styles. Perhaps this approach is necessary as an intermediate step towards better solutions?

Your thoughts are most welcome.

gallery

Try: Make gallery keyboardable
The intent of this PR is to fix #3836, but for now this is just the barebones initial work.

I would like your thoughts on the best approach to take before I optimize too far in one direction. As such, this PR does the bare minimum to get tabbing working for the gallery, and then we can go from there. For example, this PR simply adds a `tabIndex="0"` and an `onKeyDown` handler to the image, to make it accessible. It might be a better solution to wrap the image itself in a button, but this might regress editor styles. Perhaps this approach is necessary as an intermediate step towards better solutions?

Your thoughts are most welcome.

@jasmussen jasmussen self-assigned this Oct 29, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Oct 29, 2018

@mtias mtias added the Accessibility label Oct 29, 2018

@mtias mtias added this to the 4.2 milestone Oct 29, 2018

@jasmussen jasmussen requested a review from tofumatt Oct 30, 2018

@@ -102,7 +102,7 @@ class GalleryImage extends Component {
// Disable reason: Image itself is not meant to be
// interactive, but should direct image selection and unfocus caption fields
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/click-events-have-key-events

This comment has been minimized.

@aduth

aduth Oct 30, 2018

Member
Suggested change Beta
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/click-events-have-key-events
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 30, 2018

I'm moving this out of 4.2 for now until it gets properly review a11y wise.

@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 30, 2018

@mtias mtias modified the milestones: WordPress 5.0, 4.3 Nov 1, 2018

@mtias

This comment has been minimized.

Contributor

mtias commented Nov 1, 2018

@youknowriad let's not use 5.0 milestone for PRs as it gets harder to track them. We should always schedule PRs for plugin milestones, which is when they will land first.

@tofumatt

Just tested this locally and it's much better for keyboard accessibility, though it should be noted the actual labelling of these elements for screen readers is still quite bad. See:

screenshot 2018-11-01 at 20 16 28

It might be good to have an aria-label or something on images with no alt attributes… but even then it might be nice to preface it with "image 2 of 3: $alt"

That's fine for a future issue though; this issue is about keyboard accessibility and this is way better than master 👍

@designsimply

This comment has been minimized.

Contributor

designsimply commented Nov 1, 2018

I tried testing try/keyboardable-gallery (870f81d) using Firefox 63.0 on macOS 10.13.6 but when I placed the cursor in the title and tried to tab to the images in the gallery block, they were still skipped. (gif) Note: it's quite possible I've done something wrong when trying to apply the branch, I did git checkout try/keyboardable-gallery followed by npm install && npm run dev.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Nov 1, 2018

@designsimply Odd, it worked for me when I checked it out. From master, to test this you should try:

git checkout master
git pull origin master
git branch -D try/keyboardable-gallery
git fetch origin
git checkout -b try/keyboardable-gallery origin/try/keyboardable-gallery

That should definitely cover it 😄

@designsimply

This comment has been minimized.

Contributor

designsimply commented Nov 1, 2018

Thank you for the tips! Super helpful. I tried them out and tested from Chrome 70.0.3538.77 and also again on Firefox 63.0 on macOS 10.13.6. Turns out! It works in Chrome and not in Firefox. 🙃

@tofumatt

This comment has been minimized.

Member

tofumatt commented Nov 1, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 2, 2018

Based on the past few comments, I'm unsure exactly how to proceed.

Do I merge and create an issue for better labelling? Or was there a Firefox issue?

@tofumatt

This comment has been minimized.

Member

tofumatt commented Nov 2, 2018

I’ll test again today. Any chance you could test in Firefox for more data points here?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 2, 2018

Sure of course, sorry. It's working completely identically for me in Firefox, as to how it is in Chrome.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Nov 2, 2018

What's with the sorry; I'm the Canadian! 😉

I tested again and this is working fine for me in Firefox. I'm gonna go with weird caching or something and say we :shipit: I couldn't reproduce it not working in Firefox 😞

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 2, 2018

Cool. Shipping this as an improvement, and opened #11396 as a future improvement.

@jasmussen jasmussen merged commit fbc093a into master Nov 2, 2018

2 checks passed

codecov/project 48.69% (+0.13%) compared to a2f81fa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/keyboardable-gallery branch Nov 2, 2018

antpb added a commit to antpb/gutenberg that referenced this pull request Nov 5, 2018

Try: Make gallery keyboardable (WordPress#11205)
* Try: Make gallery keyboardable

The intent of this PR is to fix WordPress#3836, but for now this is just the barebones initial work.

I would like your thoughts on the best approach to take before I optimize too far in one direction. As such, this PR does the bare minimum to get tabbing working for the gallery, and then we can go from there. For example, this PR simply adds a `tabIndex="0"` and an `onKeyDown` handler to the image, to make it accessible. It might be a better solution to wrap the image itself in a button, but this might regress editor styles. Perhaps this approach is necessary as an intermediate step towards better solutions?

Your thoughts are most welcome.

* Apply suggestion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment