Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(tooltip): fix tooltip and popover not closing on escape #6108 #6177

Closed
wants to merge 1 commit into from
Closed

fix(tooltip): fix tooltip and popover not closing on escape #6108 #6177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/tooltip/test/tooltip.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('tooltip', function() {
}));

afterEach(function() {
$document.off('keypress');
$document.off('keyup');
});

function trigger(element, evt) {
Expand Down Expand Up @@ -295,10 +295,9 @@ describe('tooltip', function() {
trigger(elm, 'mouseenter');
expect(tooltipScope.isOpen).toBe(false);

$timeout.flush(500);
expect(tooltipScope.isOpen).toBe(false);
elmScope.disabled = true;
elmScope.$digest();
$timeout.flush(500);

expect(tooltipScope.isOpen).toBe(false);
});
Expand Down Expand Up @@ -343,22 +342,24 @@ describe('tooltip', function() {
expect(tooltipScope.isOpen).toBe(true);
expect(tooltipScope2.isOpen).toBe(true);

var evt = $.Event('keypress');
var evt = $.Event('keyup');
evt.which = 27;

$document.trigger(evt);
tooltipScope.$digest();
tooltipScope2.$digest();
$timeout.flush();

expect(tooltipScope.isOpen).toBe(true);
expect(tooltipScope2.isOpen).toBe(false);

var evt2 = $.Event('keypress');
var evt2 = $.Event('keyup');
evt2.which = 27;

$document.trigger(evt2);
tooltipScope.$digest();
tooltipScope2.$digest();
$timeout.flush(500);

expect(tooltipScope.isOpen).toBe(false);
expect(tooltipScope2.isOpen).toBe(false);
Expand Down
16 changes: 9 additions & 7 deletions src/tooltip/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary? Seems like it could still be kept on keypress and work fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really weird - is this only with jQuery loaded? I haven't seen this problem when it is just angular with no jQuery.

Copy link
Contributor Author

@iamfozzy iamfozzy Aug 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. Another issue where keypress was the problem and replacing with keydown solves: #5013

Have you tried with the plunker?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, your Plunker does seem to indicate it being an issue. I guess I'm ok with this change then.


$rootScope.$on('$destroy', function() {
$document.off('keypress', keypressListener);
$document.off('keyup', keypressListener);
});

function keypressListener(e) {
if (e.which === 27) {
var last = openedTooltips.top();
if (last) {
last.value.close();
openedTooltips.removeTop();
last = null;
}
}
Expand Down Expand Up @@ -207,9 +206,6 @@ angular.module('ui.bootstrap.tooltip', ['ui.bootstrap.position', 'ui.bootstrap.s
// By default, the tooltip is not open.
// TODO add ability to start tooltip opened
ttScope.isOpen = false;
openedTooltips.add(ttScope, {
close: hide
});

function toggleTooltipBind() {
if (!ttScope.isOpen) {
Expand Down Expand Up @@ -336,6 +332,10 @@ angular.module('ui.bootstrap.tooltip', ['ui.bootstrap.position', 'ui.bootstrap.s
}
});

openedTooltips.add(ttScope, {
close: hide
});

prepObservers();
}

Expand All @@ -348,6 +348,9 @@ angular.module('ui.bootstrap.tooltip', ['ui.bootstrap.position', 'ui.bootstrap.s
tooltip.remove();
tooltip = null;
}

openedTooltips.remove(ttScope);

if (tooltipLinkedScope) {
tooltipLinkedScope.$destroy();
tooltipLinkedScope = null;
Expand Down Expand Up @@ -563,7 +566,6 @@ angular.module('ui.bootstrap.tooltip', ['ui.bootstrap.position', 'ui.bootstrap.s
scope.$on('$destroy', function onDestroyTooltip() {
unregisterTriggers();
removeTooltip();
openedTooltips.remove(ttScope);
ttScope = null;
});
};
Expand Down