fix(tooltip): fix tooltip and popover not closing on escape #6108 #6177
Conversation
iamfozzy
force-pushed
the
fix/tooltip-close-on-escape
branch
from
f4a43fd to
6ddddc3
Compare
| @@ -71,18 +71,17 @@ angular.module('ui.bootstrap.tooltip', ['ui.bootstrap.position', 'ui.bootstrap.s | |||
| */ | |||
| this.$get = ['$window', '$compile', '$timeout', '$document', '$uibPosition', '$interpolate', '$rootScope', '$parse', '$$stackedMap', function($window, $compile, $timeout, $document, $position, $interpolate, $rootScope, $parse, $$stackedMap) { | |||
| var openedTooltips = $$stackedMap.createNew(); | |||
| $document.on('keypress', keypressListener); | |||
| $document.on('keyup', keypressListener); | |||
There was a problem hiding this comment.
Is this change necessary? Seems like it could still be kept on keypress and work fine.
There was a problem hiding this comment.
I've just run a quick test in chrome and keypress when pressing esc is not being captured, whereas keyup is.
If you goto http://plnkr.co/edit/WVTd5prw08Mj7tF6vEI4?p=info and place a breakpoint on uibs.js:4720 in chrome then open a popup and press escape. For me - that method is never fired when using keypress.
Comment here #6108 (comment)
The below also comes to mind:
angular/angular.js#10905
http://stackoverflow.com/questions/3901063/jquerys-keypress-doesnt-work-for-some-keys-in-chrome-how-to-work-around
There was a problem hiding this comment.
That's really weird - is this only with jQuery loaded? I haven't seen this problem when it is just angular with no jQuery.
There was a problem hiding this comment.
Not sure. Another issue where keypress was the problem and replacing with keydown solves: #5013
Have you tried with the plunker?
There was a problem hiding this comment.
Yeah, your Plunker does seem to indicate it being an issue. I guess I'm ok with this change then.
|
The crux of this change feels correct to me with the movement of where the new tooltip is added, but I'm not certain that the keypress listener needs to be changed to a keyup listener - if those changes are removed, I'd be inclined to merge it. Many thanks for this PR! I have investigated this a few different times, and felt that there was something wrong with when the tooltip was added, but could not place my finger on exactly what was happening. |
|
This LGTM - thanks a lot! |
This PR fixes issue #6108