Add option to includeNegativeTabIndex in getFocusableElementsInScope

[voideanvalue]

The idea for this pull request came up when @trueadm and I were talking about being able to differentiate between focusable and tabbable elements (to implement something like roving tabindex for focus management as recommended within aria best practices guide: https://www.w3.org/TR/wai-aria-practices/#kbd_roving_tabindex).

The current getFocusableElementsInScope implementation treats elements with tabIndex === -1 as not focusable. This introduces as boolean flag (includeNegativeTabIndex) that disables the tabIndex < 0 filter. Also made the logic more generic to accommodate for tabIndex values other than -1 and 0. And I added some tests :)

[sizebot]

ReactDOM: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: 9b55bcf...e08e3fb

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.profiling.min.js	0.0%	-0.0%	114.17 KB	114.17 KB	36.04 KB	36.04 KB	NODE_PROFILING
react-dom-unstable-native-dependencies.development.js	0.0%	-0.0%	60.76 KB	60.76 KB	15.85 KB	15.85 KB	UMD_DEV
ReactDOM-dev.js	0.0%	0.0%	922.95 KB	923.31 KB	204.53 KB	204.61 KB	FB_WWW_DEV
react-dom-unstable-native-dependencies.production.min.js	0.0%	-0.0%	10.74 KB	10.74 KB	3.68 KB	3.68 KB	UMD_PROD
ReactDOMServer-dev.js	0.0%	-0.0%	136.28 KB	136.28 KB	34.9 KB	34.9 KB	FB_WWW_DEV
react-dom-unstable-fire.development.js	0.0%	0.0%	899.54 KB	899.76 KB	204.2 KB	204.24 KB	UMD_DEV
react-dom-unstable-fire.production.min.js	0.0%	-0.0%	110.58 KB	110.58 KB	35.6 KB	35.6 KB	UMD_PROD
react-dom-unstable-fire.profiling.min.js	0.0%	-0.0%	113.91 KB	113.91 KB	36.64 KB	36.64 KB	UMD_PROFILING
react-dom-unstable-fire.development.js	0.0%	0.0%	893.83 KB	894.05 KB	202.69 KB	202.73 KB	NODE_DEV
react-dom-unstable-fire.production.min.js	0.0%	-0.0%	110.66 KB	110.66 KB	35.16 KB	35.16 KB	NODE_PROD
react-dom.development.js	0.0%	0.0%	899.19 KB	899.41 KB	204.06 KB	204.1 KB	UMD_DEV
react-dom-unstable-fire.profiling.min.js	0.0%	-0.0%	114.19 KB	114.19 KB	36.05 KB	36.05 KB	NODE_PROFILING
react-dom-server.browser.development.js	0.0%	0.0%	137.48 KB	137.48 KB	36.25 KB	36.25 KB	UMD_DEV
react-dom.production.min.js	0.0%	-0.0%	110.57 KB	110.57 KB	35.59 KB	35.59 KB	UMD_PROD
ReactFire-dev.js	0.0%	0.0%	922.16 KB	922.52 KB	204.41 KB	204.5 KB	FB_WWW_DEV
react-dom.profiling.min.js	0.0%	-0.0%	113.89 KB	113.89 KB	36.63 KB	36.63 KB	UMD_PROFILING
ReactFire-prod.js	🔺+0.1%	🔺+0.1%	363.51 KB	363.83 KB	66.43 KB	66.51 KB	FB_WWW_PROD
react-dom.development.js	0.0%	0.0%	893.49 KB	893.71 KB	202.55 KB	202.59 KB	NODE_DEV
ReactFire-profiling.js	+0.1%	+0.1%	370.38 KB	370.7 KB	67.74 KB	67.81 KB	FB_WWW_PROFILING
react-dom-server.browser.development.js	0.0%	0.0%	133.61 KB	133.61 KB	35.31 KB	35.31 KB	NODE_DEV
ReactDOM-prod.js	🔺+0.1%	🔺+0.1%	375.53 KB	375.85 KB	68.82 KB	68.89 KB	FB_WWW_PROD
ReactDOM-profiling.js	+0.1%	+0.1%	382.44 KB	382.76 KB	70.14 KB	70.21 KB	FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js	0.0%	-0.0%	60.43 KB	60.43 KB	15.72 KB	15.72 KB	NODE_DEV
react-dom-unstable-fizz.node.development.js	0.0%	-0.1%	3.88 KB	3.88 KB	1.51 KB	1.51 KB	NODE_DEV
react-dom-unstable-native-dependencies.production.min.js	0.0%	-0.1%	10.48 KB	10.48 KB	3.58 KB	3.58 KB	NODE_PROD
react-dom-unstable-fizz.node.production.min.js	0.0%	-0.2%	1.1 KB	1.1 KB	666 B	665 B	NODE_PROD
react-dom-unstable-fizz.browser.development.js	0.0%	-0.1%	3.81 KB	3.81 KB	1.54 KB	1.53 KB	UMD_DEV
react-dom-unstable-fizz.browser.production.min.js	0.0%	-0.1%	1.21 KB	1.21 KB	705 B	704 B	UMD_PROD
react-dom-test-utils.development.js	0.0%	-0.0%	56.11 KB	56.11 KB	15.55 KB	15.55 KB	NODE_DEV
react-dom-unstable-fizz.browser.production.min.js	0.0%	-0.2%	1.05 KB	1.05 KB	636 B	635 B	NODE_PROD

Generated by 🚫 dangerJS

[necolas]

Also made the logic more generic to accommodate for tabIndex values other than -1 and 0

I don't think we should look to support values other than -1 or 0, as positive values create accessibility problems.

We're also using this event component with UI components that set tabindex to -1 by default, and have their tabindex indirectly managed via a focusable prop which will set the value to 0. Are you using the Focus event component? Or are you building an event responder?

[voideanvalue]

I don't think we should look to support values other than -1 or 0, as positive values create accessibility problems.

That's a fair point. I'll undo that change and only accept 0 and -1.

Are you using the Focus event component? Or are you building an event responder?

I'm playing with the Focus event component and also building an event responder (a variant of FocusScope to enable roving tabindex on lists). I can share a fully working prototype (once I get it to that stage) with you if you think that'd be useful to see!

[necolas]

I'm not sure how this would be useful in a context where every View renders down to <div tabindex="-1">. The set of elements you'd get back would be every View's host node in the subtree. Would that be a problem? How are you hoping to use the result?

[voideanvalue]

where every View renders down to <div tabindex="-1">

If every <View> renders down to <div tabindex="-1">, I'd consider every <View> focusable (though not tabbable because of the negative tabindex value).

Basing this on reading https://html.spec.whatwg.org/multipage/interaction.html#attr-tabindex and https://html.spec.whatwg.org/multipage/interaction.html#focusable-area. Specifically:

The tabindex content attribute allows authors to indicate that an element is supposed to be focusable, and whether it is supposed to be reachable using sequential focus navigation

If the value is a negative integer
The user agent must set the element's tabindex focus flag, but should omit the element from the sequential focus navigation order.

The following table describes what objects can be focusable areas ... Elements that have their tabindex focus flag set, that are not actually disabled

When I say a tabbable element (or a tab stop), I mean that the element is part of the "sequential focus navigation order" as defined by the spec.

Probably also worth noting that I feel that the name FocusScope doesn't accurately represent its current implementation (it manages tabbability / the "sequential focus navigation order", not all kinds of focus).

---

The set of elements you'd get back would be every View's host node in the subtree. Would that be a problem?

I'm not very familiar with the semantics of the View component or the recommended uses. If every div in a web app came from a View I'd consider it a problem, but more so because I don't expect every div to become a focusable element.

---

How are you hoping to use the result?

My motivation for wanting to be able to distinguish between tabbable and focusable elements is rooted the use case of implementing roving tabindex (use tab key to move focus to across lists and arrow keys to move focus across list items within the current list).

---

An alternative to this pull request could be adding getFocusableElementsInScope and getTabbableElementsInScope to ReactDOMResponderContext (that'd be a breaking change).

[trueadm]

You should check out the other PR that adds focus manager to FocusScope too. There might be ideas that can be expanded on if similar. #15849

[necolas]

When I say a tabbable element (or a tab stop), I mean that the element is part of the "sequential focus navigation order" as defined by the spec.

I understand the semantic differences, although "tabbable" doesn't communicate that the element can also receive focus from other user actions, like mouse or touch. That's one reason why we avoid using "tabbable" for multi-input APIs.

If every div in a web app came from a View I'd consider it a problem, but more so because I don't expect every div to become a focusable element.

This is what I'm trying to understand; whether you might be making assumptions about what elements will have tabindex="-1" for this specific pattern. Flare is intended to provide building blocks for a cross-platform React API, and that would mean not writing web apps with div, etc., at all. We might want to make it so that host nodes rendered by View are programmatically focusable by default (via tabindex=-1), or use tabindex in other scenarios that don't overlap well with other assumptions.

What is it that you want to do with a list of "focusable" elements? And what is it about tabindex=-1|0 that makes that the appropriate marker for building that list?

An alternative to this pull request could be adding getFocusableElementsInScope and getTabbableElementsInScope to ReactDOMResponderContext

I considered this too. We're using focusable in React Native, React Native for Windows, and FB5 to indicate that a View should be user-focusable (via keyboard, touch, or mouse), with programmatic focus being more widely available and straight-forward than it is when working directly with the DOM, where people are used to managing their own tabindex values to make elements like div focusable.

[devongovett]

FWIW, I added a tabbable option already in my other PR for focus management: https://github.com/facebook/react/pull/15849/files#diff-587e37bf8623dff5ba8e829346a9046dR53

[voideanvalue]

"tabbable" doesn't communicate that the element can also receive focus from other user actions, like mouse or touch. That's one reason why we avoid using "tabbable" for multi-input APIs.

Fair point :). I hadn't considered that.

whether you might be making assumptions about what elements will have tabindex="-1" for this specific pattern ... What is it that you want to do with a list of "focusable" elements? And what is it about tabindex=-1|0 that makes that the appropriate marker for building that list?

I might be making assumptions here. I'm trying my best to conform to the HTML Spec and ARIA guidelines. Please let me know if I'm misunderstanding something here.

I'm trying to follow the ARIA guidelines for using roving tabindex for focus management (https://www.w3.org/TR/wai-aria-practices/#kbd_roving_tabindex). Here's a specific use case I want to enable:

<ul>
  <li><a href tabindex="-1">List 1: Link 1</a><li>
  <li><a href tabIndex="0">List 1: Link 2</a><li>
  <li><a href tabindex="-1">List 1: Link 3</a><li>
</ul>
<ul>
  <li><a href tabindex="0">List 2: Link 1</a><li>
  <li><a href tabIndex="-1">List 2: Link 2</a><li>
</ul>

When List 1: Link 2 has focus:

• pressing tab would move focus to List 2: Link 1 (which is the next tabstop)
• pressing down arrow (assuming vertical list orientation) would move focus to List 1: Link 3 and swap tabindex values (List 1: Link 3 gets tabindex 0 and List 1: Link 2 gets tabindex -1)

I have not given sufficient thought to the desirability of proliferating <div tabindex="-1" /> all over the app. It seems a bit odd at first glance.

---

I'll spend some time looking at #15849 and #16009.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!