fix(Accessibility): fix order of applying unhandled props and key han…

…dlers (#1901) * fix order of applying unhandled props and key handlers * UT and changelog * prettier * add test for correct merge order
microsoft · Sep 11, 2019 · 770538e · 770538e
1 parent 7860660
commit 770538e
Show file tree

Hide file tree

Showing 10 changed files with 88 additions and 37 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Fixes
 - Fix `Menu` and `Submenu` to use correct indicator icon and have correct width behavior [redlines] @bcalvery ([#1831](https://github.com/stardust-ui/react/pull/1831))
+- Fix order of applying unhandled props and key handlers @jurokapsiar ([#1901](https://github.com/stardust-ui/react/pull/1901)) 
 - Fix handling of `onMouseEnter` prop in `ChatMessage` @layershifter ([#1903](https://github.com/stardust-ui/react/pull/1903))
 - Fix `CreateShorthandOptions` should be typed @lucivpav ([#1886](https://github.com/stardust-ui/react/pull/1886))
 

diff --git a/packages/react/src/components/Button/Button.tsx b/packages/react/src/components/Button/Button.tsx
@@ -142,9 +142,9 @@ class Button extends UIComponent<WithAsProp<ButtonProps>> {
         onClick={this.handleClick}
         onFocus={this.handleFocus}
         {...accessibility.attributes.root}
-        {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
         {...rtlTextContainer.getAttributes({ forElements: [children] })}
         {...unhandledProps}
+        {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
       >
         {hasChildren && children}
         {!hasChildren && loading && this.renderLoader(variables, styles)}

diff --git a/packages/react/src/components/Embed/Embed.tsx b/packages/react/src/components/Embed/Embed.tsx
@@ -131,8 +131,8 @@ class Embed extends AutoControlledComponent<WithAsProp<EmbedProps>, EmbedState>
         className={classes.root}
         onClick={this.handleClick}
         {...accessibility.attributes.root}
-        {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
         {...unhandledProps}
+        {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
       >
         {active ? (
           <>

diff --git a/packages/react/src/components/HierarchicalTree/HierarchicalTreeItem.tsx b/packages/react/src/components/HierarchicalTree/HierarchicalTreeItem.tsx
@@ -198,8 +198,8 @@ class HierarchicalTreeItem extends UIComponent<WithAsProp<HierarchicalTreeItemPr
           className={classes.root}
           {...accessibility.attributes.root}
           {...rtlTextContainer.getAttributes({ forElements: [children] })}
-          {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
           {...unhandledProps}
+          {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
         >
           {childrenExist(children) ? children : this.renderContent()}
         </ElementType>

diff --git a/packages/react/src/components/Slider/Slider.tsx b/packages/react/src/components/Slider/Slider.tsx
@@ -179,12 +179,7 @@ class Slider extends AutoControlledComponent<WithAsProp<SliderProps>, SliderStat
 
     // we need 2 wrappers around the slider rail, track, input and thumb slots to achieve correct component sizes
     return (
-      <ElementType
-        className={classes.root}
-        {...accessibility.attributes.root}
-        {...restProps}
-        {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
-      >
+      <ElementType className={classes.root} {...accessibility.attributes.root} {...restProps}>
         <div className={cx(Slider.slotClassNames.inputWrapper, classes.inputWrapper)}>
           <span className={cx(Slider.slotClassNames.rail, classes.rail)} />
           <span

diff --git a/packages/react/src/components/Tree/TreeItem.tsx b/packages/react/src/components/Tree/TreeItem.tsx
@@ -213,8 +213,8 @@ class TreeItem extends UIComponent<WithAsProp<TreeItemProps>, TreeItemState> {
         className={classes.root}
         {...accessibility.attributes.root}
         {...rtlTextContainer.getAttributes({ forElements: [children] })}
-        {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
         {...unhandledProps}
+        {...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
       >
         {childrenExist(children) ? children : this.renderContent()}
       </ElementType>

diff --git a/packages/react/test/specs/commonTests/eventTarget.ts b/packages/react/test/specs/commonTests/eventTarget.ts
@@ -0,0 +1,28 @@
+import { ReactWrapper } from 'enzyme'
+
+export const EVENT_TARGET_ATTRIBUTE = 'data-simulate-event-here'
+
+export const getEventTargetComponent = (
+  wrapper: ReactWrapper,
+  listenerName: string,
+  eventTargets?: object = {},
+) => {
+  const eventTarget = eventTargets[listenerName]
+    ? wrapper
+        .find(eventTargets[listenerName])
+        .hostNodes()
+        .first()
+    : wrapper
+        .find(`[${EVENT_TARGET_ATTRIBUTE}]`)
+        .hostNodes()
+        .first()
+
+  // if (eventTarget.length === 0) {
+  //   throw new Error(
+  //     'The event prop was not delegated to the children. You probably ' +
+  //     'forgot to use `getUnhandledProps` util to spread the `unhandledProps` props.',
+  //   )
+  // }
+
+  return eventTarget
+}
diff --git a/packages/react/test/specs/commonTests/handlesAccessibility.tsx b/packages/react/test/specs/commonTests/handlesAccessibility.tsx
@@ -1,9 +1,12 @@
 import * as React from 'react'
+import * as keyboardKey from 'keyboard-key'
 
-import { mountWithProviderAndGetComponent } from 'test/utils'
+import { mountWithProviderAndGetComponent, mountWithProvider } from 'test/utils'
 import { Accessibility, AriaRole, FocusZoneMode } from 'src/lib/accessibility/types'
 import { FocusZone } from 'src/lib/accessibility/FocusZone'
 import { FOCUSZONE_WRAP_ATTRIBUTE } from 'src/lib/accessibility/FocusZone/focusUtilities'
+import { UIComponent } from 'src/lib'
+import { EVENT_TARGET_ATTRIBUTE, getEventTargetComponent } from './eventTarget'
 
 export const getRenderedAttribute = (renderedComponent, propName, partSelector) => {
   const target = partSelector
@@ -106,6 +109,46 @@ export default (
       const role = getRenderedAttribute(rendered, 'role', partSelector)
       expect(role).toBe(testRole)
     })
+
+    test(`handles "onKeyDown" overrides`, () => {
+      const actionHandler = jest.fn()
+      const eventHandler = jest.fn()
+
+      const actionBehavior: Accessibility = () => ({
+        keyActions: {
+          root: {
+            mockAction: {
+              keyCombinations: [{ keyCode: keyboardKey.Enter }],
+            },
+          },
+        },
+      })
+
+      const wrapperProps = {
+        ...requiredProps,
+        accessibility: actionBehavior,
+        [EVENT_TARGET_ATTRIBUTE]: true,
+        onKeyDown: eventHandler,
+      }
+
+      const wrapper = mountWithProvider(<Component {...wrapperProps} />)
+      const component = wrapper.find(Component)
+      const instance = component.instance() as UIComponent<any, any>
+      if (instance.actionHandlers) {
+        instance.actionHandlers.mockAction = actionHandler
+      }
+      // Force render component to apply updated key handlers
+      wrapper.setProps({})
+
+      getEventTargetComponent(component, 'onKeyDown').simulate('keydown', {
+        keyCode: keyboardKey.Enter,
+      })
+
+      if (instance.actionHandlers) {
+        expect(actionHandler).toBeCalledTimes(1)
+      }
+      expect(eventHandler).toBeCalledTimes(1)
+    })
   }
 
   if (focusZoneDefinition) {

diff --git a/packages/react/test/specs/commonTests/isConformant.tsx b/packages/react/test/specs/commonTests/isConformant.tsx
@@ -20,6 +20,7 @@ import * as stardust from 'src/index'
 import { Accessibility, AriaRole } from 'src/lib/accessibility/types'
 import { FocusZone, IS_FOCUSABLE_ATTRIBUTE } from 'src/lib/accessibility/FocusZone'
 import { FOCUSZONE_WRAP_ATTRIBUTE } from 'src/lib/accessibility/FocusZone/focusUtilities'
+import { getEventTargetComponent, EVENT_TARGET_ATTRIBUTE } from './eventTarget'
 
 export interface Conformant {
   eventTargets?: object
@@ -85,27 +86,6 @@ export default (Component, options: Conformant = {}) => {
     return wrapperComponent ? toNextNonTrivialChild(componentElement) : componentElement
   }
 
-  const getEventTargetComponent = (wrapper: ReactWrapper, listenerName: string) => {
-    const eventTarget = eventTargets[listenerName]
-      ? wrapper
-          .find(eventTargets[listenerName])
-          .hostNodes()
-          .first()
-      : wrapper
-          .find('[data-simulate-event-here]')
-          .hostNodes()
-          .first()
-
-    // if (eventTarget.length === 0) {
-    //   throw new Error(
-    //     'The event prop was not delegated to the children. You probably ' +
-    //     'forgot to use `getUnhandledProps` util to spread the `unhandledProps` props.',
-    //   )
-    // }
-
-    return eventTarget
-  }
-
   // make sure components are properly exported
   if (componentType !== 'function') {
     throwError(`Components should export a class or function, got: ${componentType}.`)
@@ -365,12 +345,12 @@ export default (Component, options: Conformant = {}) => {
 
         const wrapperProps = {
           ...requiredProps,
-          'data-simulate-event-here': true,
+          [EVENT_TARGET_ATTRIBUTE]: true,
           [listenerName]: handler,
         }
         const wrapper = mount(<Component {...wrapperProps} />)
 
-        getEventTargetComponent(wrapper, listenerName).simulate(eventName)
+        getEventTargetComponent(wrapper, listenerName, eventTargets).simulate(eventName)
         expect(handler).toBeCalledTimes(1)
       })
     })
@@ -398,11 +378,11 @@ export default (Component, options: Conformant = {}) => {
         const props = {
           ...requiredProps,
           [listenerName]: handlerSpy,
-          'data-simulate-event-here': true,
+          [EVENT_TARGET_ATTRIBUTE]: true,
         }
 
         const component = mount(<Component {...props} />)
-        const eventTarget = getEventTargetComponent(component, listenerName)
+        const eventTarget = getEventTargetComponent(component, listenerName, eventTargets)
         const customHandler: Function = eventTarget.prop(listenerName)
 
         if (customHandler) {

diff --git a/packages/react/test/specs/components/Embed/Embed-test.tsx b/packages/react/test/specs/components/Embed/Embed-test.tsx
@@ -1,11 +1,15 @@
 import * as React from 'react'
 import Embed from 'src/components/Embed/Embed'
-import { isConformant } from 'test/specs/commonTests'
+import { isConformant, handlesAccessibility } from 'test/specs/commonTests'
 import { mountWithProviderAndGetComponent } from 'test/utils'
 
 describe('Embed', () => {
   isConformant(Embed)
 
+  describe('accessibility', () => {
+    handlesAccessibility(Embed, { defaultRootRole: 'presentation' })
+  })
+
   describe('onClick', () => {
     test('is called with (e, props) on a click', () => {
       const onClick = jest.fn()