From 20af7a44ceb5d1985b778e5f5cbf68073068f28d Mon Sep 17 00:00:00 2001 From: Thomas Allmer Date: Tue, 7 Jun 2022 11:51:44 +0200 Subject: [PATCH] fix(form-core): maintain correct formElements order with non-form elements in dom --- .changeset/odd-tools-sing.md | 17 ++++ .../src/registration/FormRegistrarMixin.js | 17 +++- .../FormRegistrationMixins.suite.js | 98 ++++++++++++++++--- 3 files changed, 118 insertions(+), 14 deletions(-) create mode 100644 .changeset/odd-tools-sing.md diff --git a/.changeset/odd-tools-sing.md b/.changeset/odd-tools-sing.md new file mode 100644 index 000000000..636eeccaf --- /dev/null +++ b/.changeset/odd-tools-sing.md @@ -0,0 +1,17 @@ +--- +'@lion/form-core': patch +--- + +Make sure form elements are added in the correct order even when non-form elements are in between. + +```html + + + + +
+ + +
+ +``` diff --git a/packages/form-core/src/registration/FormRegistrarMixin.js b/packages/form-core/src/registration/FormRegistrarMixin.js index e97de5c07..7ecd5fc71 100644 --- a/packages/form-core/src/registration/FormRegistrarMixin.js +++ b/packages/form-core/src/registration/FormRegistrarMixin.js @@ -222,11 +222,22 @@ const FormRegistrarMixinImplementation = superclass => } ev.stopPropagation(); - // Check for siblings to determine the right order to insert into formElements - // If there is no next sibling, index is -1 + // Check for DOM order to determine the right order to insert into formElements + // If there is no other element, index is -1 (e.g. add it to the end) let indexToInsertAt = -1; if (this.formElements && Array.isArray(this.formElements)) { - indexToInsertAt = this.formElements.indexOf(child.nextElementSibling); + // we start comparing from the end of the array as it's the most likely position where the element will be added + for (const [i, formElement] of this.formElements.entries()) { + // compareDocumentPosition returns a bitmask + // eslint-disable-next-line no-bitwise + if (formElement.compareDocumentPosition(child) & Node.DOCUMENT_POSITION_FOLLOWING) { + // nothing as child is after formElement in DOM + } else { + // first time child is NOT after formElement in DOM we insert it + indexToInsertAt = i; + break; + } + } } this.addFormElement(child, indexToInsertAt); } diff --git a/packages/form-core/test-suites/FormRegistrationMixins.suite.js b/packages/form-core/test-suites/FormRegistrationMixins.suite.js index c359bc462..67363b43d 100644 --- a/packages/form-core/test-suites/FormRegistrationMixins.suite.js +++ b/packages/form-core/test-suites/FormRegistrationMixins.suite.js @@ -1,4 +1,4 @@ -import { LitElement } from '@lion/core'; +import { LitElement, uuid } from '@lion/core'; import { defineCE, expect, fixture, html, unsafeStatic } from '@open-wc/testing'; import { FormRegisteringMixin } from '../src/registration/FormRegisteringMixin.js'; import { FormRegistrarMixin } from '../src/registration/FormRegistrarMixin.js'; @@ -71,6 +71,74 @@ export const runRegistrationSuite = customConfig => { expect(el.formElements.length).to.equal(1); }); + it('maintains dom order if there are arbitrary none-form elements between registering elements', async () => { + const el = /** @type {RegistrarClass} */ ( + await fixture(html` + <${parentTag}> + <${childTag} pos="1"> + + <${childTag} pos="2"> + + `) + ); + + const newField = await fixture(html` + <${childTag} pos="insert-between-1-and-2"> + `); + el.insertBefore(newField, el.children[1]); + + expect(el.formElements.length).to.equal(3); + expect(el.formElements.map(fel => fel.getAttribute('pos'))).to.deep.equal([ + '1', + 'insert-between-1-and-2', + '2', + ]); + }); + + it('maintains dom order if there are arbitrary none-form group elements between registering elements', async () => { + /** + * @param {string} tagString + */ + function lazyDefine(tagString) { + class Extension extends RegisteringClass {} + customElements.define(tagString, Extension); + } + + const [tagName1, tagName2, tagName3] = [uuid('lazy-1'), uuid('lazy-2'), uuid('lazy-3')]; + const [tag1, tag2, tag3] = [tagName1, tagName2, tagName3].map(name => unsafeStatic(name)); + + const el = /** @type {RegistrarClass} */ ( + await fixture(html` + <${parentTag} .name=${'test-group'}> +
+ <${tag1} .name=${'one'}> +
+
+ <${tag2} .name=${'two'}> + <${tag3} .name=${'three'}> +
+ + `) + ); + expect(el.formElements.length).to.equal(0); + + lazyDefine(tagName3); + await el.updateComplete; + expect(el.formElements.map(fel => fel.localName)).to.deep.equal([tagName3]); + + lazyDefine(tagName1); + await el.updateComplete; + expect(el.formElements.map(fel => fel.localName)).to.deep.equal([tagName1, tagName3]); + + lazyDefine(tagName2); + await el.updateComplete; + expect(el.formElements.map(fel => fel.localName)).to.deep.equal([ + tagName1, + tagName2, + tagName3, + ]); + }); + it('supports nested registration parents', async () => { const el = /** @type {RegistrarClass} */ ( await fixture(html` @@ -147,30 +215,38 @@ export const runRegistrationSuite = customConfig => { */ const newField = /** @type {RegisteringClass & prop} */ ( await fixture(html` - <${childTag}> - `) + <${childTag} pos="inserted-before-1"> + `) ); - newField.setAttribute('pos', 'inserted-before-1'); + // newField.setAttribute('pos', 'inserted-before-1'); el.insertBefore(newField, el.children[1]); expect(el.formElements.length).to.equal(4); - const secondChild = /** @type {RegisteringClass & prop} */ (el.children[1]); - expect(secondChild.getAttribute('pos')).to.equal('inserted-before-1'); - expect(el.formElements[1].getAttribute('pos')).to.equal('inserted-before-1'); + expect(el.formElements.map(fel => fel.getAttribute('pos'))).to.deep.equal([ + '0', + 'inserted-before-1', + '1', + '2', + ]); /** INSERT field before the pos=0 (e.g. at the top) */ const topField = /** @type {RegisteringClass & prop} */ ( await fixture(html` - <${childTag}> - `) + <${childTag} pos="inserted-before-0"> + `) ); - topField.setAttribute('pos', 'inserted-before-0'); el.insertBefore(topField, el.children[0]); // expect(el.formElements.length).to.equal(5); const firstChild = /** @type {RegisteringClass & prop} */ (el.children[0]); expect(firstChild.getAttribute('pos')).to.equal('inserted-before-0'); - expect(el.formElements[0].getAttribute('pos')).to.equal('inserted-before-0'); + expect(el.formElements.map(fel => fel.getAttribute('pos'))).to.deep.equal([ + 'inserted-before-0', + '0', + 'inserted-before-1', + '1', + '2', + ]); }); describe('FormRegistrarPortalMixin', () => {