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

Added ability to scroll with keyboard #587

Merged
merged 3 commits into from
Sep 15, 2021
Merged

Added ability to scroll with keyboard #587

merged 3 commits into from
Sep 15, 2021

Conversation

Vikkivi
Copy link

@Vikkivi Vikkivi commented Aug 11, 2021

It was impossible to scroll slider with keyboard in Chrome and Safari.
I added tabindex="0" to allow focusing on slider and aria-label for screen readers.
More info here https://marcus.io/blog/accessible-overflow

@Grsmto
Copy link
Owner

Grsmto commented Sep 15, 2021

Great stuff! Thanks!

@Grsmto Grsmto merged commit 18d3270 into Grsmto:master Sep 15, 2021
@oleg-slapdash
Copy link

@Grsmto this should be configurable since it breaks existing keyboard navigation in existing products. According to your previous comment:

I don't understand what you mean by "Keyboard operability is absent". It seems to work exactly the same way as a native overflow: auto. Just applying overflow: auto to a div won't give it a tabindex so SimpleBar doesn't neither.
It would be too arbitrary for SimpleBar to add tabindex by default as it's not there natively. So you would have to add it yourself manually in JS.

Which makes total sense. Is it possible to make it configurable?

@codeart1st
Copy link

I don't understand the need for setting the tabindex on the wrapper container. It might be needed for pure text content, but with an tabable item in it, it was completely fine before the change. Now it's an unnecessary extra tab. Also some extra css for the focus state is needed to keep up with existing UI.

The change might be fine if it's optional or can be disabled.

@paulkirow
Copy link

paulkirow commented Apr 8, 2022

Hard coding role="region" is also quite opinionated. Prior to this update we used simplebar on our navigation element which had tabable items inside. In our context, for screen readers there is now an additional unnecessary declaration of "scrollable content - region".

I can remove the parent role in my page but I'm unable to update this role="region" to a more applicable role.

For now I'm just going to use a version prior to this change.

I agree with other comments, this feature should be configurable, ideally disabled by default.

@mcuppi
Copy link

mcuppi commented Jul 7, 2022

For anyone who wants to disable this behavior, you can use the following pattern:

const simpleBar = new SimpleBarJS(scrollElement, options);
simpleBar.contentWrapperEl.removeAttribute("tabindex");

This isn't ideal, but it works until the feature is made configurable (if ever).

Grsmto added a commit that referenced this pull request Jan 2, 2023
@meldafert meldafert mentioned this pull request Jan 2, 2024
@kalnode
Copy link

kalnode commented Apr 17, 2024

Where did this end up?

@mcuppi 's workaround works for me, but is it still necessary?

shubham-padia added a commit to shubham-padia/simplebar that referenced this pull request Jun 12, 2024
After Grsmto#587, there had been many complaints about `tabIndex=0`
breaking existing behaviour. This PR adds `tabIndex` as an option
with it's default set to 0.
shubham-padia added a commit to shubham-padia/simplebar that referenced this pull request Jun 12, 2024
After Grsmto#587, there had been many complaints about `tabIndex=0`
breaking existing behaviour. This PR adds `tabIndex` as an option
with it's default set to 0.
Grsmto pushed a commit that referenced this pull request Jun 13, 2024
* options: Add tabIndex as a configurable option.

After #587, there had been many complaints about `tabIndex=0`
breaking existing behaviour. This PR adds `tabIndex` as an option
with it's default set to 0.

* README: Add ariaLabel as an option.
@shubham-padia
Copy link
Contributor

tabIndex is now configurable via #695

@tlthiem
Copy link

tlthiem commented Jul 10, 2024

time for a release then

@shubham-padia
Copy link
Contributor

Latest release already has this change.

@tlthiem
Copy link

tlthiem commented Jul 10, 2024


nvm 3.2.6 got it

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

Successfully merging this pull request may close these issues.

None yet

9 participants