Merge pull request #1121 from riovir/fix/overlaysConfigChange

fix(overlays): sync config change to controller

.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.

.changeset/purple-beers-applaud.md

@@ -0,0 +1,5 @@
+---
+'@lion/overlays': patch
+---
+
+Return promises for the OverlayMixin toggle, open and close methods, since the OverlayController hiding/showing is asynchronous.

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;

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;
     });

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();
+      }
     }
 
     /**
@@ -319,22 +325,22 @@ export const OverlayMixinImplementation = superclass =>
     /**
      * Toggles the overlay
      */
-    toggle() {
-      /** @type {OverlayController} */ (this._overlayCtrl).toggle();
+    async toggle() {
+      await /** @type {OverlayController} */ (this._overlayCtrl).toggle();
     }
 
     /**
      * Shows the overlay
      */
-    open() {
-      /** @type {OverlayController} */ (this._overlayCtrl).show();
+    async open() {
+      await /** @type {OverlayController} */ (this._overlayCtrl).show();
     }
 
     /**
      * Hides the overlay
      */
-    close() {
-      /** @type {OverlayController} */ (this._overlayCtrl).hide();
+    async close() {
+      await /** @type {OverlayController} */ (this._overlayCtrl).hide();
     }
   };
 export const OverlayMixin = dedupeMixin(OverlayMixinImplementation);

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);
+}

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}>
+          <div slot="content">content</div>
+          <button slot="invoker">invoker button</button>
+        </${tag}>
+      `));
+      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}>
+          <div slot="content">content</div>
+          <button slot="invoker">invoker button</button>
+        </${tag}>
+      `));
+      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`, () => {

packages/overlays/test/OverlayController.test.js

@@ -920,9 +920,11 @@ describe('OverlayController', () => {
         expect(ctrl2.content).to.be.displayed;
 
         await ctrl3.show();
+        await ctrl3._showComplete;
         expect(ctrl3.content).to.be.displayed;
 
         await ctrl2.hide();
+        await ctrl2._hideComplete;
         expect(ctrl0.content).to.be.displayed;
         expect(ctrl1.content).to.be.displayed;
 

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);
+  });
+});

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