diff --git a/.changeset/dirty-students-smile.md b/.changeset/dirty-students-smile.md new file mode 100644 index 000000000..5bc785169 --- /dev/null +++ b/.changeset/dirty-students-smile.md @@ -0,0 +1,5 @@ +--- +'@lion/overlays': patch +--- + +Sync config change to the controller and make sure that an overlay is re-opened/re-hidden after the updateConfig handling, instead of always closed. diff --git a/packages/input-datepicker/test-helpers/DatepickerInputObject.js b/packages/input-datepicker/test-helpers/DatepickerInputObject.js index 4da2b8a4c..ee8c12a84 100644 --- a/packages/input-datepicker/test-helpers/DatepickerInputObject.js +++ b/packages/input-datepicker/test-helpers/DatepickerInputObject.js @@ -33,7 +33,7 @@ export class DatepickerInputObject { * @param {number} day */ async selectMonthDay(day) { - this.overlayController.show(); + await this.overlayController.show(); await this.calendarEl.updateComplete; this.calendarObj.getDayEl(day).click(); return true; diff --git a/packages/overlays/src/OverlayController.js b/packages/overlays/src/OverlayController.js index 2fb72cbbc..eeb23dbca 100644 --- a/packages/overlays/src/OverlayController.js +++ b/packages/overlays/src/OverlayController.js @@ -657,6 +657,11 @@ export class OverlayController extends EventTargetShim { * @param {HTMLElement} elementToFocusAfterHide */ async show(elementToFocusAfterHide = this.elementToFocusAfterHide) { + // Subsequent shows could happen, make sure we await it first. + // Otherwise it gets replaced before getting resolved, and places awaiting it will time out. + if (this._showComplete) { + await this._showComplete; + } this._showComplete = new Promise(resolve => { this._showResolve = resolve; }); diff --git a/packages/overlays/src/OverlayMixin.js b/packages/overlays/src/OverlayMixin.js index 543ba2362..5cb671585 100644 --- a/packages/overlays/src/OverlayMixin.js +++ b/packages/overlays/src/OverlayMixin.js @@ -1,5 +1,6 @@ import { dedupeMixin } from '@lion/core'; import { OverlayController } from './OverlayController.js'; +import { isEqualConfig } from './utils/is-equal-config.js'; /** * @typedef {import('../types/OverlayConfig').OverlayConfig} OverlayConfig @@ -43,10 +44,15 @@ export const OverlayMixinImplementation = superclass => /** @param {OverlayConfig} value */ set config(value) { - if (this._overlayCtrl) { + const shouldUpdate = !isEqualConfig(this.config, value); + + if (this._overlayCtrl && shouldUpdate) { this._overlayCtrl.updateConfig(value); } this.__config = value; + if (this._overlayCtrl && shouldUpdate) { + this.__syncToOverlayController(); + } } /** diff --git a/packages/overlays/src/utils/is-equal-config.js b/packages/overlays/src/utils/is-equal-config.js new file mode 100644 index 000000000..7e0051ecf --- /dev/null +++ b/packages/overlays/src/utils/is-equal-config.js @@ -0,0 +1,24 @@ +/** + * @typedef {import('../../types/OverlayConfig').OverlayConfig} OverlayConfig + */ + +/** + * Compares two OverlayConfigs to equivalence. Intended to prevent unnecessary resets. + * Note that it doesn't cover as many use cases as common implementations, such as Lodash isEqual. + * + * @param {OverlayConfig} a + * @param {OverlayConfig} b + * @returns {boolean} Whether the configs are equivalent + */ +export function isEqualConfig(a, b) { + if (typeof a !== 'object' || typeof a !== 'object') { + return a === b; + } + const aProps = Object.keys(a); + const bProps = Object.keys(b); + if (aProps.length !== bProps.length) { + return false; + } + const isEqual = /** @param {string} prop */ prop => isEqualConfig(a[prop], b[prop]); + return aProps.every(isEqual); +} diff --git a/packages/overlays/test-suites/OverlayMixin.suite.js b/packages/overlays/test-suites/OverlayMixin.suite.js index 483eca504..bc8f63038 100644 --- a/packages/overlays/test-suites/OverlayMixin.suite.js +++ b/packages/overlays/test-suites/OverlayMixin.suite.js @@ -114,6 +114,7 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) { await itEl.updateComplete; expect(itEl._overlayCtrl.trapsKeyboardFocus).to.be.false; + await nextFrame(); itEl.config = { viewportConfig: { placement: 'left' } }; expect(itEl._overlayCtrl.viewportConfig.placement).to.equal('left'); }); @@ -235,6 +236,44 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) { await nextFrame(); expect(el.opened).to.be.false; }); + + /** See: https://github.com/ing-bank/lion/issues/1075 */ + it('stays open after config update', async () => { + const el = /** @type {OverlayEl} */ (await fixture(html` + <${tag}> +
content
+ + + `)); + el.open(); + await el._overlayCtrl._showComplete; + el.config = { ...el.config, hidesOnOutsideClick: !el.config.hidesOnOutsideClick }; + await nextFrame(); + expect(el.opened).to.be.true; + expect(getComputedStyle(el._overlayCtrl.contentWrapperNode).display).not.to.equal('none'); + }); + + /** Prevent unnecessary reset side effects, such as show animation. See: https://github.com/ing-bank/lion/issues/1075 */ + it('does not call updateConfig on equivalent config change', async () => { + const el = /** @type {OverlayEl} */ (await fixture(html` + <${tag}> +
content
+ + + `)); + el.open(); + await nextFrame(); + + const stub = sinon.stub(el._overlayCtrl, 'updateConfig'); + stub.callsFake(() => { + throw new Error('Unexpected config update'); + }); + + expect(() => { + el.config = { ...el.config }; + }).to.not.throw; + stub.restore(); + }); }); describe(`OverlayMixin${suffix} nested`, () => { diff --git a/packages/overlays/test/utils-tests/is-equal-config.test.js b/packages/overlays/test/utils-tests/is-equal-config.test.js new file mode 100644 index 000000000..d6e1b78f1 --- /dev/null +++ b/packages/overlays/test/utils-tests/is-equal-config.test.js @@ -0,0 +1,72 @@ +import { expect } from '@open-wc/testing'; +import { isEqualConfig } from '../../src/utils/is-equal-config.js'; + +function TestConfig() { + return { + placementMode: 'local', + hidesOnOutsideClick: true, + popperConfig: { + modifiers: { + offset: { + enabled: false, + }, + }, + }, + }; +} + +/** Used for detecting if it's safe to disregard OverlayConfig changes with equal value. */ +describe('isEqualConfig()', () => { + it('returns true for same reference', () => { + const config = TestConfig(); + expect(isEqualConfig(config, config)).eql(true); + }); + + it('compares shallow props', () => { + const config = TestConfig(); + expect(isEqualConfig(config, { ...config })).eql(true); + const differentConfig = { ...config, hidesOnOutsideClick: !config.hidesOnOutsideClick }; + expect(isEqualConfig(config, differentConfig)).eql(false); + }); + + it('compares prop count', () => { + const config = TestConfig(); + expect(isEqualConfig(config, { ...config, extra: 'value' })).eql(false); + expect(isEqualConfig({ ...config, extra: 'value' }, config)).eql(false); + }); + + it('regards missing props different from ones with undefined value', () => { + const config = TestConfig(); + expect(isEqualConfig(config, { ...config, extra: undefined })).eql(false); + }); + + it('compares nested props', () => { + const config = TestConfig(); + const sameConfig = { + ...config, + popperConfig: { + ...config.popperConfig, + modifiers: { + ...config.popperConfig.modifiers, + offset: { + ...config.popperConfig.modifiers.offset, + }, + }, + }, + }; + expect(isEqualConfig(config, sameConfig)).eql(true); + const differentConfig = { + ...config, + popperConfig: { + ...config.popperConfig, + modifiers: { + ...config.popperConfig.modifiers, + offset: { + enabled: !config.popperConfig.modifiers.offset.enabled, + }, + }, + }, + }; + expect(isEqualConfig(config, differentConfig)).eql(false); + }); +}); diff --git a/packages/select-rich/test/lion-select-rich.test.js b/packages/select-rich/test/lion-select-rich.test.js index e6b9cd0dd..5696d2c07 100644 --- a/packages/select-rich/test/lion-select-rich.test.js +++ b/packages/select-rich/test/lion-select-rich.test.js @@ -353,6 +353,7 @@ describe('lion-select-rich', () => { el.opened = true; await el.updateComplete; await el.updateComplete; // safari takes a little longer + await el._overlayCtrl._showComplete; // noDefaultSelected will now flip the override back to what was the initial reference width // @ts-ignore allow protected access in tests