From 0e8886648184e38e82926db55fb22deae1cab258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Wed, 6 Apr 2022 23:00:57 +0200 Subject: [PATCH 1/4] fix: prevent dialog from closing when moving within the DOM --- packages/dialog/src/vaadin-dialog.js | 8 +++++++- packages/dialog/test/dialog.test.js | 14 ++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/packages/dialog/src/vaadin-dialog.js b/packages/dialog/src/vaadin-dialog.js index 73e1457e695..d5806f0d62b 100644 --- a/packages/dialog/src/vaadin-dialog.js +++ b/packages/dialog/src/vaadin-dialog.js @@ -569,7 +569,13 @@ class Dialog extends ThemePropertyMixin(ElementMixin(DialogDraggableMixin(Dialog /** @protected */ disconnectedCallback() { super.disconnectedCallback(); - this.opened = false; + // Delay closing the dialog in case it, or an ancestor, was just + // attached to a different node + setTimeout(() => { + if (!this.isConnected) { + this.opened = false; + } + }); } /** @private */ diff --git a/packages/dialog/test/dialog.test.js b/packages/dialog/test/dialog.test.js index 1a2fa036892..53aaf72491c 100644 --- a/packages/dialog/test/dialog.test.js +++ b/packages/dialog/test/dialog.test.js @@ -1,5 +1,5 @@ import { expect } from '@esm-bundle/chai'; -import { esc, fixtureSync } from '@vaadin/testing-helpers'; +import { aTimeout, esc, fixtureSync } from '@vaadin/testing-helpers'; import '@vaadin/polymer-legacy-adapter/template-renderer.js'; import '../vaadin-dialog.js'; import { html, PolymerElement } from '@polymer/polymer/polymer-element.js'; @@ -137,10 +137,20 @@ describe('vaadin-dialog', () => { }); describe('detaching', () => { - it('should close the overlay when detached', () => { + it('should close the overlay when detached', async () => { dialog.parentNode.removeChild(dialog); + await aTimeout(0); expect(dialog.opened).to.be.false; }); + + it('should not close the overlay when moved within the DOM', async () => { + const newParent = document.createElement('div'); + document.body.appendChild(newParent); + + newParent.appendChild(dialog); + await aTimeout(0); + expect(dialog.opened).to.be.true; + }); }); describe('modeless', () => { From 02287bd2bd7772895f8d000657fdb67d6e6867ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Mon, 11 Apr 2022 10:56:01 +0200 Subject: [PATCH 2/4] refactor to remove timeout --- packages/dialog/src/vaadin-dialog.js | 19 ++++++++++++------- packages/dialog/test/dialog.test.js | 4 +--- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/dialog/src/vaadin-dialog.js b/packages/dialog/src/vaadin-dialog.js index d5806f0d62b..bd4391d5331 100644 --- a/packages/dialog/src/vaadin-dialog.js +++ b/packages/dialog/src/vaadin-dialog.js @@ -566,16 +566,21 @@ class Dialog extends ThemePropertyMixin(ElementMixin(DialogDraggableMixin(Dialog this.$.overlay.setProperties({ owner: this, renderer, headerRenderer, footerRenderer }); } + /** @protected */ + connectedCallback() { + super.connectedCallback(); + // Restore opened state if overlay was opened when disconnecting + if (this.__restoreOpened) { + this.opened = true; + } + } + /** @protected */ disconnectedCallback() { super.disconnectedCallback(); - // Delay closing the dialog in case it, or an ancestor, was just - // attached to a different node - setTimeout(() => { - if (!this.isConnected) { - this.opened = false; - } - }); + // Close overlay and memorize opened state + this.__restoreOpened = this.opened; + this.opened = false; } /** @private */ diff --git a/packages/dialog/test/dialog.test.js b/packages/dialog/test/dialog.test.js index 53aaf72491c..df8336595ae 100644 --- a/packages/dialog/test/dialog.test.js +++ b/packages/dialog/test/dialog.test.js @@ -1,5 +1,5 @@ import { expect } from '@esm-bundle/chai'; -import { aTimeout, esc, fixtureSync } from '@vaadin/testing-helpers'; +import { esc, fixtureSync } from '@vaadin/testing-helpers'; import '@vaadin/polymer-legacy-adapter/template-renderer.js'; import '../vaadin-dialog.js'; import { html, PolymerElement } from '@polymer/polymer/polymer-element.js'; @@ -139,7 +139,6 @@ describe('vaadin-dialog', () => { describe('detaching', () => { it('should close the overlay when detached', async () => { dialog.parentNode.removeChild(dialog); - await aTimeout(0); expect(dialog.opened).to.be.false; }); @@ -148,7 +147,6 @@ describe('vaadin-dialog', () => { document.body.appendChild(newParent); newParent.appendChild(dialog); - await aTimeout(0); expect(dialog.opened).to.be.true; }); }); From e16b6ac96a0fafa1c60695f4153cd73006d5e4ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Mon, 11 Apr 2022 10:56:25 +0200 Subject: [PATCH 3/4] fix login overlay closing when moving within DOM --- packages/login/src/vaadin-login-overlay.js | 8 ++++++++ packages/login/test/login-overlay.test.js | 12 ++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/packages/login/src/vaadin-login-overlay.js b/packages/login/src/vaadin-login-overlay.js index 950fa020a32..3d4cd2cdcd1 100644 --- a/packages/login/src/vaadin-login-overlay.js +++ b/packages/login/src/vaadin-login-overlay.js @@ -127,6 +127,11 @@ class LoginOverlay extends LoginMixin(ElementMixin(ThemableMixin(PolymerElement) this.$.vaadinLoginOverlayWrapper.addEventListener('vaadin-overlay-outside-click', this._preventClosingLogin); this.$.vaadinLoginOverlayWrapper.addEventListener('vaadin-overlay-escape-press', this._preventClosingLogin); + + // Restore opened state if overlay was open when disconnecting + if (this.__restoreOpened) { + this.$.vaadinLoginOverlayWrapper.opened = true; + } } /** @protected */ @@ -135,6 +140,9 @@ class LoginOverlay extends LoginMixin(ElementMixin(ThemableMixin(PolymerElement) this.$.vaadinLoginOverlayWrapper.removeEventListener('vaadin-overlay-outside-click', this._preventClosingLogin); this.$.vaadinLoginOverlayWrapper.removeEventListener('vaadin-overlay-escape-press', this._preventClosingLogin); + + // Close overlay and memorize opened state + this.__restoreOpened = this.$.vaadinLoginOverlayWrapper.opened; this.$.vaadinLoginOverlayWrapper.opened = false; } diff --git a/packages/login/test/login-overlay.test.js b/packages/login/test/login-overlay.test.js index eeea2aece5f..3c1f2beecfe 100644 --- a/packages/login/test/login-overlay.test.js +++ b/packages/login/test/login-overlay.test.js @@ -43,12 +43,20 @@ describe('opened overlay', () => { it('should set opened using attribute', () => { expect(overlay.opened).to.be.true; - expect(document.querySelector('vaadin-login-form-wrapper')).to.be.ok; + expect(document.querySelector('vaadin-login-form-wrapper')).to.exist; }); it('should remove form wrapper when closed', () => { overlay.opened = false; - expect(document.querySelector('vaadin-login-form-wrapper')).to.be.not.ok; + expect(document.querySelector('vaadin-login-form-wrapper')).not.to.exist; + }); + + it('should not remove form wrapper when moved within DOM', () => { + const newParent = document.createElement('div'); + document.body.appendChild(newParent); + newParent.appendChild(overlay); + + expect(document.querySelector('vaadin-login-form-wrapper')).to.exist; }); it('should propagate theme to a wrapper', () => { From 49b5c9627a46c2e08bdf662b2969e1e783c00483 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Mon, 11 Apr 2022 11:09:52 +0200 Subject: [PATCH 4/4] make tests sync --- packages/dialog/test/dialog.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dialog/test/dialog.test.js b/packages/dialog/test/dialog.test.js index df8336595ae..dd5ee962e4d 100644 --- a/packages/dialog/test/dialog.test.js +++ b/packages/dialog/test/dialog.test.js @@ -137,12 +137,12 @@ describe('vaadin-dialog', () => { }); describe('detaching', () => { - it('should close the overlay when detached', async () => { + it('should close the overlay when detached', () => { dialog.parentNode.removeChild(dialog); expect(dialog.opened).to.be.false; }); - it('should not close the overlay when moved within the DOM', async () => { + it('should not close the overlay when moved within the DOM', () => { const newParent = document.createElement('div'); document.body.appendChild(newParent);