fix(ngAria): bind to keydown instead of keypress in ngClick
#14065
Conversation
The ngAria keyboard support for html elements used as button now binds to the keydown event instead of keypress to better emulate behavior of native buttons. BREAKING CHANGE: The ngAria configuration flag that controls binding to keyboard input has been renamed from `bindKeypress` to `bindKeydown` so that it accurately dscribes the implemented behavior. Closes #14063
|
|
||
| expect(clickFn).toHaveBeenCalledWith('div'); | ||
| expect(clickFn).toHaveBeenCalledWith('li'); | ||
| }); | ||
|
|
||
| it('should not override existing ng-keypress', function() { |
There was a problem hiding this comment.
Don't remove this test, just change it to use keydown instead.
| @@ -364,8 +364,8 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) { | |||
| elem.attr('tabindex', 0); | |||
| } | |||
|
|
|||
| if ($aria.config('bindKeypress') && !attr.ngKeypress) { | |||
| elem.on('keypress', function(event) { | |||
| if ($aria.config('bindKeydown') && !attr.ngKeypress) { | |||
There was a problem hiding this comment.
attr.ngKeypress should be also changed to attr.ngKeydown.
I wonder if it's a good idea to check for both (ngKeydown and ngKeypress).
|
Thank you for your feedback @gkalpak, I've implemented changes as you suggested. |
|
There is one remaining occurrence of And I still wonder if we should not do anything if the element does already have an I believe it would be best to leave them alone in that case, since they seem to know what they're doing. |
|
The more I think about it, the more I agree with you. It's clear the developer already has specific behavior in mind, that they have already implemented. Also, our willingness in this PR to switch from one key event to another signifies our opinion that in this context, they are not significantly different. It seems reasonable that we would not bind to key events if the user has already defined any custom key event behavior. |
|
@gkalpak any other feedback here? |
|
@LeeAdcock, I didn't have time to take a look yet - will do in the next days. |
|
Looks good to me other than the documentation comment @gkalpak pointed out. I think it should just say "keydown event" since that's all it wires up–not keydown and keyup. |
|
I would change |
|
I made the necessary changes and merged 🎉 |
The ngAria keyboard support for html elements used as button now binds to the keydown
event instead of keypress to better emulate behavior of native buttons.
BREAKING CHANGE: The ngAria configuration flag that controls binding to keyboard input
has been renamed from
bindKeypresstobindKeydownso that it accurately describes theimplemented behavior.
Closes #14063