[RFC]Feat(Accessibility): Add keyboard action handlers #121

sophieH29 · 2018-08-22T17:12:27Z

This PR introduces the initial implementation for keyboard action handlers.

Actions are defined in Component - Component does know what it can do
Keys mapping to actions are defined in Accessibility behavior (actionsDefinition) - Component's actions can be invoked by different keys depending on Accessibility behavior needs

If the Component has matching actions with it's accessibility behavior actionsDefinition, then onKeyDown handler will be added to the rest props in renderComponent.tsx with appropriate actions called and filtered by keys.

Actions were added to MenuItem for now, so by pressing Enter or Space, click handler will be invoked.

jurokapsiar · 2018-08-22T18:27:42Z

src/lib/renderComponent.tsx

@@ -58,11 +60,47 @@ const getAccessibility = <P extends {}>(props, state) => {
  })
 }

+/**


I think this function is now long enough to be separated to a separate file (similarly to how renderComponent is in its own ts file

Agree, will move to separate file

codecov · 2018-08-23T08:21:34Z

Codecov Report

Merging #121 into master will decrease coverage by 0.09%.
The diff coverage is 50%.

@@            Coverage Diff            @@
##           master     #121     +/-   ##
=========================================
- Coverage   89.32%   89.23%   -0.1%     
=========================================
  Files          50       50             
  Lines         834      836      +2     
  Branches      119      119             
=========================================
+ Hits          745      746      +1     
- Misses         85       86      +1     
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Menu/MenuItem.tsx	`89.28% <50%> (-3.03%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 012fcad...c7af853. Read the comment docs.

miroslavstastny · 2018-08-23T09:26:49Z

src/lib/addKeyDownHandler.ts

+  if (!actions || !actionsDefinition) return
+
+  let hasCommonActions = false
+  for (const actionName in actionsDefinition) {


Not significantly shorter, but easier to read?

const hasCommonActions = _.intersection(_.keys(actionsDefinition), _.keys(actions)).length > 0

Definitely, thanks! Still can't get used to lodash, like vanilla :)

mnajdova · 2018-08-23T11:01:27Z

src/lib/addKeyDownHandler.ts

+      )
+      eventHandler && eventHandler(event)
+    }
+


You can also refactor this with iterating the elements of the .intersection(.keys(actionsDefinition), _.keys(actions)) that Miro suggested

mnajdova · 2018-08-23T11:10:26Z

Everything looks ok so far, but I would suggest adding some tests for covering this, especially because is something general used in the renderComponent.

sophieH29 · 2018-08-23T11:46:39Z

@mnajdova sure, working on it right now

smykhailov · 2018-08-23T13:54:18Z

src/lib/accessibility/interfaces.ts

+
+export interface KeyCombinations {
+  keyCode: number
+  shiftKey?: boolean


I'd rather use default values instead of optional, as this will cause compiler error when we enable strict null check options.

@smykhailov Make sense. But as it's interface, we'll need to set these values for each key combination. Or you meant some other usage?

I think it is OK to leave it as it is. This should work with strict null checks, see the condition in keyboardHandlerFilter

levithomason · 2018-08-24T16:42:00Z

src/lib/renderComponent.tsx

@@ -76,6 +77,9 @@ const renderComponent = <P extends {}>(
        const ElementType = getElementType({ defaultProps }, props)
        const rest = getUnhandledProps({ handledProps }, props)

+        const accessibility = getAccessibility(props, state)
+        addKeyDownHandler(rest, actions, accessibility, props)


Since rest/props are component centric ideas, proposing we merge these in the addKeyDownHandler signature so that the method is not aware of internal component implementations.

addKeyDownHandler({ ...rest, ...props }, actions, accessibility)

Now, there is just a props argument if you will.

jurokapsiar · 2018-08-27T13:20:31Z

src/lib/accessibility/interfaces.ts

 export interface IAccessibilityDefinition {
  attributes?: AccessibilityAttributes
-  keyHandlers?: AccessibilityKeyHandlers
+  actionsDefinition?: ActionsDefinition
+  handlers?: ActionsHandler


let's separate handlers to a separate interface so that they can't be defined in the behavior but can only be a result of getAccessibility

@jurokapsiar, If I understood correctly, we should have something like

export interface IAccessibilityBehavior { attributes?: AccessibilityAttributes handlers?: ActionsHandler }

or

export interface IAccessibilityBehavior extends IAccessibilityDefinition { handlers?: ActionsHandler }

as a result of getAccessibility?

jurokapsiar · 2018-08-27T13:25:55Z

src/lib/addKeyDownHandler.ts

+  accessibility: IAccessibilityDefinition,
+  props: IRenderConfigProps,
+) => {
+  accessibility.handlers = {}


this might lead to a state when global accessibility object is modified. instead of modification, create a copy of the object, for example using

const result = { {...accessibility}, handlers: {} }

Agree, will change

…avior

…lity behavior" This reverts commit 577fcf3.

miroslavstastny · 2018-09-06T08:30:01Z

src/lib/getKeyDownHandlers.ts

+/**
+ * Assigns onKeyDown handler to the Component's part element, based on Component's actions
+ * and keys mappings defined in Accessibility behavior
+ * @param {AccessibilityActions} actions The input element which is to loose focus.


Please update param descriptions

right, thx :)

kuzhelov · 2018-09-06T09:12:44Z

src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts

@@ -11,6 +12,14 @@ const MenuItemBehavior: Accessibility = (props: any) => ({
      tabIndex: '0',
    },
  },
+
+  actionsDefinition: {


maybe we could use simpler name like just actions?

Yeah, but it can break the meaning of it, especially during mapping real actions of a component to these. As for me, this name really means what it does.

Fix description for method param # Conflicts: # src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts # src/lib/accessibility/interfaces.ts # src/lib/renderComponent.tsx

miroslavstastny

Approving, please take a look into the comments added

miroslavstastny · 2018-09-06T12:03:48Z

src/lib/keyboardHandlerFilter.ts

+      (keysCombinations.metaKey && !metaKey) ||
+      (keysCombinations.ctrlKey && !ctrlKey)
+    ) {
+      return null


The predicate should return boolean, so false here.
You can also use lodash some to improve performance, and (nit) put keyCode comparison first in the condition for short-circuit evaluation:

const { shiftKey, altKey, metaKey, ctrlKey } = event const isHandled = _.some(keysCombinations, keysCombination => ( keysCombination.keyCode === keyboardKey.getCode(event) && (!keysCombination.altKey || altKey) && (!keysCombination.shiftKey || shiftKey) && (!keysCombination.metaKey || metaKey) && (!keysCombination.ctrlKey || ctrlKey) )) if (isHandled) { handler(event) }

miroslavstastny · 2018-09-06T12:09:56Z

src/lib/accessibility/interfaces.ts

+
+export interface KeyCombinations {
+  keyCode: number
+  shiftKey?: boolean


I think it is OK to leave it as it is. This should work with strict null checks, see the condition in keyboardHandlerFilter

kuzhelov · 2018-09-06T12:20:48Z

src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts

+
+  actionsDefinition: {
+    anchor: {
+      performClick: {


this name seems to be a bit confusing: essentially, it reads like make a click when key is pressed - while, essentially, the intent is to select menu item (this would describe what we'd like to achieve, not how - by click or something else). What do you think?

yes and no :) Totally agree with your point to have a defined intent in actions' namings. But, in the current case, we want to trigger click event by pressing Enter or Space. Let's assume that the click handler will be changed to make some other action (not selecting a menu item).
But our intent will stay the same on Enter and Space do whatever click does
And then, if we'll need to have an action for selecting a menu item, we can create it separately.

kuzhelov · 2018-09-06T12:30:59Z

src/lib/getKeyDownHandlers.ts

+ * Assigns onKeyDown handler to the Component's part element, based on Component's actions
+ * and keys mappings defined in Accessibility behavior
+ * @param {AccessibilityActions} actions Actions defined in a component.
+ * @param {IAccessibilityDefinition} accessibility The input element which is to loose focus.


seems that description of this param is incorrect

seems like I am blind :) thank you, people, for reading it!

kuzhelov · 2018-09-06T12:38:43Z

src/components/Menu/MenuItem.tsx

        {childrenExist(children) ? (
          children
        ) : (
          <a
            className={cx('ui-menu__item__anchor', classes.anchor)}
            onClick={this.handleClick}
            {...accessibility.attributes.anchor}
+            {...accessibility.handlers.anchor}


handlers seems to be a name that quite loosely describes the purpose of this member (as the name is very generic) - so it is hard to get the intent of the client code by just reading it. Maybe we could make it more expressive by introducing name like keyHandlers, which it, in fact, is

agree, I'll rename it if everyone is ok with that.

kuzhelov · 2018-09-06T12:48:57Z

src/lib/UIComponent.tsx


 class UIComponent<P, S> extends React.Component<P, S> {
  private readonly childClass = this.constructor as typeof UIComponent
  static defaultProps: { [key: string]: any }
  static displayName: string
  static className: string
  static handledProps: any
+  protected actions: AccessibilityActions


I would suggest to make this name to be a bit more precise in terms of semantics. Here we are not defining the actions, but rather defining action handlers - the logic that should be performed by the component if specific action (or 'intent') has been raised by behavior. So, would suggest to rename to actionHandlers - this will also make it easy to read all the code snippets where this prop is used

I'm having a bit different point of view, you might disagree :)
A component has actions - a map of things it can do. They can be raised or not, based on behavior now. The behavior could even don't exist. But the intent Hey, I am component and can do next things which you can call stays. In some time we can change our mind, and replace behaviors with something else, but still component will have actions which can be called directly or indirectly - by adding it to keyboard handlers.

kuzhelov · 2018-09-06T13:03:30Z

src/lib/getKeyDownHandlers.ts

+ * @param {IAccessibilityDefinition} accessibility The input element which is to loose focus.
+ * @param {IState & IPropsWithVarsAndStyles} props The props which are used to invoke onKeyDown handler passed from top.
+ */
+const getKeyDownHandlers = (


here also would suggest to do couple of renamings, so that it would be easier to read:

actions -> componentActionHandlers

actionsDefinition -> behaviorActionDefinitions (which those, in fact, are)

elementName -> componentPart (this naming corresponds to the common semantics of anatomy)

currentActionDef -> componentPartActionDefinitions (as a consequence of previous renamings)

commonActions -> handledActions (as common term rather describes 'how' those are fetched, and not the intent - obtain actions that are supported/handled by component)

this will result in the following code - this one seems to be more readable, as well as its intent is much easier to grasp:

const getKeyDownHandlers = ( componentActionHandlers: AccessibilityActions, accessibility: IAccessibilityDefinition, props: IRenderConfigProps, ): ActionsHandler => { const keyDownHandlers = {} const behaviorActionDefinitions = accessibility.actionsDefinition if (!componentActionHandlers || !behaviorActionDefinitions) return keyDownHandlers for (const componentPartName in behaviorActionDefinitions) { const componentPartActionDefinitions = behaviorActionDefinitions[componentPartName] const handledActions = _.intersection(_.keys(componentPartActionDefinitions), _.keys(componentActionHandlers)) if (!handledActions.length) continue keyDownHandlers[componentPartName] = { onKeyDown: (event: React.KeyboardEvent) => { handledActions.forEach(actionName => { const eventHandler = keyboardHandlerFilter( componentActionHandlers[actionName], componentPartActionDefinitions[actionName].keyCombinations, ) eventHandler && eventHandler(event) }) _.invoke(props, 'onKeyDown', event) }, } } return keyDownHandlers }

please, let me know about your thoughts on that

Agree on this suggestions. But referring to comment above, I would make componentActionHandlers -> componentActions

kuzhelov · 2018-09-06T13:16:43Z

src/lib/getKeyDownHandlers.ts

+  props: IState & IPropsWithVarsAndStyles,
+): ActionsHandler => {
+  const handlers = {}
+  const actionsDefinition = accessibility.actionsDefinition


sorry, but still am haunted by thought of renaming actionsDefinition part for the following reasons:

those actions are currently provided for key events - it is better to reflect this in naming

it seems that we could achieve better consistency if we'll use more specific actions <-> handlers part

Regarding second point, wouldn't it be better to use accessibility.keyActions for defining them, and accessibility.keyHandlers for those handlers that were fetched from supported component's actions? This seems to be much more expressive, due to now we do immediately see (just by reading the code) that only key handling concept is handled by accessibility, as well as handlers are populated from general actionHandlers defined by component (this 'general' part is necessary, due to these component's action handlers could be consumed later to map actions from other sources, like joystick or whatever :)

kuzhelov · 2018-09-06T14:21:26Z

src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts

  handledProps: ['aria-label', 'aria-labelledby'],
+
+  actionsDefinition: {


this one should be keyActions

Sofiya Huts added 3 commits August 22, 2018 14:59

Introduce actions for menu items and add onKeyDown handler in renderProp

9943581

Change event type to React type

7869e37

Minor improvements

8f8c1bb

sophieH29 self-assigned this Aug 22, 2018

sophieH29 requested review from levithomason, miroslavstastny and jurokapsiar August 22, 2018 17:12

sophieH29 requested review from dvdzkwsk, kuzhelov and mnajdova as code owners August 22, 2018 17:12

kuzhelov added the 🚀 ready for review label Aug 22, 2018

jurokapsiar reviewed Aug 22, 2018

View reviewed changes

Move addKeyDownHandler function to separate file

afb05a7

miroslavstastny reviewed Aug 23, 2018

View reviewed changes

mnajdova reviewed Aug 23, 2018

View reviewed changes

Add unit tests, improvements after code review comments

1287307

smykhailov reviewed Aug 23, 2018

View reviewed changes

sophieH29 added the RFC label Aug 24, 2018

levithomason reviewed Aug 24, 2018

View reviewed changes

Make improvements after code review comments

3eede54

jurokapsiar reviewed Aug 27, 2018

View reviewed changes

Sofiya Huts and others added 5 commits August 27, 2018 16:44

Make improvements after code review comments

3faf1f2

Merge branch 'master' into feat/keydown-action-handlers

a2a67a2

Merge branch 'master' into feat/keydown-action-handlers

2d09377

Made improvement - move getKeyDownHandlers logic to accessebility beh…

577fcf3

…avior

Merge branch 'master' into feat/keydown-action-handlers

bca5891

Sofiya Huts and others added 4 commits August 31, 2018 10:29

Revert "Made improvement - move getKeyDownHandlers logic to accessebi…

3a6c40b

…lity behavior" This reverts commit 577fcf3.

Merge master into branch

3efd266

Merge branch 'master' into feat/keydown-action-handlers

3f062f9

Merge branch 'master' into feat/keydown-action-handlers

3dccb1c

miroslavstastny reviewed Sep 6, 2018

View reviewed changes

kuzhelov reviewed Sep 6, 2018

View reviewed changes

Merge branch 'master' into feat/keydown-action-handlers

47184bb

Fix description for method param # Conflicts: # src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts # src/lib/accessibility/interfaces.ts # src/lib/renderComponent.tsx

miroslavstastny approved these changes Sep 6, 2018

View reviewed changes

Merge branch 'master' into feat/keydown-action-handlers

d2cd31d

kuzhelov reviewed Sep 6, 2018

View reviewed changes

Improvement after CR comments

6c24800

kuzhelov reviewed Sep 6, 2018

View reviewed changes

sophieH29 added 3 commits September 6, 2018 16:28

Update MenuItemBehavior.ts

d90e4b8

Merge branch 'master' into feat/keydown-action-handlers

e8cf925

Update CHANGELOG.md

c7af853

sophieH29 merged commit 534829f into master Sep 7, 2018

sophieH29 deleted the feat/keydown-action-handlers branch September 7, 2018 09:01

@@ @@ -58,11 +60,47 @@ const getAccessibility = <P extends {}>(props, state) => { @@
                 })
               }
+              /**

		handledProps: ['aria-label', 'aria-labelledby'],

		actionsDefinition: {

[RFC]Feat(Accessibility): Add keyboard action handlers #121

[RFC]Feat(Accessibility): Add keyboard action handlers #121

Conversation

sophieH29 commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Aug 23, 2018

sophieH29 commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Aug 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophieH29 Aug 27, 2018 • edited

Choose a reason for hiding this comment

jurokapsiar Aug 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miroslavstastny left a comment

Choose a reason for hiding this comment

miroslavstastny Sep 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Sep 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Sep 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Sep 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2018 •

edited

levithomason Aug 24, 2018 •

edited

sophieH29 Aug 27, 2018 •

edited

jurokapsiar Aug 27, 2018 •

edited

miroslavstastny Sep 6, 2018 •

edited

kuzhelov Sep 6, 2018 •

edited

kuzhelov Sep 6, 2018 •

edited

kuzhelov Sep 6, 2018 •

edited