From 129134df84417b561ffafa25b2043d78be9c7340 Mon Sep 17 00:00:00 2001 From: Thomas Heller Date: Thu, 9 Jan 2020 10:40:21 +0100 Subject: [PATCH] fix(filter-field): Fixes an issue with instant submission of free-text. When selecting a freetext node from the autocomplete with the keyboard the the different eventHandlers of the autocomplete and the filterfield raced to a broken state. To prevent the instant submission of an empty filter we are handling the locking of the input field and synching that lock with the zone cycle. Additionally this commit implements a fix for submission of empty values if the value and submission happend in a shorter time than the DT_FILTER_FIELD_TYPING_DEBOUNCE. Fixes #294 --- .../filter-field/filter-field.e2e.ts | 144 ++++++++++++++++++ .../filter-field/filter-field.po.ts | 19 ++- .../components/filter-field/filter-field.ts | 13 ++ components/filter-field/src/filter-field.html | 1 - .../filter-field/src/filter-field.spec.ts | 54 ++++++- components/filter-field/src/filter-field.ts | 70 ++++++--- 6 files changed, 270 insertions(+), 31 deletions(-) diff --git a/apps/components-e2e/src/components/filter-field/filter-field.e2e.ts b/apps/components-e2e/src/components/filter-field/filter-field.e2e.ts index 860b19f809..df7c9acb7b 100644 --- a/apps/components-e2e/src/components/filter-field/filter-field.e2e.ts +++ b/apps/components-e2e/src/components/filter-field/filter-field.e2e.ts @@ -23,6 +23,8 @@ import { input, clearAll, filterTags, + focusFilterFieldInput, + getFilterfieldTags, } from './filter-field.po'; import { Selector } from 'testcafe'; @@ -37,6 +39,10 @@ test('should not show a error box if there is no validator provided', async (tes test('should show a error box if does not meet the validation function', async (testController: TestController) => { await clickOption(3); await testController.typeText(input, 'a'); + + // Wiat for the filter field to refresh the error message. + await testController.wait(250); + await testController.expect(await errorBox.exists).ok(); await testController .expect(await errorBox.innerText) @@ -76,3 +82,141 @@ test('should remove all filters when clicking the clear-all button', async (test await testController.wait(300); await testController.expect(await filterTags.exists).notOk(); }); + +test('should choose a freetext node with the keyboard and submit the correct value', async (testController: TestController) => { + // Wait for the input before it can be focussed. + await input.exists; + await focusFilterFieldInput(); + + // Select the test autocomplete + await testController.pressKey('down down down down enter'); + + // Wait for a certain amount of time to let the filterfield refresh + await testController.wait(250); + + // Select the free text node and start typing + await testController.pressKey('down down down enter'); + + await testController.typeText(input, 'Custom selection'); + + // Wait for a certain amout fo time to let the filterfield refresh + await testController.wait(250); + + // Confirm the text typed in + await testController.pressKey('enter'); + + const tags = await getFilterfieldTags(); + + await testController.expect(tags.length).eql(1); + await testController + .expect(tags[0]) + .eql('Autocomplete with free text optionsCustom selection'); +}); + +test('should choose a freetext node with the keyboard and submit an empty value', async (testController: TestController) => { + // Wait for the input before it can be focussed. + await input.exists; + await focusFilterFieldInput(); + + // Select the test autocomplete + await testController.pressKey('down down down down enter'); + + // Wait for a certain amount of time to let the filterfield refresh + await testController.wait(250); + + // Select the free text node and start typing + await testController.pressKey('down down down enter'); + + // Wait for a certain amout fo time to let the filterfield refresh + await testController.wait(250); + + // Confirm the text typed in + await testController.pressKey('enter'); + + const tags = await getFilterfieldTags(); + + await testController.expect(tags.length).eql(1); + await testController + .expect(tags[0]) + .eql('Autocomplete with free text options'); +}); + +test('should choose a freetext node with the keyboard and submit an empyty value immediately', async (testController: TestController) => { + await focusFilterFieldInput(); + + // Select the test autocomplete + await testController.pressKey('down down down down enter'); + + // Wait for a certain amount of time to let the filterfield refresh + await testController.wait(250); + + // Select the free text node and start typing + await testController.pressKey('down down down enter'); + + // Focus the filter field + await testController.pressKey('enter'); + + const tags = await getFilterfieldTags(); + + await testController.expect(tags.length).eql(1); + await testController + .expect(tags[0]) + .eql('Autocomplete with free text options'); +}); + +test('should choose a freetext node with the mouse and submit the correct value immediately', async (testController: TestController) => { + // Select the test autocomplete + await clickOption(5); + + // Wait for a certain amount of time to let the filterfield refresh + await testController.wait(250); + + // Select the free text node and start typing + await clickOption(4); + + // Wait for a certain amout fo time to let the filterfield refresh + await testController.wait(250); + + // Send the correct value into the input field + await testController.typeText(input, 'Custom selection'); + + // Focus the filter field + await focusFilterFieldInput(); + + // Submit the value immediately + await testController.pressKey('enter'); + + const tags = await getFilterfieldTags(); + + await testController.expect(tags.length).eql(1); + await testController + .expect(tags[0]) + .eql('Autocomplete with free text optionsCustom selection'); +}); + +test('should choose a freetext node with the mouse and submit an empty value immediately', async (testController: TestController) => { + // Select the test autocomplete + await clickOption(5); + + // Wait for a certain amount of time to let the filterfield refresh + await testController.wait(250); + + // Select the free text node and start typing + await clickOption(4); + + // Wait for a certain amout fo time to let the filterfield refresh + await testController.wait(250); + + // Confirm the text typed in + await focusFilterFieldInput(); + + // Submit the empty value immediately + await testController.pressKey('enter'); + + const tags = await getFilterfieldTags(); + + await testController.expect(tags.length).eql(1); + await testController + .expect(tags[0]) + .eql('Autocomplete with free text options'); +}); diff --git a/apps/components-e2e/src/components/filter-field/filter-field.po.ts b/apps/components-e2e/src/components/filter-field/filter-field.po.ts index 31657239b0..6f1c15f33c 100644 --- a/apps/components-e2e/src/components/filter-field/filter-field.po.ts +++ b/apps/components-e2e/src/components/filter-field/filter-field.po.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Selector, t } from 'testcafe'; +import { Selector, t, ClientFunction } from 'testcafe'; export const errorBox = Selector('.dt-filter-field-error'); export const filterField = Selector('#filter-field'); @@ -33,3 +33,20 @@ export async function clickOption( await controller.click(filterField); await controller.click(option(nth)); } + +/** Focus the input of the filter field to send key events to it. */ +export const focusFilterFieldInput = ClientFunction(() => { + (document.querySelector('#filter-field input') as HTMLElement).focus(); +}); + +/** Retreive all set tags in the filter field and their values. */ +export const getFilterfieldTags = ClientFunction(() => { + const filterFieldTags: HTMLElement[] = [].slice.call( + document.querySelectorAll('.dt-filter-field-tag'), + ); + const contents: string[] = []; + for (const tag of filterFieldTags) { + contents.push(tag.textContent || ''); + } + return contents; +}); diff --git a/apps/components-e2e/src/components/filter-field/filter-field.ts b/apps/components-e2e/src/components/filter-field/filter-field.ts index 320a4ab7b4..adbd4530c3 100644 --- a/apps/components-e2e/src/components/filter-field/filter-field.ts +++ b/apps/components-e2e/src/components/filter-field/filter-field.ts @@ -51,6 +51,19 @@ const TEST_DATA = { }, ], }, + { + name: 'Autocomplete with free text options', + autocomplete: [ + 'Autocomplete option 1', + 'Autocomplete option 2', + 'Autocomplete option 3', + { + name: 'Autocomplete free text', + suggestions: ['Suggestion 1', 'Suggestion 2', 'Suggestion 3'], + validators: [], + }, + ], + }, ], }; diff --git a/components/filter-field/src/filter-field.html b/components/filter-field/src/filter-field.html index a61db15645..031354e394 100644 --- a/components/filter-field/src/filter-field.html +++ b/components/filter-field/src/filter-field.html @@ -35,7 +35,6 @@ [dtFilterFieldRange]="range" [dtFilterFieldRangeDisabled]="!(_currentDef && !!_currentDef!.range) || loading" (keydown)="_handleInputKeyDown($event)" - (keyup)="_handleInputKeyUp($event)" [value]="_inputValue" /> { tick(DT_FILTER_FIELD_TYPING_DEBOUNCE); fixture.detectChanges(); - dispatchKeyboardEvent(inputEl, 'keyup', ENTER); + dispatchKeyboardEvent(inputEl, 'keydown', ENTER); + + tick(DT_FILTER_FIELD_TYPING_DEBOUNCE); + fixture.detectChanges(); + + const tags = getFilterTags(fixture); + expect(tags.length).toBe(1); + expect(tags[0].key).toBe('Free'); + expect(tags[0].separator).toBe('~'); + expect(tags[0].value).toBe('abc'); + expect(spy).toHaveBeenCalledTimes(1); + subscription.unsubscribe(); + })); + + it('should switch to free text with keyboard interaction and on enter fire a filterChanges event and create a tag', fakeAsync(() => { + const spy = jest.fn(); + const subscription = filterField.filterChanges.subscribe(spy); + filterField.focus(); + zone.simulateMicrotasksEmpty(); + zone.simulateZoneExit(); + fixture.detectChanges(); + + const inputEl = getInput(fixture); + + // Select the free text option with the keyboard and hit enter + dispatchKeyboardEvent(inputEl, 'keydown', DOWN_ARROW); + dispatchKeyboardEvent(inputEl, 'keydown', DOWN_ARROW); + + // Use keydown and keyup arrow, as this is what happens in the real world as well. + dispatchKeyboardEvent(inputEl, 'keydown', ENTER); + + zone.simulateMicrotasksEmpty(); + fixture.detectChanges(); + + typeInElement('abc', inputEl); + tick(DT_FILTER_FIELD_TYPING_DEBOUNCE); + + fixture.detectChanges(); + dispatchKeyboardEvent(inputEl, 'keydown', ENTER); + + tick(DT_FILTER_FIELD_TYPING_DEBOUNCE); fixture.detectChanges(); const tags = getFilterTags(fixture); + expect(tags.length).toBe(1); expect(tags[0].key).toBe('Free'); expect(tags[0].separator).toBe('~'); @@ -2065,8 +2106,13 @@ function getFilterTags( key && key.getAttribute('data-separator') ? key.getAttribute('data-separator')! : ''; - const value = ele.nativeElement.querySelector('.dt-filter-field-tag-value') - .textContent; + + // Get the value of the filter element, if no filter value element is rendered + // assume an empty string. + const valueElement = ele.nativeElement.querySelector( + '.dt-filter-field-tag-value', + ); + const value = valueElement ? valueElement.textContent : ''; return { ele, diff --git a/components/filter-field/src/filter-field.ts b/components/filter-field/src/filter-field.ts index fb15fe792d..ca3207437f 100644 --- a/components/filter-field/src/filter-field.ts +++ b/components/filter-field/src/filter-field.ts @@ -68,6 +68,9 @@ import { switchMap, take, takeUntil, + map, + distinctUntilChanged, + tap, } from 'rxjs/operators'; import { @@ -196,6 +199,13 @@ export class DtFilterField implements AfterViewInit, OnDestroy, OnChanges { private _stateChanges = new Subject(); private _outsideClickSubscription: Subscription | null; + /** + * Locks the inputfield from receiving additional keyEvents + * to prevent race conditions between the autocomplete and + * filterfield handlers. + */ + private _inputFieldKeyboardLocked = false; + /** * Whether a loading spinner should be shown or not. * This will be automatically set if you provide set async to true via the data source. @@ -375,6 +385,11 @@ export class DtFilterField implements AfterViewInit, OnDestroy, OnChanges { this._stateChanges .pipe(switchMap(() => this._zone.onMicrotaskEmpty.pipe(take(1)))) .subscribe(() => { + // Unlocking the input field again. It is now ready to receive + // keyboard events again. The locking mechanism is necessary to prevent + // races between the filter field keyboardEvent handler and the autocomplete + // keyboardEvent handler. + this._inputFieldKeyboardLocked = false; if (this._isFocused) { if ( isDtAutocompleteDef(this._currentDef) || @@ -421,6 +436,10 @@ export class DtFilterField implements AfterViewInit, OnDestroy, OnChanges { // tslint:disable-next-line:no-any this._autocomplete.optionSelected.subscribe( (event: DtAutocompleteSelectedEvent) => { + // Locking keyboardEvents until they are being unlocked again in the next change + // detection cycle. This is to prevent races between autocomplete and the + // filterfield keybaord event listeners. + this._inputFieldKeyboardLocked = true; this._handleAutocompleteSelected(event); }, ); @@ -429,6 +448,11 @@ export class DtFilterField implements AfterViewInit, OnDestroy, OnChanges { fromEvent(this._inputEl.nativeElement, 'input') .pipe( takeUntil(this._destroy), + map(() => this._inputEl.nativeElement.value), + distinctUntilChanged(), + tap(value => { + this._inputValue = value; + }), debounceTime(DT_FILTER_FIELD_TYPING_DEBOUNCE), ) .subscribe(() => { @@ -507,15 +531,12 @@ export class DtFilterField implements AfterViewInit, OnDestroy, OnChanges { /** @internal Keep track of the values in the input fields. Write the current value to the _inputValue property */ _handleInputChange(): void { const value = this._inputEl.nativeElement.value; - if (value !== this._inputValue) { - this._inputValue = value; - this._writeControlValue(value); - this._updateAutocompleteOptionsOrGroups(); - this.inputChange.emit(value); + this._writeControlValue(value); + this._updateAutocompleteOptionsOrGroups(); + this.inputChange.emit(value); - this._validateInput(); - this._changeDetectorRef.markForCheck(); - } + this._validateInput(); + this._changeDetectorRef.markForCheck(); } /** @internal */ @@ -535,26 +556,25 @@ export class DtFilterField implements AfterViewInit, OnDestroy, OnChanges { if (this._editModeStashedValue) { this._cancelEditMode(); } + } else { + if (this._inputFieldKeyboardLocked) { + return; + } + const value = this._inputEl.nativeElement.value; + this._writeControlValue(value); + this._validateInput(); + if ( + keyCode === ENTER && + isDtFreeTextDef(this._currentDef) && + this._control && + this._control.valid + ) { + this._handleFreeTextSubmitted(); + } + this._changeDetectorRef.markForCheck(); } } - /** @internal */ - _handleInputKeyUp(event: KeyboardEvent): void { - const keyCode = readKeyCode(event); - const value = this._inputEl.nativeElement.value; - this._writeControlValue(value); - this._validateInput(); - if ( - keyCode === ENTER && - isDtFreeTextDef(this._currentDef) && - this._control && - this._control.valid - ) { - this._handleFreeTextSubmitted(); - } - this._changeDetectorRef.markForCheck(); - } - /** * @internal * Handles removing a filter from the filters list.