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

accessibility: tabindex is always 1 now #152

Closed
hongbo-miao opened this issue Apr 21, 2018 · 13 comments
Closed

accessibility: tabindex is always 1 now #152

hongbo-miao opened this issue Apr 21, 2018 · 13 comments

Comments

@hongbo-miao
Copy link

hongbo-miao commented Apr 21, 2018

Environment

  • Version of smooth-scrollbar: 8.2.7

Issue Summary

Current tabindex is always 1.

containerEl.setAttribute('tabindex', '1');

It would be great to give user ability to change tabindex number

@idiotWu
Copy link
Owner

idiotWu commented Apr 21, 2018

Using a tabindex greater than 0 is considered an anti-pattern. --Using tabindex

I'm wondering if it's better to set tabindex="0"?

@hongbo-miao
Copy link
Author

Hi @idiotWu I am not sure actually. I am quite new to accessibility.

I saw for react-select, they set to 0 by default and provide an option. So maybe you are right! https://github.com/JedWatson/react-select/blob/82910d9225f7aff73ac5052da9dc1e7f52f13de7/lib/Select.js#L1046

@idiotWu
Copy link
Owner

idiotWu commented Apr 21, 2018

I'm not sure too. But now I think tabindex="-1" maybe a good idea, because:

  1. we need to set tabindex to make scrollbar focusable (for keyboard events)
  2. but a scrollable area is not supposed to be focusable with keyboard navigation, since it's not an input element or a link

So set tabindex to -1 will make it focusable with mouse clicking, but will remove it from natural tab order so it will never be focused by pressing tab key.

What do you think?

@hongbo-miao
Copy link
Author

hongbo-miao commented Apr 21, 2018

I feel providing a number by default, and then provide user an option is best solution.
I checked Google search results page's tabIndex. When the focus is on the results component, you can press Up and Down keys to go up and down the page. So this is similar to the situation when the focus is on smooth-scrollbar. You can also press Up and Down keys to control.
So I feel sometimes the focus should be on the scroll bar if the content is important, but sometimes the tab should skip that area if it is not important.

-1 or 0 by default is really out of my knowledge... Hope someone can help us.

@idiotWu
Copy link
Owner

idiotWu commented Apr 21, 2018

-1 makes the scrollbar container unfocusable, but the elements inside it are still focusable! IMO it's the most native-like behavior so far.

You can try it on the demo page (change tabindex to "-1" in devtool).

@idiotWu
Copy link
Owner

idiotWu commented Apr 21, 2018

So this is similar to the situation when the focus is on smooth-scrollbar. You can also press Up and Down keys to control.

This is a little hard to implement. The logic may be:

  1. document.activeElement is an input or textarea or element that is content-editable -> do not scroll
  2. document.activeElement is the container or an element inside container -> scroll

@hongbo-miao
Copy link
Author

hongbo-miao commented Apr 21, 2018

Got it, then I think maybe go with -1 is best solution at now!

idiotWu added a commit that referenced this issue Apr 21, 2018
idiotWu added a commit that referenced this issue Apr 21, 2018
@idiotWu
Copy link
Owner

idiotWu commented Apr 21, 2018

@hongbo-miao I've made some updates on the accessibility branch, would you mind giving it some tests? 😁

@hongbo-miao
Copy link
Author

Sure, I will test this weekend!

@alejost848
Copy link

@idiotWu @hongbo-miao Hello, I was auditing my site with Lighthouse and this came up:
image

#160 seems to fix this. Is there anything else to do to get it merged into master?

@idiotWu
Copy link
Owner

idiotWu commented Nov 18, 2018

@alejost848 it looks good to me, but I think we may need some more tests. Can you guys help me test the PR #160 ?

idiotWu added a commit that referenced this issue Nov 18, 2018
@antoninlanglade
Copy link

@idiotWu Can you merge it into the master branch ? It's really important for google lighthouse audit.

@idiotWu
Copy link
Owner

idiotWu commented May 13, 2019

@alejost848 @antoninlanglade fixed in v8.4.0.

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

No branches or pull requests

4 participants