Possible contribution for better accessibility features

[in0ni]

Hello @dimsemenov,

Our organization is interested in using this library as a base for our gallery components. We must meet accessibility standards, and are looking to follow the following guideline: https://www.w3.org/WAI/ARIA/apg/patterns/carousel/

I have already started making modification to do very simple things like fix tags, add attributes here:
https://github.com/in0ni/PhotoSwipe/tree/aria-pattern

But before continuing, I'd like to know how open are you to merging these changes and having a little discussion regarding other items to ensure accessibility. I also have checked out the dynamic caption plugin, and a very minor change should suffice to get it working according to this pattern.

Please let me know. We really would like to contribute, and do our best to ensure this library meets high accessibility guidelines without changing any functionality.

Thanks in advance

[dimsemenov]

Hi, sure, If you have reasonable changes - I'd gladly accept the PR.

But note that PhotoSwipe is not a carousel, it's designed as an enhancement to a simple link to an image (and not as a standalone gallery).

I'm not sure if adding aria-live regions makes sense since it duplicates the functionality of a simple image grid (captions can be placed in
alt or aria-describedby).

[in0ni]

Hey, thanks for getting back to me.

I think that the functionality is identical to that of the carousel. Just focus on the feature from the div.pswp_scroll-wrap element down the tree: you have a single image displayed at a time, you can switch back/fourth using buttons/keys. There is absolutely no difference in functionality, and javascript is not an issue as assistive technologies work with javascript.

Regarding the aria-live the spec states: "indicates that an element will be updated, and describes the types of updates the user agents, assistive technologies, and user can expect from the live region." Adding the attribute does not affect functionality, rather will ensure accessibility if we follow the pattern.

Here is exactly what we are looking to do:

1. Follow the pattern, which from my branch is mostly implemented. For the most part it's simply adding attributes, take a look at the codepen on this page for details.
2. We would like to also make minor changes to the dynamic caption plugin, so far the only change is ensuring it's parent is the 'slide', which has a group role, and is labeled as a slide.
3. There is one change that is worthy of discussion and that is the method of showing/hiding the slides.

Showing/Hiding Slides
The pattern page has a rather good explanation regarding the requirements, one of which is:

"For instance, the screen reader experience can be confusing and disorienting if slides that are not visible on screen are incorrectly hidden, e.g., displayed off-screen."

This is where I some discussion is required. Currently there is no animation between slides, and the current approach is to have the elements off-screen. I assume this is done to implement animations for slide switching later? Here is what I would like to propose:

• I don't think there is an issue keeping the method as is, but simply ensuring a display: none css property is set to those that are off-screen, and display: block when they are on-screen.
• If you are looking to implement animations in the future, we keep a 'no animation' option, which will keep this behavior as-is.
• When adding animations, then prev/next slides most-likely need to be display: block for the animation -- and this is fine.

The idea is to keep the default method of changing "slides" as an accessible transition (just show hide, perhaps fade in/out is fine). If you want to provide animations in the future, no problem just can note that they might be problematic for screen readers.

What do you think?

[dimsemenov]

There is one change that is worthy of discussion and that is the method of showing/hiding the slides.

You cannot just display:none nearby slides, they exist for one purpose - to pre-render them that ensure a smooth transition. You'd need to hide them via aria attribute and make sure that they cannot be focused.

We would like to also make minor changes to the dynamic caption plugin, so far the only change is ensuring it's parent is the 'slide', which has a group role, and is labeled as a slide.

It may be quite difficult structurally. You'd need to either make pswp__item your root for the slide, or use aria-labelledby to connect the caption.

And as I mentioned before, PhotoSwipe is designed as an enhancement. A simple list of links will be much easier to navigate for users with various assistive technologies on. The guideline that you linked assumes that a "carousel" is the only way to view the content.

[in0ni]

Hey @dimsemenov, a bit of a late reply as I've been waiting feedback from our team.

We would like to review and get back to you, though I am off next week. The following week I will get back to you so we can wrap this up and see if you are OK with us moving forward. Have a great weekend!

[in0ni]

@dimsemenov,

Sorry for the delay, but to finalize things so we can go ahead and move forward we would like to propose the following to start.

1. Showing/Hiding slides

• We will use aria-hidden as you suggest and toggle the state according to visible slide

3. Captions

• It'd like to give it a shot to see if we can get away without using aria-labeledby -- but if it's problematic, we'll just use it.

5. Use of pattern

• We are not looking to make changes to the base functionality or have any perceivable changes
• These are minor changes -- it's simply updating/changing attributes to aid accessibility
• This is a recommended standard approach by ARIA. The functionality of PhotoSwipe's Lightbox is exactly the same no matter the name of the pattern. Once the image is clicked and it goes into Lightbox mode, you can navigate backward/forward between images, that is what the pattern is covering.
• Lastly, the enhancement your module provides is great, and is not a limitation for assistive technologies. A list of links isn't necessarily easier to navigate.

If you are OK with this, we would like to proceed and provide an MR for your review. Please let me know at your earliest convenience and will get that MR out soon.

[dimsemenov]

I'm ok with those changes, feel free to submit. Thanks!

[in0ni]

@dimsemenov,

Thanks for merging those changes. We have found an important issue that we want to tackle... but the truth is I think it might be best for you to handle this, or if you can guide me with some details I could do it as well (as you know the code structure/functionality best).

The issue is simple: when in lightbox mode, and a user tabs between buttons with the keyboard -- tabbing should cycle between only the buttons in the modal window. Currently it tabs out to items outside of this.

It appears the only way to do this is with Javascript by doing the following:

• Get all elements that are 'tabbable' (buttons/anchors)
• Identify the first/last one
• Attach event to the last one, so that when tab is pressed, it focuses the first one.

Or do you see another approach to solve this?

[dimsemenov]

Not sure what do you mean, as the PhotoSwipe already does this, you can debug it at https://github.com/dimsemenov/PhotoSwipe/blob/master/src/js/keyboard.js#L130