Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(Accessibility): fix order of applying unhandled props and key handlers #1901

Merged
merged 7 commits into from
Sep 11, 2019

Conversation

jurokapsiar
Copy link
Contributor

Fixes #1899

Key handlers need to be applied after unhandled props so that onKeyDown is not overwritten but correctly respects both the key handlers from behaviors as well as onKeyDown passed by the consumer or by a wrapping component (popup)

@jurokapsiar jurokapsiar changed the title fix order of applying unhandled props and key handlers fix(Accessibility): fix order of applying unhandled props and key handlers Sep 9, 2019
@codecov
Copy link

codecov bot commented Sep 9, 2019

Codecov Report

Merging #1901 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1901      +/-   ##
==========================================
+ Coverage    69.9%   69.97%   +0.06%     
==========================================
  Files         894      895       +1     
  Lines        7796     7814      +18     
  Branches     2251     2281      +30     
==========================================
+ Hits         5450     5468      +18     
  Misses       2336     2336              
  Partials       10       10
Impacted Files Coverage Δ
packages/react/src/components/Button/Button.tsx 71.42% <ø> (ø) ⬆️
packages/react/src/components/Embed/Embed.tsx 95.65% <ø> (ø) ⬆️
packages/react/src/components/Tree/TreeItem.tsx 71.42% <ø> (ø) ⬆️
...mponents/HierarchicalTree/HierarchicalTreeItem.tsx 75% <ø> (ø) ⬆️
packages/react/src/components/Slider/Slider.tsx 91.17% <ø> (ø) ⬆️
...ckages/react/test/specs/commonTests/eventTarget.ts 100% <100%> (ø)
...ages/react/test/specs/commonTests/isConformant.tsx 92.15% <100%> (-0.19%) ⬇️
...ct/test/specs/commonTests/handlesAccessibility.tsx 85% <100%> (+3.46%) ⬆️

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 7860660...11ceb0a. Read the comment docs.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Let's add an entry to CHANGELOG
  2. Let's add a proper UT, may be it will catch additional cases (under isConformant):
    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,
        'data-simulate-event-here': true,
        onKeyDown: eventHandler,
      }
      const wrapper = mount(<Component {...wrapperProps} />)

      ;(wrapper.instance() as UIComponent<any, any>).actionHandlers.mockAction = actionHandler
      // Force render component to apply updated key handlers
      wrapper.setProps({})

      getEventTargetComponent(wrapper, 'onKeyDown').simulate(
        'keydown',
        { keyCode: keyboardKey.Enter },
      )

      expect(actionHandler).toBeCalledTimes(1)
      expect(eventHandler).toBeCalledTimes(1)
    })

@layershifter layershifter added needs author changes Author needs to implement changes before merge 🧰 fix Introduces fix for broken behavior. and removed 🚀 ready for review labels Sep 9, 2019
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix CI before merge 🚀

{...unhandledProps}
{...applyAccessibilityKeyHandlers(accessibility.keyHandlers.root, unhandledProps)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct applyAccessibilityKeyHandlers should always be spread after the unhandledProps, as it will invoke the key events provided by the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, there's now even a test verifying that for all compoennts (that have the actions defined)

@jurokapsiar jurokapsiar deleted the fix/key-handlers-unhandled-props branch September 11, 2019 18:35
layershifter pushed a commit that referenced this pull request Sep 13, 2019
…dlers (#1901)

* fix order of applying unhandled props and key handlers

* UT and changelog

* prettier

* add test for correct merge order

(cherry picked from commit 770538e)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embed does not apply key handlers correctly
3 participants