From b483cfd4d3a7acda743b2adf22abc717d76fed53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Broutier?= Date: Mon, 12 Sep 2022 15:51:11 +0200 Subject: [PATCH 1/7] Fix modal event listeners (#37126) Add a check to ensure that the element that triggered the event is still present in the DOM. --- js/src/modal.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/js/src/modal.js b/js/src/modal.js index c977225388dd..afa729559b80 100644 --- a/js/src/modal.js +++ b/js/src/modal.js @@ -224,6 +224,10 @@ class Modal extends BaseComponent { EventHandler.on(this._element, EVENT_MOUSEDOWN_DISMISS, event => { EventHandler.one(this._element, EVENT_CLICK_DISMISS, event2 => { + if (!document.body.contains(event.target)) { + return; + } + // a bad trick to segregate clicks that may start inside dialog but end outside, and avoid listen to scrollbar clicks if (this._dialog.contains(event.target) || this._dialog.contains(event2.target)) { return From df21232388486302e493285ae5d0057e7601e98b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Broutier?= Date: Thu, 15 Sep 2022 10:05:26 +0200 Subject: [PATCH 2/7] Add unit test. --- js/tests/unit/modal.spec.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/js/tests/unit/modal.spec.js b/js/tests/unit/modal.spec.js index e774fc4e8e55..74f49b7aa076 100644 --- a/js/tests/unit/modal.spec.js +++ b/js/tests/unit/modal.spec.js @@ -734,6 +734,30 @@ describe('Modal', () => { }) }) + it('should not close modal when clicking on an element removed from modal content', () => { + return new Promise(resolve => { + fixtureEl.innerHTML = '
' + + const modalEl = fixtureEl.querySelector('.modal') + const buttonEl = modalEl.querySelector('.btn') + const modal = new Modal(modalEl) + + const spy = spyOn(modal, 'hide') + + modalEl.addEventListener('shown.bs.modal', () => { + modalEl.dispatchEvent(createEvent('mousedown')) + buttonEl.addEventListener('click', () => { + buttonEl.remove() + }) + buttonEl.click() + expect(spy).not.toHaveBeenCalled() + resolve() + }) + + modal.show() + }) + }) + it('should do nothing is the modal is not shown', () => { fixtureEl.innerHTML = '
' From 39e349d05e3fa6d1cfc27c0d642022247a12db76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Broutier?= Date: Thu, 15 Sep 2022 10:20:23 +0200 Subject: [PATCH 3/7] Check both events instead of just one. --- js/src/modal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/modal.js b/js/src/modal.js index afa729559b80..819dce05f678 100644 --- a/js/src/modal.js +++ b/js/src/modal.js @@ -224,7 +224,7 @@ class Modal extends BaseComponent { EventHandler.on(this._element, EVENT_MOUSEDOWN_DISMISS, event => { EventHandler.one(this._element, EVENT_CLICK_DISMISS, event2 => { - if (!document.body.contains(event.target)) { + if (!document.body.contains(event.target) || !document.body.contains(event2.target)) { return; } From 40250e6a42ed9afee8372984080721f06790af80 Mon Sep 17 00:00:00 2001 From: GeoSot Date: Thu, 15 Sep 2022 11:41:56 +0300 Subject: [PATCH 4/7] lint --- js/src/modal.js | 5 +++-- js/tests/unit/modal.spec.js | 14 ++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/js/src/modal.js b/js/src/modal.js index 819dce05f678..1d5fc717a2ca 100644 --- a/js/src/modal.js +++ b/js/src/modal.js @@ -224,8 +224,9 @@ class Modal extends BaseComponent { EventHandler.on(this._element, EVENT_MOUSEDOWN_DISMISS, event => { EventHandler.one(this._element, EVENT_CLICK_DISMISS, event2 => { - if (!document.body.contains(event.target) || !document.body.contains(event2.target)) { - return; + // handle click on removed elements + if (!this._element.contains(event2.target)) { + return } // a bad trick to segregate clicks that may start inside dialog but end outside, and avoid listen to scrollbar clicks diff --git a/js/tests/unit/modal.spec.js b/js/tests/unit/modal.spec.js index 74f49b7aa076..fdee29e95abd 100644 --- a/js/tests/unit/modal.spec.js +++ b/js/tests/unit/modal.spec.js @@ -736,19 +736,25 @@ describe('Modal', () => { it('should not close modal when clicking on an element removed from modal content', () => { return new Promise(resolve => { - fixtureEl.innerHTML = '
' + fixtureEl.innerHTML = [ + '
', + '
', + ' ', + '
', + '
' + ].join('') const modalEl = fixtureEl.querySelector('.modal') const buttonEl = modalEl.querySelector('.btn') const modal = new Modal(modalEl) const spy = spyOn(modal, 'hide') + buttonEl.addEventListener('click', () => { + buttonEl.remove() + }) modalEl.addEventListener('shown.bs.modal', () => { modalEl.dispatchEvent(createEvent('mousedown')) - buttonEl.addEventListener('click', () => { - buttonEl.remove() - }) buttonEl.click() expect(spy).not.toHaveBeenCalled() resolve() From f3f89ee4a19ae41a5a1d2860ea446da6a987e857 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Broutier?= Date: Thu, 15 Sep 2022 11:25:24 +0200 Subject: [PATCH 5/7] Use strict comparison instead of contains. --- js/src/modal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/modal.js b/js/src/modal.js index 1d5fc717a2ca..9b79633faacf 100644 --- a/js/src/modal.js +++ b/js/src/modal.js @@ -225,7 +225,7 @@ class Modal extends BaseComponent { EventHandler.on(this._element, EVENT_MOUSEDOWN_DISMISS, event => { EventHandler.one(this._element, EVENT_CLICK_DISMISS, event2 => { // handle click on removed elements - if (!this._element.contains(event2.target)) { + if (this._element !== event.target || this._element !== event2.target) { return } From ece7f444b602405c175e23f39c2c543a4a6c1efc Mon Sep 17 00:00:00 2001 From: GeoSot Date: Thu, 15 Sep 2022 13:18:19 +0300 Subject: [PATCH 6/7] remove redundant check --- js/src/modal.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/js/src/modal.js b/js/src/modal.js index 9b79633faacf..78bbfdcd474c 100644 --- a/js/src/modal.js +++ b/js/src/modal.js @@ -229,11 +229,6 @@ class Modal extends BaseComponent { return } - // a bad trick to segregate clicks that may start inside dialog but end outside, and avoid listen to scrollbar clicks - if (this._dialog.contains(event.target) || this._dialog.contains(event2.target)) { - return - } - if (this._config.backdrop === 'static') { this._triggerBackdropTransition() return From 41f664f90b1ca7db925c0277c313fa90a3b8b29d Mon Sep 17 00:00:00 2001 From: GeoSot Date: Thu, 15 Sep 2022 13:27:36 +0300 Subject: [PATCH 7/7] add comment --- js/src/modal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/modal.js b/js/src/modal.js index 78bbfdcd474c..a39597b3a8ef 100644 --- a/js/src/modal.js +++ b/js/src/modal.js @@ -223,8 +223,8 @@ class Modal extends BaseComponent { }) EventHandler.on(this._element, EVENT_MOUSEDOWN_DISMISS, event => { + // a bad trick to segregate clicks that may start inside dialog but end outside, and avoid listen to scrollbar clicks EventHandler.one(this._element, EVENT_CLICK_DISMISS, event2 => { - // handle click on removed elements if (this._element !== event.target || this._element !== event2.target) { return }