From 6318367dd228e3cd637612fbcac9234b4273b712 Mon Sep 17 00:00:00 2001 From: Sune Simonsen Date: Thu, 13 Sep 2018 15:52:40 +0200 Subject: [PATCH 1/4] fix: attach and detach click outside handlers when menu open and closes --- .../menus/src/containers/MenuContainer.js | 12 +++++- .../src/containers/MenuContainer.spec.js | 38 +++++++++++++++---- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/packages/menus/src/containers/MenuContainer.js b/packages/menus/src/containers/MenuContainer.js index d849bcfd83e..6f9c62eb248 100644 --- a/packages/menus/src/containers/MenuContainer.js +++ b/packages/menus/src/containers/MenuContainer.js @@ -150,18 +150,22 @@ class MenuContainer extends ControlledComponent { }; } - componentDidMount() { + attachClickOutsideHandler() { this.getDocuments().forEach(doc => { doc.addEventListener('mousedown', this.handleOutsideMouseDown); }); } - componentWillUnmount() { + detachClickOutsideHandler() { this.getDocuments().forEach(doc => { doc.removeEventListener('mousedown', this.handleOutsideMouseDown); }); } + componentWillUnmount() { + this.detachClickOutsideHandler(); + } + componentDidUpdate(prevProps, prevState) { const { isOpen, closedByBlur } = this.getControlledState(); const previousisOpen = this.isControlledProp('isOpen') ? prevProps.isOpen : prevState.isOpen; @@ -173,6 +177,8 @@ class MenuContainer extends ControlledComponent { if (!closedByBlur) { this.triggerReference && this.triggerReference.focus(); } + + this.detachClickOutsideHandler(); } /** @@ -192,6 +198,8 @@ class MenuContainer extends ControlledComponent { setTimeout(() => { this.menuReference && this.menuReference.focus(); }, 0); + + this.attachClickOutsideHandler(); } } diff --git a/packages/menus/src/containers/MenuContainer.spec.js b/packages/menus/src/containers/MenuContainer.spec.js index b5d4a63827c..45bc0cf6717 100644 --- a/packages/menus/src/containers/MenuContainer.spec.js +++ b/packages/menus/src/containers/MenuContainer.spec.js @@ -254,17 +254,41 @@ describe('MenuContainer', () => { }); }); - describe('componentWillUnmount', () => { - it('removes mousedown event listener', () => { - const removeEventListenerSpy = jest.fn(); - - document.removeEventListener = removeEventListenerSpy; + describe('when the menu is open', () => { + beforeEach(() => { wrapper = mountWithTheme(basicExample({ onChange: onChangeSpy }), { enzymeOptions: { attachTo: document.body } }); - wrapper.unmount(); - expect(removeEventListenerSpy).toHaveBeenCalled(); + findTrigger(wrapper).simulate('click'); + }); + + describe('when clicking outside', () => { + it('closes the menu', () => { + wrapper.simulate('click'); + + expect(findMenu(wrapper)).not.toExist(); + }); + + it('removes click outside event listener', () => { + const removeEventListenerSpy = jest.fn(); + + document.removeEventListener = removeEventListenerSpy; + wrapper.simulate('click'); + + expect(removeEventListenerSpy).toHaveBeenCalled(); + }); + }); + + describe('componentWillUnmount()', () => { + it('removes click outside event listener', () => { + const removeEventListenerSpy = jest.fn(); + + document.removeEventListener = removeEventListenerSpy; + wrapper.unmount(); + + expect(removeEventListenerSpy).toHaveBeenCalled(); + }); }); }); From 3d11c19fc279ef0a9b4f40d8b32782918e8882b0 Mon Sep 17 00:00:00 2001 From: Sune Simonsen Date: Thu, 13 Sep 2018 15:56:27 +0200 Subject: [PATCH 2/4] fix: move click outside handlers to the event capturing phase This will allow to beat other event handlers that calls `event.stopPropagation()`. --- packages/menus/src/containers/MenuContainer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/menus/src/containers/MenuContainer.js b/packages/menus/src/containers/MenuContainer.js index 6f9c62eb248..2b032de41f4 100644 --- a/packages/menus/src/containers/MenuContainer.js +++ b/packages/menus/src/containers/MenuContainer.js @@ -152,13 +152,13 @@ class MenuContainer extends ControlledComponent { attachClickOutsideHandler() { this.getDocuments().forEach(doc => { - doc.addEventListener('mousedown', this.handleOutsideMouseDown); + doc.addEventListener('mousedown', this.handleOutsideMouseDown, true); }); } detachClickOutsideHandler() { this.getDocuments().forEach(doc => { - doc.removeEventListener('mousedown', this.handleOutsideMouseDown); + doc.removeEventListener('mousedown', this.handleOutsideMouseDown, true); }); } From a5444ced751a337927e6b127df9172f2074f4427 Mon Sep 17 00:00:00 2001 From: Sune Simonsen Date: Thu, 13 Sep 2018 16:44:53 +0200 Subject: [PATCH 3/4] Make sure we restore the document.removeEventListener spy after each test --- .../menus/src/containers/MenuContainer.spec.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/menus/src/containers/MenuContainer.spec.js b/packages/menus/src/containers/MenuContainer.spec.js index 45bc0cf6717..e24907398e3 100644 --- a/packages/menus/src/containers/MenuContainer.spec.js +++ b/packages/menus/src/containers/MenuContainer.spec.js @@ -256,6 +256,8 @@ describe('MenuContainer', () => { describe('when the menu is open', () => { beforeEach(() => { + jest.spyOn(document, 'removeEventListener'); + wrapper = mountWithTheme(basicExample({ onChange: onChangeSpy }), { enzymeOptions: { attachTo: document.body } }); @@ -263,6 +265,10 @@ describe('MenuContainer', () => { findTrigger(wrapper).simulate('click'); }); + afterEach(() => { + document.removeEventListener.mockRestore(); + }); + describe('when clicking outside', () => { it('closes the menu', () => { wrapper.simulate('click'); @@ -271,23 +277,17 @@ describe('MenuContainer', () => { }); it('removes click outside event listener', () => { - const removeEventListenerSpy = jest.fn(); - - document.removeEventListener = removeEventListenerSpy; wrapper.simulate('click'); - expect(removeEventListenerSpy).toHaveBeenCalled(); + expect(document.removeEventListener).toHaveBeenCalled(); }); }); describe('componentWillUnmount()', () => { it('removes click outside event listener', () => { - const removeEventListenerSpy = jest.fn(); - - document.removeEventListener = removeEventListenerSpy; wrapper.unmount(); - expect(removeEventListenerSpy).toHaveBeenCalled(); + expect(document.removeEventListener).toHaveBeenCalled(); }); }); }); From 80931e0be8295d403ce6577c92deb307a7466fe7 Mon Sep 17 00:00:00 2001 From: Sune Simonsen Date: Thu, 13 Sep 2018 22:33:07 +0200 Subject: [PATCH 4/4] Document use capture boolean argument for event handlers --- packages/menus/src/containers/MenuContainer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/menus/src/containers/MenuContainer.js b/packages/menus/src/containers/MenuContainer.js index 2b032de41f4..9f3413b4a41 100644 --- a/packages/menus/src/containers/MenuContainer.js +++ b/packages/menus/src/containers/MenuContainer.js @@ -152,13 +152,13 @@ class MenuContainer extends ControlledComponent { attachClickOutsideHandler() { this.getDocuments().forEach(doc => { - doc.addEventListener('mousedown', this.handleOutsideMouseDown, true); + doc.addEventListener('mousedown', this.handleOutsideMouseDown, true /* use capture */); }); } detachClickOutsideHandler() { this.getDocuments().forEach(doc => { - doc.removeEventListener('mousedown', this.handleOutsideMouseDown, true); + doc.removeEventListener('mousedown', this.handleOutsideMouseDown, true /* use capture */); }); }