New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SafeAnchor a11y - Provide keydown handler for "space" #2697
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import elementType from 'prop-types-extra/lib/elementType'; | ||
import createChainedFunction from './utils/createChainedFunction'; | ||
|
||
const propTypes = { | ||
href: PropTypes.string, | ||
onClick: PropTypes.func, | ||
onKeyDown: PropTypes.func, | ||
disabled: PropTypes.bool, | ||
role: PropTypes.string, | ||
tabIndex: PropTypes.oneOfType([ | ||
|
@@ -36,6 +38,7 @@ class SafeAnchor extends React.Component { | |
super(props, context); | ||
|
||
this.handleClick = this.handleClick.bind(this); | ||
this.handleKeyDown = this.handleKeyDown.bind(this); | ||
} | ||
|
||
handleClick(event) { | ||
|
@@ -55,8 +58,18 @@ class SafeAnchor extends React.Component { | |
} | ||
} | ||
|
||
handleKeyDown(event) { | ||
event.preventDefault(); | ||
if (event.key === ' ') { | ||
this.handleClick(event); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should probably |
||
} | ||
} | ||
|
||
render() { | ||
const { componentClass: Component, disabled, ...props } = this.props; | ||
const { componentClass: Component, disabled, onKeyDown, ...props } = this.props; | ||
|
||
const handleKeyDown = Component === 'a' ? | ||
createChainedFunction(this.handleKeyDown, onKeyDown) : onKeyDown; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be more performant to do this in the constructor, but if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, sorry, i take that back – i forgot that using |
||
|
||
if (isTrivialHref(props.href)) { | ||
props.role = props.role || 'button'; | ||
|
@@ -74,6 +87,7 @@ class SafeAnchor extends React.Component { | |
<Component | ||
{...props} | ||
onClick={this.handleClick} | ||
onKeyDown={handleKeyDown} | ||
/> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,17 @@ describe('SafeAnchor', () => { | |
handleClick.should.have.been.calledOnce; | ||
}); | ||
|
||
it('provides onClick handler as onKeyDown handler for "space"', () => { | ||
const handleClick = sinon.spy(); | ||
|
||
tsp(<SafeAnchor onClick={handleClick} />) | ||
.shallowRender() | ||
.find('a') | ||
.trigger('keyDown', { key: ' ', preventDefault() {} }); | ||
|
||
handleClick.should.have.been.calledOnce; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're equivalent, I was following the pattern of the "it forwards onclick" test https://github.com/andy-j-d/react-bootstrap/blob/b8800dbba51e86ed4bdd34c046f820a67194029c/test/SafeAnchorSpec.js#L39 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah this is fine as long as we're not using assert – i prefer |
||
}); | ||
|
||
it('prevents default when no href is provided', () => { | ||
const handleClick = sinon.spy(); | ||
|
||
|
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.
only if key is space