Lighthouse Accessibility warning

Aduffy commented 11 months ago

Checks

[X] Not a duplicate.
[X] Not a question, feature request, or anything other than a bug report directly related to Splide. Use Discussions for these topics: https://github.com/Splidejs/splide/discussions

Version

V4

Description

Lighthouse test now reporting "Values assigned to role="" are not valid ARIA roles."

Light house report for splide home page

Reproduction Link

https://googlechrome.github.io/lighthouse/viewer/?psiurl=https%3A%2F%2Fsplidejs.com%2F&strategy=mobile&category=performance&category=accessibility&category=best-practices&category=seo&category=pwa&utm_source=lh-chrome-ext

Steps to Reproduce

visit https://splidejs.com/ in Chrome
Rung lighthouse test with chrome extension

Expected Behaviour

no Lighthouse errors

emaia commented 11 months ago

Same here

kristiansp commented 11 months ago

Solution – TL;DR version (full explanation below) I found a (temporary) solution: Use <div>s instead of <ul> and <li> for the splide_list and splide_slide elements, respectively. I would still love to see a proper fix for this, though. Especially since it seems the creator of Splide has taken such care of good accessibility.

Relevant point in Splide code https://github.com/Splidejs/splide/blob/master/src/js/components/Elements/Elements.ts#L216

Full explanation I have been seeing the same, and was confused, because the slides have role="tabpanel" set, which definitely is the correct ARIA role, but I think I have tracked down the problem. After a lot of digging, I've realised that the problem is when the slides are a list of <li> elements within a <ul> (which is how the examples are done in the docs).

Splide sets role="presentation" on the <ul> element, which, like role="none", tells screen readers not to read out the role (which is fine and what we want, as we don't need to announce "Here is a list", or "Here is the group of slides" to screen readers, as the carousel itself has proper ARIA role and description).

The problem is that role="presentation" has a quirk when it's used on HTML elements that have required descendants, in which case these descendants (but not the elements within them) inherit the role of the parent element. So, since <ul> requires <li>s as descendants, and although the <li>s correctly get role="tabpanel" set, this role is suppressed, because they inherit the role="navigation" from the <ul>element. That's what's causing the Lighthouse warning. (I assume the reason the ARIA specs dictates this behaviour is because, if you don't want to read out that something is a list, you shouldn't want to read out that something is a list item either.)

This article explains it:

When an element has required descendants, such as the various <table> elements and children of a <ul> or <ol>, the presentation or none role on the table or list removes the default semantics of the element on which it was applied and their required descendant elements.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/presentation_role

I did a little test now, and using <div>s instead of <ul> and <li> gets a green flag from Lighthouse. Since <div>s don't have any required descendants, the slide <div>s keep their role="tabpanel". (I couldn't find a quick and easy way to test if it works with <ul> without role="navigation" set).

Let me know if it works for you as well, @Aduffy and @emaia.

Aduffy commented 11 months ago

@kristiansp thanks.. Yes divs is the solution for now. Cheers.

nboliver-ventureweb commented 9 months ago

Similar, but slightly different issue here:

I believe this is caused because role="presentation" is being used with aria-orientation="vertical". Other than changing the layout I'm not sure if there's an easy workaround. Anyone have any ideas?

kristiansp commented 9 months ago

@nboliver-ventureweb Is this a Splide carousel used as navigation for another carousel? I suspect that is what causes the issue.

When having a normal pagination (whether dots or textual), the <ul> element gets role="tablist", and all the <li>s get role="presentation", so it would work (role="tablist" supports the aria-orientation attribute). I guess the best solution would be if carousels used as navigation for another carousel got the same aria roles and attributes set.

If you really want to correct this in wait for a fix (there doesn't seem to be so much activity lately), I would assume the easiest way would be to hook onto either the mounted or the navigation:mounted event, fetch the slider in question, and change the role to role="tablist" (I assume) in JS.

I guess the issue there would be that the slides themselves still will have aria-roledescription="slide" set. Which they kind of also are, I guess – they are serving a double function, so I'm not versed enough in a11y to know what the best structure would be. Worst case it's also possible to strip those out.

Btw, I think this only makes sense if you support keyboard navigation. As I've understood it, the point of aria-orientation us to know if you should go previous/next with left/right or up/down arrows. Which definitely is useful information, but only if you can use it for something 😄

nboliver-ventureweb commented 9 months ago

@kristiansp Thanks for the thoughtful comment. We're not using keyboard nav, and changing to tablist seems to fix it.

Nintron27 commented 9 months ago

For anyone else running into this and using Svelte, you don't need to actually use the SplideSlide and SplideTrack elements, you can just put divs in and give them the correct classes from here.

kristiansp commented 9 months ago

FWIW, because Splide sadly seems to be abandoned, I'm transitioning away from it. So far Embla seems to be the one that both fits my use, and is still actively maintained. First impression of performance is very good, even though features and options are a bit different.

https://www.embla-carousel.com/

aprinciple commented 6 months ago

It's error bcz "group" is not valid for the "role" attribute of the <li> element. For <li> need use role="listitem", but <li> elements already have the listitem role in HTML by default. Therefore, you can explicitly specify this using the role attribute. Just remove it from the <li> element

guanacone commented 6 months ago

Is there any know fix for those using @splidejs/react-splide as we are using <SplideSlide> to render the individual slide (whis is a <li> and we can not overwrite that.

paleacci commented 6 months ago

Thanks to the inspiration of @kristiansp I have built a simple but working solution, if anyone needs a source code I will be happy to share it with you.


const splide = new Splide('.reviews-slider', {
    type: 'loop',
    perPage: 2,
    arrows: false,
    breakpoints: {
        1024: {
            perPage: 1,
        },
    },
});

splide.on('mounted', function () {
    for (const list of splide.Components.Elements.slides) {
        list.setAttribute('role', 'presentation');
    }
});

splide.mount();

bronisMateusz commented 2 months ago

I forked this repo and make fix for that issue. Now Accesibility audit in Google Lighthouse is passed: https://github.com/Splidejs/splide/pull/1318

Splidejs / splide