carbon-for-ibm-dotcom
carbon-for-ibm-dotcom copied to clipboard
carbon-design-system
fix(lightbox-carousel): accessibility & QA improvements to lightbox carousels
Related Ticket(s)
Closes: #8911 #8913 #8915
Also related: #8912 #8914
Description
This PR makes several accessibility improvements to the DDSCarousel and DDSLightboxMediaViewer components, as well as the lightbox carousel story
#8911 Update Lightbox to use semantic headings ✅
Lightbox titles now render title text within <h2 style="all: inherit;"> tags. This gives them semantic h2 headers but does not bring along the user agent/carbon styles for the element.
#8912 Add label for modal dialog ✅
While elements with [role="dialog"] are required to have a label for WCAG, that label should be relevant to the content within the dialog. Because the component cannot know what content (or even what language) is in it, all we can do is make space for content entry. This already exists within the component in the form of an aria-labelledby attribute pointing to a div that accepts slotted content. This PR adds a visually hidden modal title to the lightbox carousel story
#8913 Announce carousel navigation to screen reader ✅
The carousel's renderStatus() method now renders a visually-hidden span with aria-live="polite" and uses a new formatAnnouncement property function to format that announcement into a string that accounts for single slides or slide groups. This property is written adjacent to the formatStatus property that allows localizing the pagination information.
#8914 Create new landmark region for carousels ✅
~No changes were made for this ticket.~
~8914 suggests, "to provide more context to the screen reader user, the carousel which includes the slides and the Carousel controls could be given in a labelled landmark region." While this would be good, wouldn't adding a landmark region require a label that describes what is in the carousel, not just that it is a carousel? It would not be difficult to add a fallback label if this is not the case.~
The carousel now has an additional <div> wrapping the scrolling contents and the navigation. This div has role="region" and an aria-labelledby attribute that points to a new div with a <slot name="title"><span class="bx--visually-hidden">Carousel</span></slot>. This allows authors to provide a title relevant to the carousel's contents with the generic fallback label of "carousel"
#8915 Update carousel's next/previous button labels ✅
The carousel allows authors to specify the labels of the previous & next buttons, but we also provide a default. Previously this value was "next page", but the word page can be confusing. We now provide a default fallback that looks at the carousel's pageSize and returns either "next slide" or "next slide group" if more than one slide is visible.
[No Ticket] Refactor carousel item's interactivity based on inert
When the [inert] attribute was added to the carousel, it was added in a way that wasn't implemented fully in safari/firefox, and didn't account for carousels with multiple visible values well. That's been updated to now use an intersection observer, and it will mark carousel items more less than 75% visible as inert and aria-hidden.
Changelog
Changed
- Accessibility improvements to lightbox media viewer component
- Accessibility improvements to lightbox carousel story
- Accessibility improvements to carousel component
Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/9149/index.html
Built with commit: 54f3957f7a4ee50edeca8222e79fdcdfcd1390ca
Deploy preview created for package NextJS Test Application:
https://ibmdotcom-nextjs-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9149/index.html
Built with commit: b6b9d852c54c6a0b2a4ea64e2695768ca4971651
Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9149/index.html
Built with commit: b6b9d852c54c6a0b2a4ea64e2695768ca4971651
Deploy preview created for package Web Components (React wrapper):
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9149/index.html
Built with commit: 54f3957f7a4ee50edeca8222e79fdcdfcd1390ca
Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9149/index.html
Built with commit: 54f3957f7a4ee50edeca8222e79fdcdfcd1390ca
@proeung @andy-blum We had a review QA about the a11y issues with this component. For users who rely on screen readers, the labels for previous and next are not really an issue. Simply having previous/next is enough, and in fact next slide group is more confusing. The main point is the visually hidden element with aria-live. The most informative would be an announcement to the effect of 'Slide x of y with n items'. It looks like this announcement is already part of the PR, so QA is going to review the deploy preview and share any comments with us.
One thing to note is the visually hidden element will need to exist for sm, md, and >=lg breakpoints, as the number of slides and items displayed will be different for all 3.
@kennylam @Gopi916 The most recent commit updates the aria-live announcement logic. It utilizes the intersection observer to count the number of visible items and update the announcement text after a brief 100ms delay to ensure the slide transition is fully complete.
If the screen reader issue experience in firefox is a render lifecycle race condition, this fix should hopefully also address that.
@kennylam Looks like this PR is at a good place, but could probably benefit from a code review from your team. Can you take a look and let us know if these changes can be marked as "Ready to merge"? Thanks!
@Gopi916 I've updated the PR to wrap carousels in <div role="region" aria-labeledby="carousel-title"> and added a slot for authors to add carousel title text. Additionally, I think the iOS voiceover text should be fixed. I updated the delay from 100ms to 400ms, and it's now using the design token from @carbon/motion.
Updated PR summary to indicate issues that have been closed by @Gopi916 and to update the solution description for #8914
CDN generated: https://1.www.s81c.com/common/carbon-for-ibm-dotcom/tag/v1/deploy-preview/masthead.min.js
Built with commit: 54f3957f7a4ee50edeca8222e79fdcdfcd1390ca
Per conversation last week with @Rasubra8 and @kennylam this has passed accessibility review.
@oliviaflory would you mind taking a look to make sure everything is still aligned with the designs?
@andy-blum the designs look good! The one issue I found is I cannot scroll within the lightbox media viewer with carousel. The carousel buttons are partially hidden or I cannot access them at all if the browser is a bit shorter, and on my phone. I can only access the controls if I use the keyboard to tab.
I tried on Chrome, Firefox and Safari on my iPhone 12 https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9149/iframe.html?id=components-lightbox-media-viewer--with-carousel
@andy-blum thanks for fixing the carousel controls! A few additional comments:
1. Unexpected image/video scaling
The image or video is scaling when the browser has a shorter height, see exaggerated examples below. @jwitkowski79 and I were expecting the image to stay the same width as the default lightbox media viewer.
Carousel
Default
2. No fade on mobile
On my iPhone 12 Pro in Safari and Chrome I don't see the gradient fade that appears in the desktop experience. This is helpful for users to know there is more content below. Right now you only see tiny specs of the additional copy. Let me know if this isn't a part of the Default lightbox and we can open a feature enhancement!
Default with gradient fade
3. Difficult to read text with fade
Lastly, it's difficult to read the last line of text with the gradient fade. We might want to account for extra padding at the bottom so the user can scroll the text enough past the gradient to have the text fully visible.
Just merged in the carbon-web-components addition and had to do a bunch of merge conflicts. There may be some regressions at this point.
@ariellalgilmore Thanks for the review! Is this PR ready to be merged or do we need your accessibility team to do another review before we add the "Ready to merge" label?
cc: @kennylam
@andy-blum @proeung this looks good to me!! the accessibility updates look great. The only thing I am noticing is that it seems like the gradient is not sitting in the right location for when the user scrolls in lightbox. These are the specs it should follow:
Thanks @ariellalgilmore !
I've adjusted things around the gradient. Due to the change we made affecing the scoll area at desktop, where the whole body of the lightbox content scrolls together (image / video and title / description), I also needed to change the gradient at desktop such that it spans the whole width of the scrolling area. This is an implementation limitation, I tried at some length to affect only the description, to no avail. In the end, to me this fits the intent of the gratient which by my interpretation is to signal additional contents beyond the visibile area of the scroll container. Curious to get your thoughts!
I believe it was design intention to not have the media scroll in the larger breakpoints, so wondering what @oliviaflory and @RichKummer think. Maybe some background on why this was changed for the Accessibility fixes would be helpful...
We arrived at scrolling the media together with the description with consideration for video. When a video is used in the lightbox carousel, the user needs to be able to scroll to the media controls once playback is initiated. Otherwise, if the media was clipped, the user wouldn't be able to access any of the media controls. Taken together with @oliviaflory 's request to avoid scaling the video to fit the available height (https://github.com/carbon-design-system/carbon-for-ibm-dotcom/pull/9149#issuecomment-1279434723), the logical conclusion was to scroll the media and description together, while pinning the carousel controls to the bottom of the lightbox so they remain accessible regardless of available height.
Deploy preview created for package "Carbon Web Components":
https://carbon-web-components.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9149/index.html
Built with commit: 54f3957f7a4ee50edeca8222e79fdcdfcd1390ca
Friendly bump for @ariellalgilmore / @oliviaflory / @RichKummer . Are we OK with the implementation here as is, or is more iteration necessary? Thanks.
just noticing now the tests for react/web components are failing as well @m4olivei
just noticing now the tests for react/web components are failing as well @m4olivei
Thanks for pointing that out. I'll work on that.
Great thanks @oliviaflory ! I also fixed the test failures we were seeing. So as far as I know (still new to the process), we're ready to merge?
@m4olivei hmm it looks like the Test app deploy previews are failing, is that normal @ariellalgilmore ?
i think @kennylam mentioned he was looking at the test app failures, but shouldn't be an issue with this PR