From 3b5ed3222f6029c4330eead05e331ed8075fddfd Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Wed, 31 Mar 2021 19:47:56 +0200 Subject: [PATCH] fix(form-core): do not preprocess during composition --- .changeset/curly-lamps-cheer.md | 7 + packages/form-core/src/FormatMixin.js | 34 +- .../test-suites/FormatMixin.suite.js | 675 ++++++++++-------- .../form-core/types/FormatMixinTypes.d.ts | 1 + 4 files changed, 394 insertions(+), 323 deletions(-) create mode 100644 .changeset/curly-lamps-cheer.md diff --git a/.changeset/curly-lamps-cheer.md b/.changeset/curly-lamps-cheer.md new file mode 100644 index 000000000..418ed740c --- /dev/null +++ b/.changeset/curly-lamps-cheer.md @@ -0,0 +1,7 @@ +--- +'@lion/form-core': patch +--- + +### Bug fixes + +fix(form-core): do not preprocess during composition diff --git a/packages/form-core/src/FormatMixin.js b/packages/form-core/src/FormatMixin.js index e5269bd6d..86e299ded 100644 --- a/packages/form-core/src/FormatMixin.js +++ b/packages/form-core/src/FormatMixin.js @@ -233,13 +233,6 @@ const FormatMixinImplementation = superclass => this.__preventRecursiveTrigger = false; } - /** - * @param {string} value - */ - __callPreprocessor(value) { - return this.preprocessor(value); - } - /** * @param {string|undefined} value * @return {?} @@ -348,7 +341,9 @@ const FormatMixinImplementation = superclass => * to the parsing/formatting/serializing loop. */ _syncValueUpwards() { - this.value = this.__callPreprocessor(this.value); + if (!this.__isHandlingComposition) { + this.value = this.preprocessor(this.value); + } this.modelValue = this.__callParser(this.value); } @@ -366,6 +361,10 @@ const FormatMixinImplementation = superclass => } /** + * Every time .formattedValue is attempted to sync to the view value (on change/blur and on + * modelValue change), this condition is checked. When enhancing it, it's recommended to + * call `super._reflectBackOn()` + * @overridable * @return {boolean} */ _reflectBackOn() { @@ -393,10 +392,25 @@ const FormatMixinImplementation = superclass => this.__isHandlingUserInput = false; } + /** + * @param {Event} event + */ + __onCompositionEvent({ type }) { + if (type === 'compositionstart') { + this.__isHandlingComposition = true; + } else if (type === 'compositionend') { + this.__isHandlingComposition = false; + // in all other cases this would be triggered via user-input-changed + this._syncValueUpwards(); + } + } + constructor() { super(); this.formatOn = 'change'; this.formatOptions = /** @type {FormatOptions} */ ({}); + + this.__onCompositionEvent = this.__onCompositionEvent.bind(this); } connectedCallback() { @@ -422,6 +436,8 @@ const FormatMixinImplementation = superclass => if (this._inputNode) { this._inputNode.addEventListener(this.formatOn, this._reflectBackFormattedValueDebounced); this._inputNode.addEventListener('input', this._proxyInputEvent); + this._inputNode.addEventListener('compositionstart', this.__onCompositionEvent); + this._inputNode.addEventListener('compositionend', this.__onCompositionEvent); } } @@ -435,6 +451,8 @@ const FormatMixinImplementation = superclass => /** @type {EventListenerOrEventListenerObject} */ (this ._reflectBackFormattedValueDebounced), ); + this._inputNode.removeEventListener('compositionstart', this.__onCompositionEvent); + this._inputNode.removeEventListener('compositionend', this.__onCompositionEvent); } } }; diff --git a/packages/form-core/test-suites/FormatMixin.suite.js b/packages/form-core/test-suites/FormatMixin.suite.js index 4e1a238d7..906a3a409 100644 --- a/packages/form-core/test-suites/FormatMixin.suite.js +++ b/packages/form-core/test-suites/FormatMixin.suite.js @@ -35,10 +35,17 @@ class FormatClass extends FormatMixin(LitElement) { /** * @param {FormatClass} formControl * @param {?} newViewValue + * @param {{caretIndex?:number}} config */ -function mimicUserInput(formControl, newViewValue) { +function mimicUserInput(formControl, newViewValue, { caretIndex } = {}) { formControl.value = newViewValue; // eslint-disable-line no-param-reassign - formControl._inputNode.dispatchEvent(new CustomEvent('input', { bubbles: true })); + if (caretIndex) { + // eslint-disable-next-line no-param-reassign + formControl._inputNode.selectionStart = caretIndex; + // eslint-disable-next-line no-param-reassign + formControl._inputNode.selectionEnd = caretIndex; + } + formControl._inputNode.dispatchEvent(new Event('input', { bubbles: true })); } /** @@ -87,7 +94,7 @@ export function runFormatMixinSuite(customConfig) { describe('FormatMixin', async () => { /** @type {{d: any}} */ - let elem; + let tag; /** @type {FormatClass} */ let nonFormat; /** @type {FormatClass} */ @@ -97,87 +104,31 @@ export function runFormatMixinSuite(customConfig) { if (!cfg.tagString) { cfg.tagString = defineCE(FormatClass); } - elem = unsafeStatic(cfg.tagString); + tag = unsafeStatic(cfg.tagString); nonFormat = await fixture(html` - <${elem} + <${tag} .formatter="${/** @param {?} v */ v => v}" .parser="${/** @param {string} v */ v => v}" .serializer="${/** @param {?} v */ v => v}" .deserializer="${/** @param {string} v */ v => v}" > - + `); fooFormat = await fixture(html` - <${elem} + <${tag} .formatter="${/** @param {string} value */ value => `foo: ${value}`}" .parser="${/** @param {string} value */ value => value.replace('foo: ', '')}" .serializer="${/** @param {string} value */ value => `[foo] ${value}`}" .deserializer="${/** @param {string} value */ value => value.replace('[foo] ', '')}" > - + `); }); - it('fires `model-value-changed` for every input triggered by user', async () => { - const formatEl = /** @type {FormatClass} */ (await fixture( - html`<${elem}>`, - )); - - let counter = 0; - let isTriggeredByUser = false; - formatEl.addEventListener('model-value-changed', ( - /** @param {CustomEvent} event */ event, - ) => { - counter += 1; - isTriggeredByUser = /** @type {CustomEvent} */ (event).detail.isTriggeredByUser; - }); - - mimicUserInput(formatEl, generateValueBasedOnType()); - expect(counter).to.equal(1); - expect(isTriggeredByUser).to.be.true; - - // Counter offset +1 for Date because parseDate created a new Date object - // when the user changes the value. - // This will result in a model-value-changed trigger even if the user value was the same - // TODO: a proper solution would be to add `hasChanged` to input-date, like isSameDate() - // from calendar utils - const counterOffset = cfg.modelValueType === Date ? 1 : 0; - - mimicUserInput(formatEl, generateValueBasedOnType()); - expect(counter).to.equal(1 + counterOffset); - - mimicUserInput(formatEl, generateValueBasedOnType({ toggleValue: true })); - expect(counter).to.equal(2 + counterOffset); - }); - - it('fires `model-value-changed` for every programmatic modelValue change', async () => { - const el = /** @type {FormatClass} */ (await fixture( - html`<${elem}>`, - )); - let counter = 0; - let isTriggeredByUser = false; - - el.addEventListener('model-value-changed', event => { - counter += 1; - isTriggeredByUser = /** @type {CustomEvent} */ (event).detail.isTriggeredByUser; - }); - - el.modelValue = 'one'; - expect(counter).to.equal(1); - expect(isTriggeredByUser).to.be.false; - - // no change means no event - el.modelValue = 'one'; - expect(counter).to.equal(1); - - el.modelValue = 'two'; - expect(counter).to.equal(2); - }); - it('has modelValue, formattedValue and serializedValue which are computed synchronously', async () => { expect(nonFormat.modelValue).to.equal('', 'modelValue initially'); expect(nonFormat.formattedValue).to.equal('', 'formattedValue initially'); @@ -189,110 +140,212 @@ export function runFormatMixinSuite(customConfig) { expect(nonFormat.serializedValue).to.equal(generatedValue, 'serializedValue synchronized'); }); - it('has an input node (like /