feat(Accessibility): Added focus handling for List component #256

smykhailov · 2018-09-20T14:01:38Z

Let me introduce focus handling for List component, which is based on the idea of and roving tabindex technique. Also, the idea is based on the current implementation of accessibility behaviors and key handlers. This implementation is based on the research and PoC located in the accessibility repository

Container/AtomicItem hierarchy

A lot of components can be presented as a hierarchical structure as container and item(s), each item may also contain a nested container with its own items. Having this in mind, we can encapsulate focus handling into two components and inject the corresponding functionality into the React component. When container created it can set the first (or any other) item to be focused. The item itself handles the pressed key and fires the corresponding event. Container receives this event and does what is requested as it knows about children, how to handle them and when they have to be updated and how.

Roving tabindex technique

Setting the tabindex of the focused element to "0" ensures that if the user tabs away from the widget and then return, the selected item within the group retains focus. Note that updating the tabindex to "0" requires also updating the previously selected item to tabindex="-1". This technique involves programmatically moving the focus in response to key events and updating the tabindex to reflect the currently focused item.

<radio tabindex="-1">1</radio>
<radio tabindex="-1">2</radio>
<radio tabindex="0">3</radio>
<radio tabindex="-1">4</radio>
<radio tabindex="-1">5</radio>

more details about roving tabindex

src/lib/accessibility/FocusHandling/AtomicItemFocusHandler.tsx

kuzhelov · 2018-10-09T11:41:43Z

src/components/List/List.tsx

@@ -90,7 +108,13 @@ class List extends UIComponent<Extendable<IListProps>, any> {
    const { items } = this.props
    const itemProps = _.pick(this.props, List.itemProps)

-    return _.map(items, item => ListItem.create(item, { defaultProps: itemProps }))
+    return _.map(items, (item, idx) => {
+      itemProps.focusableItemProps = this.focusContainer.assignAtomicItemsProps(idx, items.length)


probably, prepareItemProps or createItemProps (with Item being a singular form) would be better, due to this method

prepares props for single item element

there is no assignment for the result props object made there

probably, we don't need to repeat Atomic in method's names

kuzhelov · 2018-10-09T11:49:57Z

src/components/List/ListItem.tsx

 import { ComponentVariablesInput, ComponentPartStyle } from '../../../types/theme'
 import { Extendable } from '../../../types/utils'

 export interface IListItemProps {
  accessibility?: Accessibility
  as?: any
+  focusableItemProps?: IFocusableItemProps


nit: for the sake of consistency, lets preserve alphabetical order

kuzhelov · 2018-10-09T11:53:03Z

src/components/List/ListItem.tsx

@@ -35,6 +46,7 @@ class ListItem extends UIComponent<Extendable<IListItemProps>, any> {

  static propTypes = {
    as: customPropTypes.as,
+    focusableItemProps: PropTypes.object,


nit: maybe we should place it closer to accessibility prop

kuzhelov · 2018-10-09T11:53:34Z

src/components/List/ListItem.tsx

@@ -75,7 +87,17 @@ class ListItem extends UIComponent<Extendable<IListItemProps>, any> {
    accessibility: listItemBehavior as Accessibility,
  }

-  state: any = {}
+  constructor(props: IListItemProps, context: any) {


context should be omitted as well :) We are not supporting legacy React API. Also, in either case, there is no need to provide this arg here

kuzhelov · 2018-10-09T11:58:43Z

src/components/List/ListItem.tsx

@@ -85,6 +107,10 @@ class ListItem extends UIComponent<Extendable<IListItemProps>, any> {
    this.setState({ isHovering: false })
  }

+  componentDidUpdate() {
+    this.focusableItem.tryFocus(ReactDOM.findDOMNode(this.itemRef.current!) as HTMLElement)


this is better to be rewritten the following way, as we will reduce amount of casts used by one:

ReactDOM.findDOMNode(this.itemRef.current) as HTMLElement

src/components/List/List.tsx

kuzhelov · 2018-10-09T12:10:30Z

src/lib/accessibility/Behaviors/List/selectableListItemBehavior.ts

@@ -11,6 +11,7 @@ const selectableListItemBehavior: Accessibility = (props: any) => ({
    root: {
      role: 'option',
      'aria-selected': !!props['active'],
+      tabIndex: props.focusableItemProps.isFocused ? '0' : '-1',


seems that we should at least specify type for props here, so that it will be clear from which context focusableItemProps come from. Also, we should think about following thing - what if this behavior will be applied to the component that doesn't have this prop support (for some reason, like some custom component that haven't introduced support for FocusableContainer - to support some custom focus handling logic, for instance)? In that case we shouldn't make tabIndex assignments here - otherwise behavior for selectableListItem will be tightly coupled with the focus handling one.

May suggest to either

consider to merge focus handling to be a thing that is defined by accessibility behavior (so that there won't be any code like FocusContainer.create in the component's code - all this will come as accessibility behavior concern). See this option is a preferable one

or decouple these two completely - so that focus handling logic will be entirely responsible for handling tabIndex attributes as well

For the next round of refactoring

kuzhelov · 2018-10-09T15:36:36Z

agreed with Sergey to address several follow-up points by means of PRs that will come next. For the sake of making the roadmap clear, will file an issue item where all these findings will be summarised - and after that merge the PR 👍

In general, lets scope the area to which this approach is applied to List and ListItem components, till the moment when all the necessary refactoring steps will be made and, as a result, general strategy of applying this functionality will reach its maturity point.

kuzhelov

provided functionality raise the following error on switch between Shorthand API and Children API modes of List Selection example. Let sync tomorrow and fix before merging

smykhailov and others added 20 commits September 14, 2018 15:17

Added ability to set initial tabIndex on list items

3efed2d

Added draft support for focus navigation in list with arrow keys

41b133c

Fixed state update

218ec96

Added ability to focus list items

4468ca7

Exposed focusing logic into base components

4d6ef27

Attempt to add focus handling for accordion

178a610

Removed accordion focus handling

c622244

Refactored container component to inject container focusability

0791038

Attempt to update container state from atomic item

8a39ee3

Fixed container part

cb524ef

Moved keydown to keyhandlers

5b24cdb

Made atomic item focus handler injectable

287df48

Fixed ability to focus atomic item when the handler injected

8a14311

Fixed moveNext

11eaa6a

Decoupled container from react component

bf997e1

Added types to container constructor arguments

f5bad97

Decoupled focus handlers from react

24f66f6

Removed unneccessery console.logs

e106a37

Added Home and End key navigation in List

11555c2

Revert Accordion component changes

4d3f144

smykhailov requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners September 20, 2018 14:01

smykhailov changed the title ~~Added focus handling for List component~~ (RFC) Added focus handling for List component Sep 20, 2018

smykhailov changed the title ~~(RFC) Added focus handling for List component~~ [RFC] Added focus handling for List component Sep 20, 2018

Merge branch 'master' into smykhailov/focus-navigation-list

ba0aabc

sophieH29 reviewed Sep 20, 2018

View reviewed changes

src/lib/accessibility/FocusHandling/AtomicItemFocusHandler.tsx Outdated Show resolved Hide resolved

kuzhelov added the 🚀 ready for review label Sep 20, 2018

kuzhelov reviewed Oct 9, 2018

View reviewed changes

src/components/List/List.tsx Show resolved Hide resolved

kuzhelov reviewed Oct 9, 2018

View reviewed changes

Sergii Mykhailov and others added 2 commits October 9, 2018 14:58

Addressed PR comments

c560284

Merge branch 'master' into smykhailov/focus-navigation-list

1c25c2a

miroslavstastny approved these changes Oct 9, 2018

View reviewed changes

kuzhelov-ms and others added 3 commits October 10, 2018 02:07

fix lint issues

5511e2c

Merge branch 'master' into smykhailov/focus-navigation-list

16250f5

update changelog

78fe200

kuzhelov approved these changes Oct 10, 2018

View reviewed changes

kuzhelov added ready for merge and removed 🚀 ready for review labels Oct 10, 2018

fix build after merging

afb519d

kuzhelov suggested changes Oct 10, 2018

View reviewed changes

kuzhelov added needs author changes Author needs to implement changes before merge and removed ready for merge labels Oct 10, 2018

kuzhelov approved these changes Oct 10, 2018

View reviewed changes

Fixed the issue with undefined props, while try focus

f251ecc

smykhailov added ready for merge and removed needs author changes Author needs to implement changes before merge labels Oct 11, 2018

Merge branch 'master' into smykhailov/focus-navigation-list

cd608a1

smykhailov merged commit d0a31a6 into master Oct 11, 2018

smykhailov deleted the smykhailov/focus-navigation-list branch October 11, 2018 08:24

gopalgoel19 mentioned this pull request Oct 16, 2018

feat(ListItem): add active prop #332

Closed

11 tasks

levithomason mentioned this pull request Oct 25, 2018

Multiple Selectable lists cause scrolling issues due to auto-focus #370

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Accessibility): Added focus handling for List component #256

feat(Accessibility): Added focus handling for List component #256

smykhailov commented Sep 20, 2018

kuzhelov Oct 9, 2018

smykhailov Oct 9, 2018

kuzhelov Oct 9, 2018

smykhailov Oct 9, 2018

kuzhelov Oct 9, 2018

smykhailov Oct 9, 2018

kuzhelov Oct 9, 2018 •

edited

smykhailov Oct 9, 2018

kuzhelov Oct 9, 2018

smykhailov Oct 9, 2018

kuzhelov Oct 9, 2018 •

edited

smykhailov Oct 9, 2018

kuzhelov commented Oct 9, 2018 •

edited

kuzhelov left a comment

feat(Accessibility): Added focus handling for List component #256

feat(Accessibility): Added focus handling for List component #256

Conversation

smykhailov commented Sep 20, 2018

Container/AtomicItem hierarchy

Roving tabindex technique

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Oct 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Oct 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov commented Oct 9, 2018 • edited

kuzhelov left a comment

Choose a reason for hiding this comment

kuzhelov Oct 9, 2018 •

edited

kuzhelov Oct 9, 2018 •

edited

kuzhelov commented Oct 9, 2018 •

edited