From f7b95d03d3cab83ab99b2f7b85100cfb715a40b8 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Sep 2018 16:17:30 -0700 Subject: [PATCH 1/5] fixes bug with keyboard input in click tooltips Co-Authored-By: David Wilson --- src/tooltip-manager.js | 3 +-- src/tooltip.js | 9 +++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/tooltip-manager.js b/src/tooltip-manager.js index 9779e78fc..106df415f 100644 --- a/src/tooltip-manager.js +++ b/src/tooltip-manager.js @@ -153,11 +153,10 @@ class TooltipManager { } window.addEventListener('resize', hideTooltip) - window.addEventListener('keydown', hideTooltip, { capture: true }) const disposable = new Disposable(() => { window.removeEventListener('resize', hideTooltip) - window.removeEventListener('keydown', hideTooltip, { capture: true }) + hideTooltip() tooltip.destroy() diff --git a/src/tooltip.js b/src/tooltip.js index 3f21cb11a..e6960a8ef 100644 --- a/src/tooltip.js +++ b/src/tooltip.js @@ -82,6 +82,7 @@ Tooltip.prototype.init = function (element, options) { var eventIn, eventOut if (trigger === 'hover') { + this.hideOnKeydownOutsideOfTooltip = () => this.hide() if (this.options.selector) { eventIn = 'mouseover' eventOut = 'mouseout' @@ -220,6 +221,10 @@ Tooltip.prototype.show = function () { window.addEventListener('click', this.hideOnClickOutsideOfTooltip, true) } + if (this.hideOnKeydownOutsideOfTooltip) { + window.addEventListener('keydown', this.hideOnKeydownOutsideOfTooltip, true) + } + var tip = this.getTooltipElement() this.startObservingMutations() var tipId = this.getUID('tooltip') @@ -359,6 +364,10 @@ Tooltip.prototype.hide = function (callback) { window.removeEventListener('click', this.hideOnClickOutsideOfTooltip, true) } + if (this.hideOnKeydownOutsideOfTooltip) { + window.removeEventListener('keydown', this.hideOnKeydownOutsideOfTooltip, true) + } + this.tip && this.tip.classList.remove('in') this.stopObservingMutations() From 9e6b4b413bd8f4dc782ef9a4b4fa6c9c722fba31 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Sep 2018 17:02:05 -0700 Subject: [PATCH 2/5] add unit tests --- spec/tooltip-manager-spec.js | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/spec/tooltip-manager-spec.js b/spec/tooltip-manager-spec.js index 184f47bd3..adf684361 100644 --- a/spec/tooltip-manager-spec.js +++ b/spec/tooltip-manager-spec.js @@ -55,6 +55,18 @@ describe('TooltipManager', () => { disposables.dispose() }) + + it('hides the tooltip on keydown events', () => { + const disposable = manager.add(element, { title: 'Title', trigger: 'hover' }) + hover(element, function () { + expect(document.body.querySelector('.tooltip')).not.toBeNull() + window.dispatchEvent(new CustomEvent('keydown', { + bubbles: true + })) + expect(document.body.querySelector('.tooltip')).toBeNull() + disposable.dispose() + }) + }) }) describe("when the trigger is 'manual'", () => @@ -93,6 +105,16 @@ describe('TooltipManager', () => { }) ) + it('does not hide the tooltip on keyboard input', () => { + manager.add(element, {title: 'Title', trigger: 'click'}) + element.click() + expect(document.body.querySelector('.tooltip')).not.toBeNull() + window.dispatchEvent(new CustomEvent('keydown', { + bubbles: true + })) + expect(document.body.querySelector('.tooltip')).not.toBeNull() + }) + it('allows a custom item to be specified for the content of the tooltip', () => { const tooltipElement = document.createElement('div') manager.add(element, {item: {element: tooltipElement}}) @@ -213,20 +235,6 @@ describe('TooltipManager', () => { }) ) - describe('when a user types', () => - it('hides the tooltips', () => { - const disposable = manager.add(element, { title: 'Title' }) - hover(element, function () { - expect(document.body.querySelector('.tooltip')).not.toBeNull() - window.dispatchEvent(new CustomEvent('keydown', { - bubbles: true - })) - expect(document.body.querySelector('.tooltip')).toBeNull() - disposable.dispose() - }) - }) - ) - describe('findTooltips', () => { it('adds and remove tooltips correctly', () => { expect(manager.findTooltips(element).length).toBe(0) From dc1f6a02f39a7f95c0a53295e6c2c96236f10ed6 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Sep 2018 16:24:46 -0700 Subject: [PATCH 3/5] fix leaked state in tests. --- spec/tooltip-manager-spec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/tooltip-manager-spec.js b/spec/tooltip-manager-spec.js index adf684361..e6ca01de2 100644 --- a/spec/tooltip-manager-spec.js +++ b/spec/tooltip-manager-spec.js @@ -113,6 +113,9 @@ describe('TooltipManager', () => { bubbles: true })) expect(document.body.querySelector('.tooltip')).not.toBeNull() + // click again to hide the tooltip because otherwise state leaks + // into other tests. + element.click() }) it('allows a custom item to be specified for the content of the tooltip', () => { From 51b873345fe89e0a994dcb50ba1ed5243efe8d7e Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Sep 2018 16:26:58 -0700 Subject: [PATCH 4/5] add comment advising against adding window listeners in tooltip manager --- src/tooltip-manager.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tooltip-manager.js b/src/tooltip-manager.js index 106df415f..fa0cb1557 100644 --- a/src/tooltip-manager.js +++ b/src/tooltip-manager.js @@ -152,6 +152,7 @@ class TooltipManager { tooltip.hide() } + // note: adding a listener here adds a new listener for every tooltip element that's registered. Adding unnecessary listeners is bad for performance. It would be better to add/remove listeners when tooltips are actually created in the dom. window.addEventListener('resize', hideTooltip) const disposable = new Disposable(() => { From 945b57e2ca114d564404cfbbf19a1b739ef16fc2 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Sep 2018 17:09:38 -0700 Subject: [PATCH 5/5] :shirt: --- src/tooltip-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tooltip-manager.js b/src/tooltip-manager.js index fa0cb1557..d2d42763d 100644 --- a/src/tooltip-manager.js +++ b/src/tooltip-manager.js @@ -152,7 +152,7 @@ class TooltipManager { tooltip.hide() } - // note: adding a listener here adds a new listener for every tooltip element that's registered. Adding unnecessary listeners is bad for performance. It would be better to add/remove listeners when tooltips are actually created in the dom. + // note: adding a listener here adds a new listener for every tooltip element that's registered. Adding unnecessary listeners is bad for performance. It would be better to add/remove listeners when tooltips are actually created in the dom. window.addEventListener('resize', hideTooltip) const disposable = new Disposable(() => {