fix(field): make sure RegistrationSystem works well with ShadyDom

packages/field/index.js

@@ -4,3 +4,5 @@ export { FormatMixin } from './src/FormatMixin.js';
 export { FormControlMixin } from './src/FormControlMixin.js';
 export { InteractionStateMixin } from './src/InteractionStateMixin.js'; // applies FocusMixin
 export { LionField } from './src/LionField.js';
+export { FormRegisteringMixin } from './src/FormRegisteringMixin.js';
+export { FormRegistrarMixin } from './src/FormRegistrarMixin.js';

packages/field/src/FormControlMixin.js

@@ -1,5 +1,6 @@
 import { html, css, nothing, dedupeMixin, SlotMixin } from '@lion/core';
 import { ObserverMixin } from '@lion/core/src/ObserverMixin.js';
+import { FormRegisteringMixin } from './FormRegisteringMixin.js';
 
 /**
  * #FormControlMixin :
@@ -14,7 +15,7 @@ import { ObserverMixin } from '@lion/core/src/ObserverMixin.js';
 export const FormControlMixin = dedupeMixin(
   superclass =>
     // eslint-disable-next-line no-shadow, no-unused-vars
-    class FormControlMixin extends ObserverMixin(SlotMixin(superclass)) {
+    class FormControlMixin extends FormRegisteringMixin(ObserverMixin(SlotMixin(superclass))) {
       static get properties() {
         return {
           ...super.properties,
@@ -105,8 +106,6 @@ export const FormControlMixin = dedupeMixin(
         super.connectedCallback();
         this._enhanceLightDomClasses();
         this._enhanceLightDomA11y();
-        this._registerFormElement();
-        this._requestParentFormGroupUpdateOfResetModelValue();
       }
 
       /**
@@ -150,42 +149,6 @@ export const FormControlMixin = dedupeMixin(
         this._enhanceLightDomA11yForAdditionalSlots();
       }
 
-      /**
-       * Fires a registration event in the next frame.
-       *
-       * Why next frame?
-       * if ShadyDOM is used and you add a listener and fire the event in the same frame
-       * it will not bubble and there can not be cought by a parent element
-       * for more details see: https://github.com/Polymer/lit-element/issues/658
-       * will requires a `await nextFrame()` in tests
-       */
-      _registerFormElement() {
-        this.updateComplete.then(() => {
-          this.dispatchEvent(
-            new CustomEvent('form-element-register', {
-              detail: { element: this },
-              bubbles: true,
-            }),
-          );
-        });
-      }
-
-      /**
-       * Makes sure our parentFormGroup has the most up to date resetModelValue
-       * FormGroups will call the same on their parentFormGroup so the full tree gets the correct
-       * values.
-       *
-       * Why next frame?
-       * @see {@link this._registerFormElement}
-       */
-      _requestParentFormGroupUpdateOfResetModelValue() {
-        this.updateComplete.then(() => {
-          if (this.__parentFormGroup) {
-            this.__parentFormGroup._updateResetModelValue();
-          }
-        });
-      }
-
       /**
        * Enhances additional slots(prefix, suffix, before, after) defined by developer.
        *

packages/field/src/FormRegisteringMixin.js

@@ -0,0 +1,71 @@
+import { dedupeMixin } from '@lion/core';
+import { formRegistrarManager } from './formRegistrarManager.js';
+
+/**
+ * #FormRegisteringMixin:
+ *
+ * This Mixin registers a form element to a Registrar
+ *
+ * @polymerMixin
+ * @mixinFunction
+ */
+export const FormRegisteringMixin = dedupeMixin(
+  superclass =>
+    // eslint-disable-next-line no-shadow, no-unused-vars
+    class FormRegisteringMixin extends superclass {
+      connectedCallback() {
+        if (super.connectedCallback) {
+          super.connectedCallback();
+        }
+        this.__setupRegistrationHook();
+      }
+
+      disconnectedCallback() {
+        if (super.disconnectedCallback) {
+          super.disconnectedCallback();
+        }
+        this._unregisterFormElement();
+      }
+
+      __setupRegistrationHook() {
+        if (formRegistrarManager.ready) {
+          this._registerFormElement();
+        } else {
+          formRegistrarManager.addEventListener('all-forms-open-for-registration', () => {
+            this._registerFormElement();
+          });
+        }
+      }
+
+      _registerFormElement() {
+        this._dispatchRegistration();
+        this._requestParentFormGroupUpdateOfResetModelValue();
+      }
+
+      _dispatchRegistration() {
+        this.dispatchEvent(
+          new CustomEvent('form-element-register', {
+            detail: { element: this },
+            bubbles: true,
+          }),
+        );
+      }
+
+      _unregisterFormElement() {
+        if (this.__parentFormGroup) {
+          this.__parentFormGroup.removeFormElement(this);
+        }
+      }
+
+      /**
+       * Makes sure our parentFormGroup has the most up to date resetModelValue
+       * FormGroups will call the same on their parentFormGroup so the full tree gets the correct
+       * values.
+       */
+      _requestParentFormGroupUpdateOfResetModelValue() {
+        if (this.__parentFormGroup && this.__parentFormGroup._updateResetModelValue) {
+          this.__parentFormGroup._updateResetModelValue();
+        }
+      }
+    },
+);

packages/field/src/FormRegistrarMixin.js

@@ -0,0 +1,92 @@
+import { dedupeMixin } from '@lion/core';
+import { formRegistrarManager } from './formRegistrarManager.js';
+import { FormRegisteringMixin } from './FormRegisteringMixin.js';
+
+/**
+ * This allows an element to become the manager of a register
+ */
+export const FormRegistrarMixin = dedupeMixin(
+  superclass =>
+    // eslint-disable-next-line no-shadow, no-unused-vars
+    class FormRegistrarMixin extends FormRegisteringMixin(superclass) {
+      get formElements() {
+        return this.__formElements;
+      }
+
+      set formElements(value) {
+        this.__formElements = value;
+      }
+
+      get formElementsArray() {
+        return this.__formElements;
+      }
+
+      constructor() {
+        super();
+        this.formElements = [];
+        this.__readyForRegistration = false;
+        this.registrationReady = new Promise(resolve => {
+          this.__resolveRegistrationReady = resolve;
+        });
+        formRegistrarManager.add(this);
+
+        this._onRequestToAddFormElement = this._onRequestToAddFormElement.bind(this);
+        this.addEventListener('form-element-register', this._onRequestToAddFormElement);
+      }
+
+      isRegisteredFormElement(el) {
+        return this.formElementsArray.some(exitingEl => exitingEl === el);
+      }
+
+      firstUpdated(changedProperties) {
+        super.firstUpdated(changedProperties);
+        this.__resolveRegistrationReady();
+        this.__readyForRegistration = true;
+        formRegistrarManager.becomesReady(this);
+      }
+
+      addFormElement(child) {
+        // This is a way to let the child element (a lion-fieldset or lion-field) know, about its parent
+        // eslint-disable-next-line no-param-reassign
+        child.__parentFormGroup = this;
+
+        this.formElements.push(child);
+      }
+
+      removeFormElement(child) {
+        const index = this.formElements.indexOf(child);
+        if (index > -1) {
+          this.formElements.splice(index, 1);
+        }
+      }
+
+      _onRequestToAddFormElement(ev) {
+        const child = ev.detail.element;
+        if (child === this) {
+          // as we fire and listen - don't add ourselves
+          return;
+        }
+        if (this.isRegisteredFormElement(child)) {
+          // do not readd already existing elements
+          return;
+        }
+        ev.stopPropagation();
+        this.addFormElement(child);
+      }
+
+      _onRequestToRemoveFormElement(ev) {
+        const child = ev.detail.element;
+        if (child === this) {
+          // as we fire and listen - don't add ourselves
+          return;
+        }
+        if (!this.isRegisteredFormElement(child)) {
+          // do not readd already existing elements
+          return;
+        }
+        ev.stopPropagation();
+
+        this.removeFormElement(child);
+      }
+    },
+);

packages/field/src/formRegistrarManager.js

@@ -0,0 +1,36 @@
+/**
+ * Allows to align the timing for all Registrars (like form, fieldset).
+ * e.g. it will only be ready once all Registrars have been fully rendered
+ *
+ * This is a requirement for ShadyDOM as otherwise forms can not catch registration events
+ */
+class FormRegistrarManager {
+  constructor() {
+    this.__elements = [];
+    this._fakeExtendsEventTarget();
+    this.ready = false;
+  }
+
+  add(registrar) {
+    this.__elements.push(registrar);
+    this.ready = false;
+  }
+
+  becomesReady() {
+    if (this.__elements.every(el => el.__readyForRegistration === true)) {
+      this.dispatchEvent(new Event('all-forms-open-for-registration'));
+      this.ready = true;
+    }
+  }
+
+  // TODO: this method has to be removed when EventTarget polyfill is available on IE11
+  // issue: https://gitlab.ing.net/TheGuideComponents/lion-element/issues/12
+  _fakeExtendsEventTarget() {
+    const delegate = document.createDocumentFragment();
+    ['addEventListener', 'dispatchEvent', 'removeEventListener'].forEach(funcName => {
+      this[funcName] = (...args) => delegate[funcName](...args);
+    });
+  }
+}
+
+export const formRegistrarManager = new FormRegistrarManager();

packages/field/test/FormControlMixin.test.js

@@ -1,5 +1,4 @@
-import { expect, fixture, html, defineCE, unsafeStatic, nextFrame } from '@open-wc/testing';
-import sinon from 'sinon';
+import { expect, fixture, html, defineCE, unsafeStatic } from '@open-wc/testing';
 import { SlotMixin } from '@lion/core';
 import { LionLitElement } from '@lion/core/src/LionLitElement.js';
 
@@ -26,42 +25,6 @@ describe('FormControlMixin', () => {
     tag = unsafeStatic(elem);
   });
 
-  it('dispatches event to register in Light DOM', async () => {
-    const registerSpy = sinon.spy();
-    await fixture(html`
-      <div @form-element-register=${registerSpy}>
-        <${tag}></${tag}>
-      </div>
-    `);
-    await nextFrame();
-    expect(registerSpy.callCount).to.equal(1);
-  });
-
-  it('can by caught by listening in the appropriate dom', async () => {
-    const registerSpy = sinon.spy();
-    const testTag = unsafeStatic(
-      defineCE(
-        class extends LionLitElement {
-          connectedCallback() {
-            super.connectedCallback();
-            this.shadowRoot.addEventListener('form-element-register', registerSpy);
-          }
-
-          render() {
-            return html`
-              <${tag}></${tag}>
-            `;
-          }
-        },
-      ),
-    );
-    await fixture(html`
-      <${testTag}></${testTag}>
-    `);
-    await nextFrame();
-    expect(registerSpy.callCount).to.equal(1);
-  });
-
   it('has the capability to override the help text', async () => {
     const lionFieldAttr = await fixture(html`
       <${tag} help-text="This email address is already taken">${inputSlot}</${tag}>

packages/field/test/FormRegistrationMixins.test.js

@@ -0,0 +1,106 @@
+import { expect, fixture, html, defineCE, unsafeStatic } from '@open-wc/testing';
+import sinon from 'sinon';
+import { LitElement, UpdatingElement } from '@lion/core';
+
+import { FormRegisteringMixin } from '../src/FormRegisteringMixin.js';
+import { FormRegistrarMixin } from '../src/FormRegistrarMixin.js';
+
+describe('FormRegistrationMixins', () => {
+  before(async () => {
+    const FormRegistrarEl = class extends FormRegistrarMixin(UpdatingElement) {};
+    customElements.define('form-registrar', FormRegistrarEl);
+    const FormRegisteringEl = class extends FormRegisteringMixin(UpdatingElement) {};
+    customElements.define('form-registering', FormRegisteringEl);
+  });
+
+  it('can register a formElement', async () => {
+    const el = await fixture(html`
+      <form-registrar>
+        <form-registering></form-registering>
+      </form-registrar>
+    `);
+    await el.registrationReady;
+    expect(el.formElements.length).to.equal(1);
+  });
+
+  it('supports nested registrar', async () => {
+    const el = await fixture(html`
+      <form-registrar>
+        <form-registrar>
+          <form-registering></form-registering>
+        </form-registrar>
+      </form-registrar>
+    `);
+    await el.registrationReady;
+    expect(el.formElements.length).to.equal(1);
+    expect(el.querySelector('form-registrar').formElements.length).to.equal(1);
+  });
+
+  it('works for component that have a delayed render', async () => {
+    const tagWrapperString = defineCE(
+      class extends FormRegistrarMixin(LitElement) {
+        async performUpdate() {
+          await new Promise(resolve => setTimeout(() => resolve(), 10));
+          await super.performUpdate();
+        }
+
+        render() {
+          return html`
+            <slot></slot>
+          `;
+        }
+      },
+    );
+    const tagWrapper = unsafeStatic(tagWrapperString);
+    const registerSpy = sinon.spy();
+    const el = await fixture(html`
+      <${tagWrapper} @form-element-register=${registerSpy}>
+        <form-registering></form-registering>
+      </${tagWrapper}>
+    `);
+    await el.registrationReady;
+    expect(el.formElements.length).to.equal(1);
+  });
+
+  it('requests update of the resetModelValue function of its parent formGroup', async () => {
+    const ParentFormGroupClass = class extends FormRegistrarMixin(LitElement) {
+      _updateResetModelValue() {
+        this.resetModelValue = 'foo';
+      }
+    };
+    const ChildFormGroupClass = class extends FormRegisteringMixin(LitElement) {
+      constructor() {
+        super();
+        this.__parentFormGroup = this.parentNode;
+      }
+    };
+
+    const parentClass = defineCE(ParentFormGroupClass);
+    const formGroup = unsafeStatic(parentClass);
+    const childClass = defineCE(ChildFormGroupClass);
+    const childFormGroup = unsafeStatic(childClass);
+    const parentFormEl = await fixture(html`
+      <${formGroup}><${childFormGroup} id="child" name="child[]"></${childFormGroup}></${formGroup}>
+    `);
+    expect(parentFormEl.resetModelValue).to.equal('foo');
+  });
+
+  it('can dynamically add/remove elements', async () => {
+    const el = await fixture(html`
+      <form-registrar>
+        <form-registering></form-registering>
+      </form-registrar>
+    `);
+    const newField = await fixture(html`
+      <form-registering></form-registering>
+    `);
+
+    expect(el.formElements.length).to.equal(1);
+
+    el.appendChild(newField);
+    expect(el.formElements.length).to.equal(2);
+
+    el.removeChild(newField);
+    expect(el.formElements.length).to.equal(1);
+  });
+});

packages/fieldset/src/LionFieldset.js

@@ -3,7 +3,7 @@ import { LionLitElement } from '@lion/core/src/LionLitElement.js';
 import { CssClassMixin } from '@lion/core/src/CssClassMixin.js';
 import { ObserverMixin } from '@lion/core/src/ObserverMixin.js';
 import { ValidateMixin } from '@lion/validate';
-import { FormControlMixin } from '@lion/field';
+import { FormControlMixin, FormRegistrarMixin } from '@lion/field';
 
 // TODO: extract from module like import { pascalCase } from 'lion-element/CaseMapUtils.js'
 const pascalCase = str => str.charAt(0).toUpperCase() + str.slice(1);
@@ -14,8 +14,8 @@ const pascalCase = str => str.charAt(0).toUpperCase() + str.slice(1);
  * @customElement
  * @extends LionLitElement
  */
-export class LionFieldset extends FormControlMixin(
-  ValidateMixin(CssClassMixin(SlotMixin(ObserverMixin(LionLitElement)))),
+export class LionFieldset extends FormRegistrarMixin(
+  FormControlMixin(ValidateMixin(CssClassMixin(SlotMixin(ObserverMixin(LionLitElement))))),
 ) {
   static get properties() {
     return {
@@ -109,8 +109,6 @@ export class LionFieldset extends FormControlMixin(
     this.addEventListener('focused-changed', this._updateFocusedClass);
     this.addEventListener('touched-changed', this._updateTouchedClass);
     this.addEventListener('dirty-changed', this._updateDirtyClass);
-    this.addEventListener('form-element-register', this.__onFormElementRegister);
-    this.addEventListener('form-element-unregister', this.__onFormElementUnRegister);
     this._setRole();
   }
 
@@ -121,19 +119,6 @@ export class LionFieldset extends FormControlMixin(
     this.removeEventListener('focused-changed', this._updateFocusedClass);
     this.removeEventListener('touched-changed', this._updateTouchedClass);
     this.removeEventListener('dirty-changed', this._updateDirtyClass);
-    this.removeEventListener('form-element-register', this.__onFormElementRegister);
-    this.removeEventListener('form-element-unregister', this.__onFormElementUnRegister);
-    if (this.__parentFormGroup) {
-      const event = new CustomEvent('form-element-unregister', {
-        detail: { element: this },
-        bubbles: true,
-      });
-      this.__parentFormGroup.dispatchEvent(event);
-    }
-  }
-
-  isRegisteredFormElement(el) {
-    return Object.keys(this.formElements).some(name => el.name === name);
   }
 
   // eslint-disable-next-line class-methods-use-this
@@ -299,10 +284,13 @@ export class LionFieldset extends FormControlMixin(
     return serializedValues;
   }
 
-  __onFormElementRegister(event) {
-    const child = event.detail.element;
-    if (child === this) return; // as we fire and listen - don't add ourselves
-
+  /**
+   * Adds the element to an object with the child name as a key
+   * Note: this is different to the default behavior of just beeing an array
+   *
+   * @override
+   */
+  addFormElement(child) {
     const { name } = child;
     if (!name) {
       console.info('Error Node:', child); // eslint-disable-line no-console
@@ -312,9 +300,9 @@ export class LionFieldset extends FormControlMixin(
       console.info('Error Node:', child); // eslint-disable-line no-console
       throw new TypeError(`You can not have the same name "${name}" as your parent`);
     }
-    event.stopPropagation();
 
     if (this.disabled) {
+      // eslint-disable-next-line no-param-reassign
       child.disabled = true;
     }
     if (name.substr(-2) === '[]') {
@@ -332,6 +320,7 @@ export class LionFieldset extends FormControlMixin(
     }
 
     // This is a way to let the child element (a lion-fieldset or lion-field) know, about its parent
+    // eslint-disable-next-line no-param-reassign
     child.__parentFormGroup = this;
 
     // aria-describedby of (nested) children
@@ -381,18 +370,15 @@ export class LionFieldset extends FormControlMixin(
     // might go wrong then when dom order changes per instance. Although we could check if
     // 'provision' has taken place or not
     const orderedEls = this._getAriaElementsInRightDomOrder(descriptionElements);
-    orderedEls.forEach(el => field.addToAriaDescription(el.id));
+    orderedEls.forEach(el => {
+      if (field.addToAriaDescription) {
+        field.addToAriaDescription(el.id);
+      }
+    });
   }
 
-  __onFormElementUnRegister(event) {
-    const child = event.detail.element;
+  removeFormElement(child) {
     const { name } = child;
-    if (child === this) {
-      return;
-    } // as we fire and listen - don't add ourself
-
-    event.stopPropagation();
-
     if (name.substr(-2) === '[]' && this.formElements[name]) {
       const index = this.formElements[name].indexOf(child);
       if (index > -1) {

packages/fieldset/test/lion-fieldset.test.js

@@ -77,11 +77,9 @@ describe('<lion-fieldset>', () => {
     let error = false;
     const el = await fixture(`<lion-fieldset></lion-fieldset>`);
     try {
-      // we need to use the private api here as errors thrown from a web component are in a
+      // we test the api directly as errors thrown from a web component are in a
       // different context and we can not catch them here => register fake elements
-      el.__onFormElementRegister({
-        detail: { element: {} },
-      });
+      el.addFormElement({});
     } catch (err) {
       error = err;
     }
@@ -98,11 +96,9 @@ describe('<lion-fieldset>', () => {
     let error = false;
     const el = await fixture(`<lion-fieldset name="foo"></lion-fieldset>`);
     try {
-      // we need to use the private api here as errors thrown from a web component are in a
+      // we test the api directly as errors thrown from a web component are in a
       // different context and we can not catch them here => register fake elements
-      el.__onFormElementRegister({
-        detail: { element: { name: 'foo' } },
-      });
+      el.addFormElement({ name: 'foo' });
     } catch (err) {
       error = err;
     }
@@ -119,16 +115,10 @@ describe('<lion-fieldset>', () => {
     let error = false;
     const el = await fixture(`<lion-fieldset></lion-fieldset>`);
     try {
-      // we need to use the private api here as errors thrown from a web component are in a
+      // we test the api directly as errors thrown from a web component are in a
       // different context and we can not catch them here => register fake elements
-      el.__onFormElementRegister({
-        stopPropagation: () => {},
-        detail: { element: { name: 'fooBar', addToAriaDescription: () => {} } },
-      });
-      el.__onFormElementRegister({
-        stopPropagation: () => {},
-        detail: { element: { name: 'fooBar' } },
-      });
+      el.addFormElement({ name: 'fooBar' });
+      el.addFormElement({ name: 'fooBar' });
     } catch (err) {
       error = err;
     }
@@ -144,12 +134,10 @@ describe('<lion-fieldset>', () => {
   it('can dynamically add/remove elements', async () => {
     const fieldset = await fixture(`<${tagString}>${inputSlotString}</${tagString}>`);
     const newField = await fixture(`<lion-input name="lastName"></lion-input>`);
-    await nextFrame();
 
     expect(Object.keys(fieldset.formElements).length).to.equal(3);
 
     fieldset.appendChild(newField);
-    await nextFrame();
     expect(Object.keys(fieldset.formElements).length).to.equal(4);
 
     fieldset.inputElement.removeChild(newField);
@@ -163,8 +151,9 @@ describe('<lion-fieldset>', () => {
         <${tagString} name="newfieldset">${inputSlotString}</${tagString}>
       </lion-fieldset>
     `);
-    await nextFrame();
+    await fieldset.registrationReady;
     const newFieldset = fieldset.querySelector('lion-fieldset');
+    await newFieldset.registrationReady;
     fieldset.formElements.lastName.modelValue = 'Bar';
     newFieldset.formElements['hobbies[]'][0].modelValue = { checked: true, value: 'chess' };
     newFieldset.formElements['hobbies[]'][1].modelValue = { checked: false, value: 'football' };
@@ -649,31 +638,36 @@ describe('<lion-fieldset>', () => {
     });
 
     it('has correct validation afterwards', async () => {
-      const isCat = modelValue => ({ isCat: modelValue.value === 'cat' });
-      const containsA = modelValues => ({
-        containsA: modelValues.color.value ? modelValues.color.value.indexOf('a') > -1 : false,
-      });
+      const isCat = modelValue => ({ isCat: modelValue === 'cat' });
+      const containsA = modelValues => {
+        return {
+          containsA: modelValues.color ? modelValues.color.indexOf('a') > -1 : false,
+        };
+      };
 
-      const fieldset = await fixture(`<${tagString}>${inputSlotString}</${tagString}>`);
-      await nextFrame();
-      fieldset.formElements.color.modelValue = { value: 'onlyb' };
-      fieldset.errorValidators = [[containsA]];
-      fieldset.formElements.color.errorValidators = [[isCat]];
+      const el = await fixture(html`
+        <${tag} .errorValidators=${[[containsA]]}>
+          <lion-input name="color" .errorValidators=${[[isCat]]}></lion-input>
+          <lion-input name="color2"></lion-input>
+        </${tag}>
+      `);
+      await el.registrationReady;
+      expect(el.errorState).to.be.true;
+      expect(el.error.containsA).to.be.true;
+      expect(el.formElements.color.errorState).to.be.false;
 
-      expect(fieldset.errorState).to.equal(true);
-      expect(fieldset.error.containsA).to.equal(true);
-      expect(fieldset.formElements.color.error.isCat).to.equal(true);
+      el.formElements.color.modelValue = 'onlyb';
+      expect(el.errorState).to.be.true;
+      expect(el.error.containsA).to.be.true;
+      expect(el.formElements.color.error.isCat).to.be.true;
 
-      fieldset.formElements.color.modelValue = { value: 'cat' };
-      expect(fieldset.errorState).to.equal(false);
+      el.formElements.color.modelValue = 'cat';
+      expect(el.errorState).to.be.false;
 
-      fieldset.resetGroup();
-      fieldset.formElements.color.modelValue = { value: 'Foo' };
-      fieldset.errorValidators = [[containsA]];
-      fieldset.formElements.color.errorValidators = [[isCat]];
-      expect(fieldset.errorState).to.equal(true);
-      expect(fieldset.error.containsA).to.equal(true);
-      expect(fieldset.formElements.color.error.isCat).to.equal(true);
+      el.resetGroup();
+      expect(el.errorState).to.be.true;
+      expect(el.error.containsA).to.be.true;
+      expect(el.formElements.color.errorState).to.be.false;
     });
   });