Make carousel indicators actual buttons

[patrickhlauke]

Currently, the carousel indicators are not conveyed at all to assistive technologies, and they can only be operated using the mouse.

This change generalises some of the stylings and JS to be backwards-compatible (meaning it's non-breaking), but caters for a more correct structure where the indicators are actual button elements (thus keyboard-focusable/operable), and where the currently active indicator is conveyed with aria-current

Preview: https://deploy-preview-32661--twbs-bootstrap.netlify.app/docs/5.0/components/carousel/#with-indicators

[patrickhlauke]

This relies on #32631 being merged to remove the overridden focus indicator for the button in reboot

[ffoodd]

LGTM, this is really needed.

I only wonder if we shouldn't introduce anew class name for single indicator (eg .carousel-indicator) instead of relying on data- attribute?
Edit: would be a breaking change.

[patrickhlauke]

Edit: would be a breaking change.

yup, exactly. i see this as a first step, then we can make it a breaking change next time to offer more styling flexibility

[patrickhlauke]

@mdo good to merge?

[XhmikosR]

Nope. These are added to beta3.

…

On Sun, Jan 17, 2021, 14:22 Patrick H. Lauke ***@***.***> wrote: @mdo <https://github.com/mdo> good to merge? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#32661 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNNI4O5GB7R3J3XGAULS2LJADANCNFSM4VRZCBLA> .

[XhmikosR]

So, about the JS part. Before, we were only looping once. Now we loop twice. Why can't we use children() like before? Also, I don't get how more than one active indicators can be present?

[patrickhlauke]

Why can't we use children() like before

because it's not guaranteed that they're immediate children. before, with our structure, it was just a list, and the indicators were the immediate <li> elements. now, you may be using one container for all the indicators, and then have a few more structures in between, and only then the links or buttons of whatever you're using.

i.e. I could decide that yes, I still want an ordered list structure, so end up with

<ol class="carousel-indicators">
  <li ...><button type="button" data-bs-target="#carouselExampleIndicators" data-bs-slide-to="0" aria-label="Slide 1"></button></li>
  ...
</ol>

or whatever.

[XhmikosR]

How about closest then?

…

On Thu, Jan 28, 2021, 22:03 Patrick H. Lauke ***@***.***> wrote: Why can't we use children() like before because it's not guaranteed that they're immediate children. before, with our structure, it was just a list, and the indicators were the immediate <li> elements. now, you may be using one container for all the indicators, and then have a few more structures in between, and only *then* the links or buttons of whatever you're using. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#32661 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACVLNNCIFWGYFIBEY5ITZLS4G7KHANCNFSM4VRZCBLA> .

[XhmikosR]

NVM, that probably wouldn't work either. I guess the current solution is as good as it gets, then. I'll review tomorrow and merge it.

…

On Thu, Jan 28, 2021, 22:04 XhmikosR ***@***.***> wrote: How about closest then? On Thu, Jan 28, 2021, 22:03 Patrick H. Lauke ***@***.***> wrote: > Why can't we use children() like before > > because it's not guaranteed that they're immediate children. before, with > our structure, it was just a list, and the indicators were the immediate > <li> elements. now, you may be using one container for all the > indicators, and then have a few more structures in between, and only > *then* the links or buttons of whatever you're using. > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <#32661 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AACVLNNCIFWGYFIBEY5ITZLS4G7KHANCNFSM4VRZCBLA> > . >

[patrickhlauke]

I don't get how more than one active indicators can be present?

I don't think my code looks for more than one? I do findOne to get the current active indicator and then remove its class and aria-current, and then i loop through all indicators, but as soon as i find one that has the right data-bs-slide-to i set class/aria-current and then break (as suggested by @rohit2sharma95)