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

UseGroupRole = false generate an error #41

Closed
rianfloo opened this issue Sep 13, 2021 · 13 comments
Closed

UseGroupRole = false generate an error #41

rianfloo opened this issue Sep 13, 2021 · 13 comments

Comments

@rianfloo
Copy link

rianfloo commented Sep 13, 2021

Hello :)

I am using slick accessible but when I add option : useGroupRole: false it generates a JS error.

Cannot read property 'replace' of undefined

Any solution? Here is my code :)

$('.carousel-cards').slick({
      dots: true,
      arrows: true,
      slidesToShow: 4,
      infinite: true,
      autoplay: true,
      regionLabel: 'caroussel',
      useGroupRole: false,
      responsive: [{
          breakpoint: 3000,
          settings: {
            slidesToShow: 3
          }
        },
        {
          breakpoint: 800,
          settings: {
            slidesToShow: 2
          }
        },
        {
          breakpoint: 600,
          settings: {
            slidesToShow: 1
          }
        }
      ],
      prevArrow: '<button class="previous-button is-control">' +
        '  <span class="fas fa-angle-left-white" aria-hidden="true"></span>' +
        '  <span class="sr-only">Diapositive précédente</span>' +
        '</button>',
      nextArrow: '<button class="next-button is-control">' +
        '  <span class="fas fa-angle-right-white" aria-hidden="true"></span>' +
        '  <span class="sr-only">Diapositive suivante</span>' +
        '</button>',
    });

Thanks a lot!

@jasonwebb
Copy link

Hi @rianfloo! Can you share the markup you are using too? The config options look fine to me, so I'm guessing there is more going on.

@rianfloo
Copy link
Author

Hi Jason here it is.

<div class="carousel-cards">
   <?php foreach($cards as $card):?>
          <div title="<?php echo $card['title'];?>" class="text-center" data-aos="fade-up"
              data-aos-delay="<?php echo $delay += 100;?>">
              <div class="img--wrapper d-flex justify-content-center">
                  <img loading="lazy" class="img-fluid" src="<?php echo $card['image']['sizes']['large'];?>"
                      alt="<?php echo $card['image']['alt'];?>">
              </div>
              <h3 id="<?php echo $card['title'];?>" class="card--title"><?php echo $card['title'];?></h3>
              <div class="card--description"><?php echo $card['description'];?></div>
          </div>
   <?php endforeach;?>
 </div>

I am using WordPress and ACF to generate fields.

You can have a look to the slider live here: https://42.fr/le-campus-de-paris/42-paris-campus/#campus

I need to achieve that in order to get French RGAA norm. 🙏

Btw, most of all events (e.g afterChange) crashes when trying to use them. Is there any DOM issues? It's wrapped around a document ready function.

Thanks a lot. 🙏

@jasonwebb
Copy link

It looks like there is indeed a bug that happens when you set useGroupRole to false that wasn't caught during development. As part of a clean-up function, some text is removed from the aria-label attribute on each slide's wrapper that could be inserted when "center mode" is used. However, when useGroupRole is set to false, no aria-label gets set on the slide wrapper at all, so an error gets thrown when jQuery tries to select it.

I've prepared a fix for this which will modify the aria-label only when centerMode: true and useGroupRole: false, which I'll commit shortly. This appears to resolve the issue when I use your markup and config. I'm planning to release a new minor version (v1.0.2) of accessible-slick later this week, and this will be included in it.

In your use case, though, I really don't think the ARIA group role should be disabled. Each slide contains an image with an alt description that comes before the heading of the slide, which means that the headings don't actually "head" all the content of their slides. Without the ARIA group role, screen reader users wouldn't know which image belongs to which slide, and would most likely think that each image belongs to the previous slide, since it is technically following the heading in that slide.

Sighted users can easily see which content belongs to which slide because they are grouped together visually, but blind and low-vision screen reader users would probably think that the content is structured more like this:

Cropped screenshot of the use case from 42.fr, showing red lines surrounding the heading, description, and following slide image. The image of the first slide and the heading with description of the last slide have purple outlines around them.

The staggered red outlines show what a screen reader would think is a slide, since headings are expected to be the beginning of a section of a content. Without the ARIA group role, a screen reader user would have no way of knowing when they've exited one slide and entered another. Since the content is staggered this way, I've drawn purple boxes around the content that would be especially confusing for screen reader users, since they defy the pattern followed by the other slides.

@rianfloo
Copy link
Author

Hi @jasonwebb,

Thanks for your fast answer. It would be amazing to get the fix this week. 🍾

The agency responsible for the audit suggested to delete useGroupRole and to add a focus & tabindex to the first active item. Then when it slides, move the focus to the next active slide. I planned to use afterChange event witch contains currentSlide. (Hope your fix, will also make afterChange usable again). The only condition to let useGroupRole would be to be able to change aria-label content but I don't think your library provides that option.

Anyway, thanks for the feedback, I will share it with the agency :)

@jasonwebb
Copy link

Sure thing!

Personally I would suggest pushing back on the agency's request to remove the ARIA group role, since it seems necessary in this use case to satisfy WCAG 1.3.1 given there is related content preceding each heading in these slides.

Making slides focusable with tabindex="0" would be also strange for keyboard users, since they would only expect to be able to focus on actionable elements (meaning elements that DO something when you activate them, like links and buttons). It doesn't feel like a WCAG violation either way to me, but making each slide focusable would add unnecessary tab stops to the page sequence, which would appear to be broken for keyboard users since they wouldn't do anything when activated.

Finally, I'm not able to replicate the issues you're having with API events like afterChange. Can you please provide a working code sample, preferably on CodePen, that demonstrates this behavior?

jasonwebb added a commit that referenced this issue Sep 14, 2021
@rianfloo
Copy link
Author

rianfloo commented Sep 15, 2021

I am gonna talk to the agency.

I guess this is also related to aria-label issue. I've got the same error :)

I am gonna try your fix! Is there any new NPM version?

Thanks a lot 🍾

@rianfloo
Copy link
Author

Hi @jasonwebb any news regarding NPM package? I use your library with NPm and webpack

@rianfloo
Copy link
Author

Hi @jasonwebb, do you know when NPM package will be updated?

Last version was published 10 months ago and I need to finish the project by updating with your fix.

Thanks a lot :)

@jasonwebb
Copy link

@rianfloo Sorry, but I am no longer a maintainer for this project, so this would need to be done by someone at the @Accessible360 organization.

This is a small one-line fix that you can make in a fork on your own if you need it right away. However, as noted above, the ARIA group role seems very much appropriate for your use case, and definitely not a WCAG violation, so I still recommend leaving it in place for a better user experience.

@rianfloo
Copy link
Author

Hi @jasonwebb, thanks for you answer. I will try your solution.

Good luck for your new project :)

@damnsamn
Copy link

Just bumping this, I am running into the same issue

@rianfloo
Copy link
Author

rianfloo commented Jan 21, 2022

Hi @damnsamn @jasonwebb has merged the fix on master. You should be able to get the updated code and fix your issue but you will not able to use the library with NPM as the last version has not been published on it.

I managed to make it works on my project by minifing the code from the new version.

The only last problem is that aria-labels injected by the new version show as undefined in the markeup. You can use onInit Event to destroy all aria-label by looping on all slides. cc @Accessible360

@pratikbassi
Copy link

Issue closed as fixed on master, will be in the next minor release. Separate issue created for "aria-labels injected by the new version show as undefined in the markup" for investigation. #66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants