[RFC] feat(Accessibility): Make action handlers conditional based on Event object

[sophieH29]

As agreed during the discussion, will keep this PR for further discussion on achieving better solution for having conditional action handler based on an event object.
Meanwhile, a split of action handlers was introduced in another PR #1622

Fixes #1502

PROBLEM

There a lot of confusion in key actions, as initially key action was aimed to do always one thing, now it can do multiple things, plus on different conditions of props values and event object.

Example of such actions:
Dialog - closeAndFocusTrigger

closeAndFocusTrigger: e => {
      this.handleDialogCancel(e)
      e.stopPropagation()

      _.invoke(this.triggerRef, 'current.focus')
},

Attachment:

performClick = e => {
    if (e.currentTarget === e.target) {
      e.stopPropagation()
      this.handleClick(e)
    }
  }

TreeItem:

collapseOrReceiveFocus: e => {
      const { items, open } = this.props

      e.preventDefault()

      // Focuses the title if the event comes from a child item.
      if (e.currentTarget !== e.target && items && items.length) {
        e.stopPropagation()
        this.itemRef.current.focus()
      } else if (open) {
        e.stopPropagation()
        _.invoke(this.props, 'onTitleClick', e, this.props)
      }
    },

Also already discussed with @kuzhelov, here is a comment which is totally valid

SOLUTION

I've made some investigation, things that I found out:

• actions are called in that sequence that they appear in behaviors.
  For example, we have the next defined actions:

 keyActions: {
    root: {
      Spacebar: {
        keyCombinations: [{ keyCode: keyboardKey.Spacebar }], 
      },
      enterSpace: {
        keyCombinations: [{ keyCode: keyboardKey.Enter }, { keyCode: keyboardKey.Spacebar }],
      },
      performClick: {
        keyCombinations: [{ keyCode: keyboardKey.Enter }, { keyCode: keyboardKey.Spacebar }],
      },
      enter: {
        keyCombinations: [{ keyCode: keyboardKey.Enter }],
      },
    },
  },

When a user press SPACE, the sequence of called actions will be

1. Spacebar
2. enterSpace
3. performClick

When a user press ENTER, the sequence of called actions will be

1. enterSpace
2. performClick
3. enter

That means, in cases where we have actions like closeAndFocusTrigger in Dialog

keyActions can be transformed FROM:

keyActions: {
      popup: {
        closeAndFocusTrigger: {
          keyCombinations: [{ keyCode: keyboardKey.Escape }],
        },
  },

TO:

keyActions: {
      popup: {
        close: {
          keyCombinations: [{ keyCode: keyboardKey.Escape }],
        },
        focusTrigger: {
          keyCombinations: [{ keyCode: keyboardKey.Escape }],
        },
  },

And then implementation in Dialog:
FROM:

actionHandlers = {
    closeAndFocusTrigger: e => {
      this.handleDialogCancel(e)
      e.stopPropagation()

      _.invoke(this.triggerRef, 'current.focus')
    },
  }

TO:

actionHandlers = {
    close: e => this.handleDialogCancel(e),
    focusTrigger: e => {
      e.stopPropagation()
      _.invoke(this.triggerRef, 'current.focus')
    },
  }

Also, for cases when we depend on conditions like checking values of props and state, event object, we can extend API of keyActions and allow user to specify condition when the specific action should be called.

Extended API:

export interface KeyAction {
  keyCombinations: KeyCombinations[]
  condition?: (event, props) => boolean
}

I introduced changes in TreeItem and treeItemBehavior to showcase how it will work.

Example of usage of condition prop in treeItemBehavior

    ...
    receiveFocus: {
        keyCombinations: [{ keyCode: keyboardKey.ArrowLeft }],
        condition: doesEventComesFromChildItem,
      },
      collapse: {
        keyCombinations: [{ keyCode: keyboardKey.ArrowLeft }],
        condition: (event, props: TreeItemBehaviorProps) =>
          !doesEventComesFromChildItem(event, props) && props.open,
      },
   ...

And then in TreeItem:

receiveFocus: e => {
      e.preventDefault()
      e.stopPropagation()

      this.itemRef.current.focus()
    },
collapse: e => {
      e.preventDefault()
      e.stopPropagation()

      _.invoke(this.props, 'onTitleClick', e, this.props)
},

CONCERNS

I hesitate if should we move all conditions to behaviors as they're a part of the inner logic of components. Clients have no idea how they can impact if to change them.
I want us to discuss and agree on these points:

• separate combined actions into small actions, so then we'll have few actions assigned to the same key(s).
• move conditions logic which defines if the action should be called to behaviors OR as we'll have broken down small actions, leave conditions inside them.

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://stardust-react-git-feat-conditional-action-handlers.stardust-ui.now.sh

[miroslavstastny]

I am not sure I like the combination of conditionally adding key handlers based on props and conditionally invoking them based on event (and eventually props).
Wouldn't it be easier to switch everything to shouldHandle condition? (I am aware of the fact that this can result in added keyhandlers which never call any action.)
Other opinions?

[sophieH29]

@miroslavstastny I totally share your thought, have same concerns.
@kuzhelov @mnajdova @layershifter need your input here

[levithomason]

Suggestion: focusSubtree? It would be nice to say where we are passing the focus to. Currently, it says to pass it, but as a dev implementing this I'm not sure where to pass it to.

[levithomason]

Consider if it is enough to check the open prop. If so, consider if we can collapse this function to inline logic.

[levithomason]

We've all agreed splitting the handlers into single purpose functions is correct. let's get this merged in a separate PR. we'll continue to discuss the shouldHandle and other concerns in this PR.

[jdhuntington]

Closing as this has seen no activity for awhile. Please reopen if still relevant. Thanks!