theKashey / react-focus-lock

It is a trap! A lock for a Focus. 🔓
MIT License
1.27k stars 67 forks source link

No way to prevent AutoFocusInside from focusing tabindex="-1" elements #210

Closed cojhal closed 2 years ago

cojhal commented 2 years ago

As described in #108, there are cases where autofocusing elements with tabindex -1 is desirable, but there are other use cases for tabindex -1 where that does not make sense. For example, a composite control with a roving tab index should place focus on the active element, not necessarily the first one.

The issue can be reproduced here: https://codesandbox.io/s/react-focus-lock-tabindex-1-example-forked-371qh1. This sample works as intended if I downgrade to react-focus-lock@2.3.1.

theKashey commented 2 years ago

Easy to fix, but hard to roll out the fix, as someone can rely on this not very correct behaviour - https://www.hyrumslaw.com/

I need first to understand why 2.3.1 works as expected, because the corresponding in the underlying focus-lock has not changed.

cojhal commented 2 years ago

Right, there are cases where the current behavior is correct, as described in #108. I see the change for that issue was implemented in theKashey/focus-lock#17 and adopted in react-focus-lock@2.4.0.

theKashey commented 2 years ago

Thank you for finding the root cause. And look like I haven't understood w3c recommendations properly as it clearly states that the current behavior is advised only "If content is large enough"

Frankly speaking, which element should be focused is still a little unclear to me - some solutions(reach-ui) prefer to focus the "dialog itself", some - the first action, some - the main action (that's a bad idea).

While I cannot answer this question, let's try to answer answer one - what you want to do, and how your particular problem can be resolved? For example, can you use autofocus on a particular button, or wrap only desired button with AutoFocusInside?

cojhal commented 2 years ago

I was able to resolve my particular problem by manually moving focus when an incorrect element gets focus, so we can stick to the general case.

Since tabindex="-1" elements have other valid uses, I don't think autofocusing them is the ideal default behavior. If a developer adds one for specifically for the purpose of getting autofocus, I think it is reasonable to ask them to mark it as such with a data attribute or an option on AutoFocusInside.

I understand that would be a breaking change, so an alternative solution would be to keep the current behavior and provide a way to opt-out of it, like with a data-no-autofocus attribute or a tabbableOnly option on AutoFocusInside

theKashey commented 2 years ago

Now(react-focus-lock@2.9.0) one can control autofocus behavior by setting data-no-autofocus on an element or container - https://github.com/theKashey/focus-lock#declarative-control That should let you control autofocus behavior as desired.