-
Notifications
You must be signed in to change notification settings - Fork 55
fix(Accessibility): fix order of applying unhandled props and key handlers #1901
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Let's add an entry to CHANGELOG
- 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)
})
There was a problem hiding this 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)} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
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)