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

<AutoFocusInside/> ignores tabIndex={-1} #108

Closed
Andarist opened this issue Jun 8, 2020 · 4 comments · Fixed by #109
Closed

<AutoFocusInside/> ignores tabIndex={-1} #108

Andarist opened this issue Jun 8, 2020 · 4 comments · Fixed by #109
Assignees

Comments

@Andarist
Copy link

Andarist commented Jun 8, 2020

Explicit information like using the <AutoFocusInside/> should take precedence over the regular tabbable sequence, and this IMHO includes components with tabIndex={-1}. Alternatively, this could be controlled with some extra prop.

Why this is needed?
Sometimes it is advisable to autofocus such elements, according to https://w3c.github.io/aria-practices/#keyboard-interaction-7 :

  • "Unless a condition where doing otherwise is advisable, focus is initially set on the first focusable element." (and this might not be first tabbable)
  • "If content is large enough that focusing the first interactive element could cause the beginning of content to scroll out of view, it is advisable to add tabindex=-1 to a static element at the top of the dialog, such as the dialog title or first paragraph, and initially focus that element."

Repro:
https://codesandbox.io/s/react-focus-lock-n1w2r?file=/index.js

@theKashey
Copy link
Owner

That's a good addition. Right now autofocus is searched only among tabbable (aka focusable via keyboard navigation), however should be searched as "focusable".

@theKashey theKashey self-assigned this Jun 8, 2020
@Andarist
Copy link
Author

Andarist commented Jun 8, 2020

I'm not entirely sure if by default the search should go through all focusable elements - this could be a surprise in certain situations. However, it definitely should be possible to autoFocus such elements (non-tabbables) if there is a sufficiently explicit indicator of wanting this, where usage of <AutoFocusInside/> seems like one. A good thing about having a separate component like this for this purpose is that it can also be easily customized to include or exclude focusables, depending on what will be the chosen default.

@theKashey
Copy link
Owner

Ok. Root cause found - focus-lock was just searching among tabbable elements. And more that in one place - while it will not prevent focusing non-tabbable elements the lock, it will later be unable to calculate focus position in the node list and that's partially undefined behaviour.

👍 a very good thing to fix.

@theKashey
Copy link
Owner

v2.4.0 should resolve your issue

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 a pull request may close this issue.

2 participants