SafeAnchor a11y - Provide keydown handler for "space" #2697
Conversation
If `onKeyDown` prop is supplied, prefer that If no `onKeyDown` supplied, handle "space" with the `onClick` handler. Test that space bar calls click handler.
src/SafeAnchor.js
Outdated
| @@ -55,8 +58,14 @@ class SafeAnchor extends React.Component { | |||
| } | |||
| } | |||
|
|
|||
| handleKeyDown(event) { | |||
| if (keycode(event) === 'space') { | |||
src/SafeAnchor.js
Outdated
| @@ -74,6 +83,7 @@ class SafeAnchor extends React.Component { | |||
| <Component | |||
| {...props} | |||
| onClick={this.handleClick} | |||
| onKeyDown={onKeyDown || this.handleKeyDown} | |||
src/SafeAnchor.js
Outdated
| @@ -55,8 +58,14 @@ class SafeAnchor extends React.Component { | |||
| } | |||
| } | |||
|
|
|||
| handleKeyDown(event) { | |||
| if (keycode(event) === 'space') { | |||
| this.handleClick(event); | |||
There was a problem hiding this comment.
we should probably preventDefault here regardless to prevent scrolling?
src/SafeAnchor.js
Outdated
| @@ -74,6 +83,7 @@ class SafeAnchor extends React.Component { | |||
| <Component | |||
| {...props} | |||
| onClick={this.handleClick} | |||
| onKeyDown={onKeyDown || this.handleKeyDown} | |||
There was a problem hiding this comment.
we don't need to do this if the element is actually <button>
Chain own-props `onKeyDown` with space handler
|
👍 @taion |
src/SafeAnchor.js
Outdated
| const { componentClass: Component, disabled, onKeyDown, ...props } = this.props; | ||
|
|
||
| const handleKeyDown = Component === 'a' ? | ||
| createChainedFunction(this.handleKeyDown, onKeyDown) : onKeyDown; |
There was a problem hiding this comment.
It would be more performant to do this in the constructor, but if the onKeyDown prop were changed after the component mounted, the handler wouldn't update. Don't see another way to do this without breaking backwards compatibility.
There was a problem hiding this comment.
oops, sorry, i take that back – i forgot that using <button> was handled upstream in <Button>. you don't need to check for a here
src/SafeAnchor.js
Outdated
| @@ -55,8 +58,18 @@ class SafeAnchor extends React.Component { | |||
| } | |||
| } | |||
|
|
|||
| handleKeyDown(event) { | |||
| event.preventDefault(); | |||
src/SafeAnchor.js
Outdated
| const { componentClass: Component, disabled, onKeyDown, ...props } = this.props; | ||
|
|
||
| const handleKeyDown = Component === 'a' ? | ||
| createChainedFunction(this.handleKeyDown, onKeyDown) : onKeyDown; |
There was a problem hiding this comment.
oops, sorry, i take that back – i forgot that using <button> was handled upstream in <Button>. you don't need to check for a here
| .find('a') | ||
| .trigger('keyDown', { key: ' ', preventDefault() {} }); | ||
|
|
||
| handleClick.should.have.been.calledOnce; |
There was a problem hiding this comment.
expect(handleClick).to.have.been.calledOnce ?
There was a problem hiding this comment.
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.
yeah this is fine as long as we're not using assert – i prefer expect but consistency has some value
| .find('a') | ||
| .trigger('keyDown', { key: ' ', preventDefault() {} }); | ||
|
|
||
| handleClick.should.have.been.calledOnce; |
There was a problem hiding this comment.
yeah this is fine as long as we're not using assert – i prefer expect but consistency has some value
|
@jquense thoughts? |
IfonKeyDownprop is supplied, prefer that.If SafeAnchor renders an
atag, supply onKeyDown handler for "space" that calls theonClickhandler.Test that space bar calls click handler.
Addresses #2681