From 673af56bc1bf7d5f670e7b9a9144b0a71017ccd7 Mon Sep 17 00:00:00 2001 From: lskramarov Date: Fri, 3 Jul 2026 10:20:14 +0300 Subject: [PATCH 1/2] fix(filter-bar): restore keyboard focus ring on pipe trigger after selection (#DS-3829) --- .../components/filter-bar/pipe-add.spec.ts | 76 +++++++++++++++++++ packages/components/filter-bar/pipe-add.ts | 1 + .../components/filter-bar/pipes/base-pipe.ts | 37 +++++++++ .../filter-bar/pipes/pipe-date.spec.ts | 32 +++++++- .../components/filter-bar/pipes/pipe-date.ts | 22 ++++-- .../filter-bar/pipes/pipe-datetime.spec.ts | 32 +++++++- .../filter-bar/pipes/pipe-datetime.ts | 22 ++++-- .../pipes/pipe-multi-select.spec.ts | 16 ++++ .../filter-bar/pipes/pipe-multi-select.ts | 2 + .../pipes/pipe-multi-tree-select.spec.ts | 20 +++++ .../pipes/pipe-multi-tree-select.ts | 2 + .../filter-bar/pipes/pipe-select.spec.ts | 46 +++++++++++ .../filter-bar/pipes/pipe-select.ts | 2 + .../filter-bar/pipes/pipe-text.spec.ts | 35 ++++++--- .../components/filter-bar/pipes/pipe-text.ts | 2 + .../filter-bar/pipes/pipe-tree-select.spec.ts | 15 ++++ .../filter-bar/pipes/pipe-tree-select.ts | 2 +- 17 files changed, 334 insertions(+), 30 deletions(-) diff --git a/packages/components/filter-bar/pipe-add.spec.ts b/packages/components/filter-bar/pipe-add.spec.ts index 0fae547d8..131afba19 100644 --- a/packages/components/filter-bar/pipe-add.spec.ts +++ b/packages/components/filter-bar/pipe-add.spec.ts @@ -2,6 +2,7 @@ import { ChangeDetectorRef, Component, DebugElement, inject } from '@angular/cor import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; +import { createKeyboardEvent, dispatchEvent, ENTER } from '@koobiq/components/core'; import { KbqFilter, KbqFilterBar, @@ -413,6 +414,81 @@ describe('KbqPipeAdd', () => { })); }); + describe('keyboard (Enter)', () => { + beforeEach(() => { + fixture = TestBed.createComponent(TestComponent); + filterBarDebugElement = fixture.debugElement.query(By.directive(KbqFilterBar)); + fixture.detectChanges(); + }); + + const pressEnterOnFirstOption = () => { + const option = document.querySelectorAll('.kbq-option')[0] as HTMLElement; + + dispatchEvent(option, createKeyboardEvent('keydown', ENTER, undefined, 'Enter')); + }; + + it('should add a pipe when Enter is pressed on a template option', fakeAsync(() => { + const filterBar = getFilterBar(); + const pipeAdd = getPipeAdd(); + + pipeAdd.select().open(); + flush(); + fixture.detectChanges(); + + pressEnterOnFirstOption(); + flush(); + fixture.detectChanges(); + + expect(filterBar.filter!.pipes.length).toBe(1); + expect(filterBar.filter!.pipes[0].id).toBe(PIPE_TEMPLATE_ID_1); + })); + + it('should emit onAddPipe when Enter is pressed on a template option', fakeAsync(() => { + const pipeAdd = getPipeAdd(); + const spy = jest.fn(); + + pipeAdd.onAddPipe.subscribe(spy); + + pipeAdd.select().open(); + flush(); + fixture.detectChanges(); + + pressEnterOnFirstOption(); + flush(); + fixture.detectChanges(); + + expect(spy).toHaveBeenCalledWith(expect.objectContaining({ id: PIPE_TEMPLATE_ID_1 })); + })); + + it('should call filterBar.openPipe.next when Enter is pressed on an already-added option', fakeAsync(() => { + const filterBar = getFilterBar(); + const pipeAdd = getPipeAdd(); + const openPipeSpy = jest.spyOn(filterBar.openPipe, 'next'); + + // First Enter — add the pipe + pipeAdd.select().open(); + flush(); + fixture.detectChanges(); + + pressEnterOnFirstOption(); + flush(); + fixture.detectChanges(); + + // Second Enter — option is now selected, should trigger openPipe + pipeAdd.select().open(); + flush(); + fixture.detectChanges(); + + openPipeSpy.mockClear(); + + pressEnterOnFirstOption(); + flush(); + fixture.detectChanges(); + + expect(openPipeSpy).toHaveBeenCalledWith(PIPE_TEMPLATE_ID_1); + })); + }); + describe('compareWith', () => { beforeEach(() => { fixture = TestBed.createComponent(TestComponent); diff --git a/packages/components/filter-bar/pipe-add.ts b/packages/components/filter-bar/pipe-add.ts index 1769af7cd..9263076f2 100644 --- a/packages/components/filter-bar/pipe-add.ts +++ b/packages/components/filter-bar/pipe-add.ts @@ -40,6 +40,7 @@ import { getId } from './pipes/base-pipe'; [value]="template" [showCheckbox]="false" (click)="addPipeFromTemplate(option)" + (keydown.enter)="addPipeFromTemplate(option)" > {{ template.name }} diff --git a/packages/components/filter-bar/pipes/base-pipe.ts b/packages/components/filter-bar/pipes/base-pipe.ts index 263346827..53464c59e 100644 --- a/packages/components/filter-bar/pipes/base-pipe.ts +++ b/packages/components/filter-bar/pipes/base-pipe.ts @@ -1,3 +1,4 @@ +import { FocusMonitor, FocusOrigin } from '@angular/cdk/a11y'; import { afterNextRender, AfterViewInit, @@ -47,6 +48,13 @@ export abstract class KbqBasePipe implements AfterViewInit { protected readonly changeDetectorRef = inject(ChangeDetectorRef); /** @docs-private */ protected readonly destroyRef = inject(DestroyRef); + /** @docs-private */ + protected readonly focusMonitor = inject(FocusMonitor); + /** @docs-private */ + protected readonly elementRef = inject>(ElementRef); + + /** Last known focus origin within the pipe. Used to preserve the keyboard focus ring on restore. */ + private focusOrigin: FocusOrigin = null; /** values to select from the pipe template */ protected values; @@ -89,6 +97,18 @@ export abstract class KbqBasePipe implements AfterViewInit { this.filterBar?.internalTemplatesChanges.pipe(takeUntilDestroyed()).subscribe(this.updateTemplates); + // Track the focus origin so the trigger's keyboard focus ring can be restored after + // a value is chosen. `checkChildren` captures focus on the inner trigger button. + this.focusMonitor + .monitor(this.elementRef, true) + .pipe( + filter((origin) => !!origin), + takeUntilDestroyed() + ) + .subscribe((origin) => (this.focusOrigin = origin)); + + this.destroyRef.onDestroy(() => this.focusMonitor.stopMonitoring(this.elementRef)); + afterNextRender(() => { this.isMac = isMac(); }); @@ -146,6 +166,23 @@ export abstract class KbqBasePipe implements AfterViewInit { this.filterBar?.onChangePipe.next(this.data); } + /** + * Restores focus to the pipe's trigger button after a value is chosen or the panel closes. + * Focuses via {@link FocusMonitor} with the captured origin so a keyboard-driven interaction + * keeps its focus ring, while a mouse-driven one does not. + * + * @docs-private + */ + protected restoreTriggerFocus(): void { + const trigger = this.elementRef.nativeElement.querySelector( + 'button:not(.kbq-pipe__remove-button)' + ); + + if (trigger) { + this.focusMonitor.focusVia(trigger, this.focusOrigin ?? 'keyboard'); + } + } + /** @docs-private */ abstract open(): void; } diff --git a/packages/components/filter-bar/pipes/pipe-date.spec.ts b/packages/components/filter-bar/pipes/pipe-date.spec.ts index 38bd81d65..e1a11d9a0 100644 --- a/packages/components/filter-bar/pipes/pipe-date.spec.ts +++ b/packages/components/filter-bar/pipes/pipe-date.spec.ts @@ -1,3 +1,4 @@ +import { FocusMonitor } from '@angular/cdk/a11y'; import { OverlayContainer } from '@angular/cdk/overlay'; import { ChangeDetectorRef, Component, DebugElement, inject, LOCALE_ID } from '@angular/core'; import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; @@ -393,11 +394,12 @@ describe('KbqPipeDateComponent', () => { setupSinglePipe({ value: null }); }); - it('should set data.value, emit onChangePipe and hide popover', () => { + it('should set data.value, emit onChangePipe, hide popover and restore focus', fakeAsync(() => { const component = getPipeComponent(); const filterBar = getFilterBar(); const spy = jest.fn(); const hide = jest.fn(); + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); asInternal(component).popover = () => ({ hide }); filterBar.onChangePipe.subscribe(spy); @@ -407,7 +409,11 @@ describe('KbqPipeDateComponent', () => { expect(component.data.value).toEqual({ name: 'test', start: '', end: '' }); expect(spy).toHaveBeenCalledWith(component.data); expect(hide).toHaveBeenCalled(); - }); + + flush(); + + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + })); }); describe('onApplyPeriod', () => { @@ -415,12 +421,13 @@ describe('KbqPipeDateComponent', () => { setupSinglePipe({ value: null }); }); - it('should save custom period as ISO strings and emit onChangePipe', () => { + it('should save custom period as ISO strings, emit onChangePipe and restore focus', fakeAsync(() => { const component = getPipeComponent(); const internal = asInternal(component); const filterBar = getFilterBar(); const spy = jest.fn(); const hide = jest.fn(); + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); const start = adapter.today().minus({ days: 5 }); const end = adapter.today(); @@ -439,7 +446,11 @@ describe('KbqPipeDateComponent', () => { }); expect(spy).toHaveBeenCalledWith(component.data); expect(hide).toHaveBeenCalled(); - }); + + flush(); + + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + })); }); describe('disabled', () => { @@ -573,6 +584,19 @@ describe('KbqPipeDateComponent', () => { expect(spy).toHaveBeenCalledWith(component.data); }); + + it('should focus the period list when the popover opens so Enter can pick a preset', fakeAsync(() => { + const component = getPipeComponent(); + const focus = jest.fn(); + + asInternal(component).isListMode = true; + asInternal(component).listSelection = () => ({ focus }); + + component.popover().visibleChange.emit(true); + flush(); + + expect(focus).toHaveBeenCalled(); + })); }); describe('onClear', () => { diff --git a/packages/components/filter-bar/pipes/pipe-date.ts b/packages/components/filter-bar/pipes/pipe-date.ts index db9d29acb..bd8a46463 100644 --- a/packages/components/filter-bar/pipes/pipe-date.ts +++ b/packages/components/filter-bar/pipes/pipe-date.ts @@ -18,7 +18,6 @@ import { KbqListModule, KbqListSelection } from '@koobiq/components/list'; import { KbqPopoverModule, KbqPopoverTrigger } from '@koobiq/components/popover'; import { KbqTimepickerModule } from '@koobiq/components/timepicker'; import { KbqTitleModule } from '@koobiq/components/title'; -import { filter } from 'rxjs/operators'; import { KbqDateTimeValue } from '../filter-bar.types'; import { KbqBasePipe } from './base-pipe'; import { KbqPipeButton } from './pipe-button'; @@ -141,11 +140,18 @@ export class KbqPipeDateComponent extends KbqBasePipe imple super.ngAfterViewInit(); this.popover() - .visibleChange.pipe( - filter((visible) => !visible), - takeUntilDestroyed(this.destroyRef) - ) - .subscribe(() => this.filterBar?.onClosePipe.emit(this.data)); + .visibleChange.pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((visible) => { + if (visible) { + // Move keyboard focus onto the period list so Enter selects a preset + // instead of the focus trap landing on the "custom period" row. + if (this.isListMode) { + setTimeout(() => this.listSelection().focus()); + } + } else { + this.filterBar?.onClosePipe.emit(this.data); + } + }); } /** keydown handler @@ -170,6 +176,8 @@ export class KbqPipeDateComponent extends KbqBasePipe imple this.filterBar?.onChangePipe.next(this.data); this.popover().hide(); + + setTimeout(() => this.restoreTriggerFocus()); } onSelect(item: KbqDateTimeValue) { @@ -179,6 +187,8 @@ export class KbqPipeDateComponent extends KbqBasePipe imple this.filterBar?.onChangePipe.next(this.data); this.popover().hide(); + + setTimeout(() => this.restoreTriggerFocus()); } showPeriod() { diff --git a/packages/components/filter-bar/pipes/pipe-datetime.spec.ts b/packages/components/filter-bar/pipes/pipe-datetime.spec.ts index a209f733c..0288da451 100644 --- a/packages/components/filter-bar/pipes/pipe-datetime.spec.ts +++ b/packages/components/filter-bar/pipes/pipe-datetime.spec.ts @@ -1,3 +1,4 @@ +import { FocusMonitor } from '@angular/cdk/a11y'; import { OverlayContainer } from '@angular/cdk/overlay'; import { ChangeDetectorRef, Component, DebugElement, inject, LOCALE_ID } from '@angular/core'; import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; @@ -393,11 +394,12 @@ describe('KbqPipeDatetimeComponent', () => { setupSinglePipe({ value: null }); }); - it('should set data.value, emit onChangePipe and hide popover', () => { + it('should set data.value, emit onChangePipe, hide popover and restore focus', fakeAsync(() => { const component = getPipeComponent(); const filterBar = getFilterBar(); const spy = jest.fn(); const hide = jest.fn(); + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); asInternal(component).popover = () => ({ hide }); filterBar.onChangePipe.subscribe(spy); @@ -407,7 +409,11 @@ describe('KbqPipeDatetimeComponent', () => { expect(component.data.value).toEqual({ name: 'test', start: '', end: '' }); expect(spy).toHaveBeenCalledWith(component.data); expect(hide).toHaveBeenCalled(); - }); + + flush(); + + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + })); }); describe('onApplyPeriod', () => { @@ -415,12 +421,13 @@ describe('KbqPipeDatetimeComponent', () => { setupSinglePipe({ value: null }); }); - it('should save custom period as ISO strings and emit onChangePipe', () => { + it('should save custom period as ISO strings, emit onChangePipe and restore focus', fakeAsync(() => { const component = getPipeComponent(); const internal = asInternal(component); const filterBar = getFilterBar(); const spy = jest.fn(); const hide = jest.fn(); + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); const start = adapter.today().minus({ days: 5 }).set({ hour: 8, minute: 30, second: 0, millisecond: 0 }); const end = adapter.today().set({ hour: 17, minute: 5, second: 0, millisecond: 0 }); @@ -439,7 +446,11 @@ describe('KbqPipeDatetimeComponent', () => { }); expect(spy).toHaveBeenCalledWith(component.data); expect(hide).toHaveBeenCalled(); - }); + + flush(); + + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + })); }); describe('disabled', () => { @@ -584,6 +595,19 @@ describe('KbqPipeDatetimeComponent', () => { expect(spy).toHaveBeenCalledWith(component.data); }); + + it('should focus the period list when the popover opens so Enter can pick a preset', fakeAsync(() => { + const component = getPipeComponent(); + const focus = jest.fn(); + + asInternal(component).isListMode = true; + asInternal(component).listSelection = () => ({ focus }); + + component.popover().visibleChange.emit(true); + flush(); + + expect(focus).toHaveBeenCalled(); + })); }); describe('onClear', () => { diff --git a/packages/components/filter-bar/pipes/pipe-datetime.ts b/packages/components/filter-bar/pipes/pipe-datetime.ts index 3d0bea8c3..8adebf518 100644 --- a/packages/components/filter-bar/pipes/pipe-datetime.ts +++ b/packages/components/filter-bar/pipes/pipe-datetime.ts @@ -18,7 +18,6 @@ import { KbqListModule, KbqListSelection } from '@koobiq/components/list'; import { KbqPopoverModule, KbqPopoverTrigger } from '@koobiq/components/popover'; import { KbqTimepickerModule } from '@koobiq/components/timepicker'; import { KbqTitleModule } from '@koobiq/components/title'; -import { filter } from 'rxjs/operators'; import { KbqDateTimeValue } from '../filter-bar.types'; import { KbqBasePipe } from './base-pipe'; import { KbqPipeButton } from './pipe-button'; @@ -141,11 +140,18 @@ export class KbqPipeDatetimeComponent extends KbqBasePipe i super.ngAfterViewInit(); this.popover() - .visibleChange.pipe( - filter((visible) => !visible), - takeUntilDestroyed(this.destroyRef) - ) - .subscribe(() => this.filterBar?.onClosePipe.emit(this.data)); + .visibleChange.pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((visible) => { + if (visible) { + // Move keyboard focus onto the period list so Enter selects a preset + // instead of the focus trap landing on the "custom period" row. + if (this.isListMode) { + setTimeout(() => this.listSelection().focus()); + } + } else { + this.filterBar?.onClosePipe.emit(this.data); + } + }); } /** keydown handler @@ -171,6 +177,8 @@ export class KbqPipeDatetimeComponent extends KbqBasePipe i this.filterBar?.onChangePipe.next(this.data); this.popover().hide(); + + setTimeout(() => this.restoreTriggerFocus()); } onSelect(item: KbqDateTimeValue) { @@ -180,6 +188,8 @@ export class KbqPipeDatetimeComponent extends KbqBasePipe i this.filterBar?.onChangePipe.next(this.data); this.popover().hide(); + + setTimeout(() => this.restoreTriggerFocus()); } showPeriod() { diff --git a/packages/components/filter-bar/pipes/pipe-multi-select.spec.ts b/packages/components/filter-bar/pipes/pipe-multi-select.spec.ts index 956b34b39..ea28032c7 100644 --- a/packages/components/filter-bar/pipes/pipe-multi-select.spec.ts +++ b/packages/components/filter-bar/pipes/pipe-multi-select.spec.ts @@ -1,3 +1,4 @@ +import { FocusMonitor } from '@angular/cdk/a11y'; import { ChangeDetectorRef, Component, DebugElement, inject } from '@angular/core'; import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; @@ -571,9 +572,24 @@ describe('KbqPipeMultiSelectComponent', () => { const component = getPipeComponent(); component.onClose(); + flush(); expect(component.selected).toBeDefined(); })); + + it('should restore focus to the trigger button on close', fakeAsync(() => { + fixture.componentInstance.activeFilter = createFilter([ + createPipe({ name: 'test', value: [] }) + ]); + fixture.detectChanges(); + + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); + + getPipeComponent().onClose(); + flush(); + + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + })); }); describe('open', () => { diff --git a/packages/components/filter-bar/pipes/pipe-multi-select.ts b/packages/components/filter-bar/pipes/pipe-multi-select.ts index ecf3458f3..f1efadfb6 100644 --- a/packages/components/filter-bar/pipes/pipe-multi-select.ts +++ b/packages/components/filter-bar/pipes/pipe-multi-select.ts @@ -217,6 +217,8 @@ export class KbqPipeMultiSelectComponent extends KbqBasePipe i if (this.allOptionsSelected) { this.updateInternalSelected(); } + + setTimeout(() => this.restoreTriggerFocus()); } /** opens select */ diff --git a/packages/components/filter-bar/pipes/pipe-multi-tree-select.spec.ts b/packages/components/filter-bar/pipes/pipe-multi-tree-select.spec.ts index 3060a28b6..f10054504 100644 --- a/packages/components/filter-bar/pipes/pipe-multi-tree-select.spec.ts +++ b/packages/components/filter-bar/pipes/pipe-multi-tree-select.spec.ts @@ -1,3 +1,4 @@ +import { FocusMonitor } from '@angular/cdk/a11y'; import { ChangeDetectorRef, Component, DebugElement, inject } from '@angular/core'; import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; @@ -597,10 +598,29 @@ describe('KbqPipeMultiTreeSelectComponent', () => { // call onClose directly component.onClose(); + flush(); // after close with all selected, internalSelected should be updated expect(component.selected).toBeDefined(); })); + + it('should restore focus to the trigger button on close', fakeAsync(() => { + fixture.componentInstance.activeFilter = createFilter([ + createPipe({ name: 'test', value: [], selectAll: true }) + ]); + fixture.detectChanges(); + + openSelect(); + flush(); + fixture.detectChanges(); + + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); + + getPipeComponent().onClose(); + flush(); + + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + })); }); describe('numberOfSelectedLeaves', () => { diff --git a/packages/components/filter-bar/pipes/pipe-multi-tree-select.ts b/packages/components/filter-bar/pipes/pipe-multi-tree-select.ts index e60a2d93c..0e4869633 100644 --- a/packages/components/filter-bar/pipes/pipe-multi-tree-select.ts +++ b/packages/components/filter-bar/pipes/pipe-multi-tree-select.ts @@ -278,6 +278,8 @@ export class KbqPipeMultiTreeSelectComponent extends KbqBasePipe this.restoreTriggerFocus()); } /** handler for select all options in select */ diff --git a/packages/components/filter-bar/pipes/pipe-select.spec.ts b/packages/components/filter-bar/pipes/pipe-select.spec.ts index cdb7952bd..fb69249f7 100644 --- a/packages/components/filter-bar/pipes/pipe-select.spec.ts +++ b/packages/components/filter-bar/pipes/pipe-select.spec.ts @@ -1,7 +1,9 @@ +import { FocusMonitor } from '@angular/cdk/a11y'; import { ChangeDetectorRef, Component, DebugElement, inject } from '@angular/core'; import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; +import { dispatchKeyboardEvent, ENTER } from '@koobiq/components/core'; import { KbqFilter, KbqFilterBar, @@ -304,6 +306,50 @@ describe('KbqPipeSelectComponent', () => { })); }); + describe('keyboard selection & focus restore', () => { + beforeEach(() => { + fixture = TestBed.createComponent(TestComponent); + filterBarDebugElement = fixture.debugElement.query(By.directive(KbqFilterBar)); + }); + + const enterSelectsAndRestoresFocus = (search: boolean) => { + fixture.componentInstance.activeFilter = createFilter([createPipe({ name: 'test', value: null, search })]); + fixture.detectChanges(); + + const component = getPipeComponent(); + const filterBar = getFilterBar(); + const changeSpy = jest.fn(); + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); + + filterBar.onChangePipe.subscribe(changeSpy); + + const selectEl = fixture.debugElement.query(By.css('kbq-select')).nativeElement; + + component.select().open(); + flush(); + fixture.detectChanges(); + + // Highlight the first option, then confirm it with Enter. + component.select().keyManager.setActiveItem(0); + dispatchKeyboardEvent(selectEl, 'keydown', ENTER); + flush(); + fixture.detectChanges(); + + expect(changeSpy).toHaveBeenCalled(); + expect(component.data.value).toEqual(SELECT_VALUES[0]); + expect(component.select().panelOpen).toBe(false); + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + }; + + it('should select the active option with Enter and restore focus (no search)', fakeAsync(() => { + enterSelectsAndRestoresFocus(false); + })); + + it('should select the active option with Enter and restore focus (with search)', fakeAsync(() => { + enterSelectsAndRestoresFocus(true); + })); + }); + describe('compareByValue', () => { beforeEach(() => { fixture = TestBed.createComponent(TestComponent); diff --git a/packages/components/filter-bar/pipes/pipe-select.ts b/packages/components/filter-bar/pipes/pipe-select.ts index 2e9157747..094417dcb 100644 --- a/packages/components/filter-bar/pipes/pipe-select.ts +++ b/packages/components/filter-bar/pipes/pipe-select.ts @@ -81,6 +81,8 @@ export class KbqPipeSelectComponent extends KbqBasePipe implemen this.data.value = item; this.filterBar?.onChangePipe.emit(this.data); this.stateChanges.next(); + + setTimeout(() => this.restoreTriggerFocus()); } /** Comparator of selected options */ diff --git a/packages/components/filter-bar/pipes/pipe-text.spec.ts b/packages/components/filter-bar/pipes/pipe-text.spec.ts index 408091e70..f5f8f01ab 100644 --- a/packages/components/filter-bar/pipes/pipe-text.spec.ts +++ b/packages/components/filter-bar/pipes/pipe-text.spec.ts @@ -1,5 +1,6 @@ +import { FocusMonitor } from '@angular/cdk/a11y'; import { ChangeDetectorRef, Component, DebugElement, inject } from '@angular/core'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; import { @@ -254,7 +255,7 @@ describe('KbqPipeTextComponent', () => { filterBarDebugElement = fixture.debugElement.query(By.directive(KbqFilterBar)); }); - it('should set data.value from control value', () => { + it('should set data.value from control value', fakeAsync(() => { fixture.componentInstance.activeFilter = createFilter([createPipe({ value: null })]); fixture.detectChanges(); @@ -262,11 +263,12 @@ describe('KbqPipeTextComponent', () => { component.control.setValue('new text'); component.onApply(); + flush(); expect(component.data.value).toBe('new text'); - }); + })); - it('should mark control as pristine after apply', () => { + it('should mark control as pristine after apply', fakeAsync(() => { fixture.componentInstance.activeFilter = createFilter([createPipe({ value: null })]); fixture.detectChanges(); @@ -275,11 +277,12 @@ describe('KbqPipeTextComponent', () => { component.control.setValue('new text'); component.control.markAsDirty(); component.onApply(); + flush(); expect(component.control.pristine).toBe(true); - }); + })); - it('should call popover.hide()', () => { + it('should call popover.hide()', fakeAsync(() => { fixture.componentInstance.activeFilter = createFilter([createPipe({ value: 'some text' })]); fixture.detectChanges(); @@ -287,11 +290,24 @@ describe('KbqPipeTextComponent', () => { const hideSpy = jest.spyOn(component.popover(), 'hide'); component.onApply(); + flush(); expect(hideSpy).toHaveBeenCalled(); - }); + })); - it('should emit onChangePipe event', () => { + it('should restore focus to the trigger button after apply', fakeAsync(() => { + fixture.componentInstance.activeFilter = createFilter([createPipe({ value: 'some text' })]); + fixture.detectChanges(); + + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); + + getPipeComponent().onApply(); + flush(); + + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + })); + + it('should emit onChangePipe event', fakeAsync(() => { fixture.componentInstance.activeFilter = createFilter([createPipe({ value: 'some text' })]); fixture.detectChanges(); @@ -301,9 +317,10 @@ describe('KbqPipeTextComponent', () => { filterBar.onChangePipe.subscribe(spy); getPipeComponent().onApply(); + flush(); expect(spy).toHaveBeenCalled(); - }); + })); }); describe('onKeydown', () => { diff --git a/packages/components/filter-bar/pipes/pipe-text.ts b/packages/components/filter-bar/pipes/pipe-text.ts index 44d0f0aa6..02fdc6f75 100644 --- a/packages/components/filter-bar/pipes/pipe-text.ts +++ b/packages/components/filter-bar/pipes/pipe-text.ts @@ -78,6 +78,8 @@ export class KbqPipeTextComponent extends KbqBasePipe implements this.popover().hide(); this.filterBar?.onChangePipe.next(this.data); + + setTimeout(() => this.restoreTriggerFocus()); } /** @docs-private */ diff --git a/packages/components/filter-bar/pipes/pipe-tree-select.spec.ts b/packages/components/filter-bar/pipes/pipe-tree-select.spec.ts index 535dfdc9e..1f78e3308 100644 --- a/packages/components/filter-bar/pipes/pipe-tree-select.spec.ts +++ b/packages/components/filter-bar/pipes/pipe-tree-select.spec.ts @@ -1,3 +1,4 @@ +import { FocusMonitor } from '@angular/cdk/a11y'; import { ChangeDetectorRef, Component, DebugElement, inject } from '@angular/core'; import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; @@ -313,6 +314,20 @@ describe('KbqPipeTreeSelectComponent', () => { expect(closeSpy).toHaveBeenCalled(); })); + + it('should restore focus to the trigger button after selection', fakeAsync(() => { + fixture.componentInstance.activeFilter = createFilter([ + createPipe({ name: 'test', value: null }) + ]); + fixture.detectChanges(); + + const focusViaSpy = jest.spyOn(TestBed.inject(FocusMonitor), 'focusVia'); + + getPipeComponent().onSelect({ value: SINGLE_VALUE } as KbqTreeOption); + flush(); + + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + })); }); describe('updateTemplates', () => { diff --git a/packages/components/filter-bar/pipes/pipe-tree-select.ts b/packages/components/filter-bar/pipes/pipe-tree-select.ts index 7fab8a672..c10115977 100644 --- a/packages/components/filter-bar/pipes/pipe-tree-select.ts +++ b/packages/components/filter-bar/pipes/pipe-tree-select.ts @@ -114,7 +114,7 @@ export class KbqPipeTreeSelectComponent extends KbqBasePipe impl this.data.value = item.value; this.filterBar?.onChangePipe.emit(this.data); this.select().close(); - setTimeout(() => this.select().focus()); + setTimeout(() => this.restoreTriggerFocus()); this.stateChanges.next(); } From 84c126b2e3c7644291588b8432ac456e3d97857d Mon Sep 17 00:00:00 2001 From: lskramarov Date: Fri, 3 Jul 2026 12:06:07 +0300 Subject: [PATCH 2/2] fix: after review --- .../components/filter-bar/pipe-add.spec.ts | 26 +++++++++++++++++-- packages/components/filter-bar/pipe-add.ts | 1 + .../components/filter-bar/pipes/base-pipe.ts | 25 ++++++++++++++++-- .../components/filter-bar/pipes/pipe-date.ts | 8 +++--- .../filter-bar/pipes/pipe-datetime.ts | 8 +++--- .../filter-bar/pipes/pipe-select.spec.ts | 3 ++- .../components/filter-bar.api.md | 7 +++-- 7 files changed, 65 insertions(+), 13 deletions(-) diff --git a/packages/components/filter-bar/pipe-add.spec.ts b/packages/components/filter-bar/pipe-add.spec.ts index 131afba19..dc389b5e0 100644 --- a/packages/components/filter-bar/pipe-add.spec.ts +++ b/packages/components/filter-bar/pipe-add.spec.ts @@ -2,7 +2,7 @@ import { ChangeDetectorRef, Component, DebugElement, inject } from '@angular/cor import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; -import { createKeyboardEvent, dispatchEvent, ENTER } from '@koobiq/components/core'; +import { createKeyboardEvent, dispatchEvent, ENTER, SPACE } from '@koobiq/components/core'; import { KbqFilter, KbqFilterBar, @@ -414,7 +414,7 @@ describe('KbqPipeAdd', () => { })); }); - describe('keyboard (Enter)', () => { + describe('keyboard (Enter/Space)', () => { beforeEach(() => { fixture = TestBed.createComponent(TestComponent); filterBarDebugElement = fixture.debugElement.query(By.directive(KbqFilterBar)); @@ -427,6 +427,12 @@ describe('KbqPipeAdd', () => { dispatchEvent(option, createKeyboardEvent('keydown', ENTER, undefined, 'Enter')); }; + const pressSpaceOnFirstOption = () => { + const option = document.querySelectorAll('.kbq-option')[0] as HTMLElement; + + dispatchEvent(option, createKeyboardEvent('keydown', SPACE, undefined, ' ')); + }; + it('should add a pipe when Enter is pressed on a template option', fakeAsync(() => { const filterBar = getFilterBar(); const pipeAdd = getPipeAdd(); @@ -443,6 +449,22 @@ describe('KbqPipeAdd', () => { expect(filterBar.filter!.pipes[0].id).toBe(PIPE_TEMPLATE_ID_1); })); + it('should add a pipe when Space is pressed on a template option', fakeAsync(() => { + const filterBar = getFilterBar(); + const pipeAdd = getPipeAdd(); + + pipeAdd.select().open(); + flush(); + fixture.detectChanges(); + + pressSpaceOnFirstOption(); + flush(); + fixture.detectChanges(); + + expect(filterBar.filter!.pipes.length).toBe(1); + expect(filterBar.filter!.pipes[0].id).toBe(PIPE_TEMPLATE_ID_1); + })); + it('should emit onAddPipe when Enter is pressed on a template option', fakeAsync(() => { const pipeAdd = getPipeAdd(); const spy = jest.fn(); diff --git a/packages/components/filter-bar/pipe-add.ts b/packages/components/filter-bar/pipe-add.ts index 9263076f2..72b438291 100644 --- a/packages/components/filter-bar/pipe-add.ts +++ b/packages/components/filter-bar/pipe-add.ts @@ -41,6 +41,7 @@ import { getId } from './pipes/base-pipe'; [showCheckbox]="false" (click)="addPipeFromTemplate(option)" (keydown.enter)="addPipeFromTemplate(option)" + (keydown.space)="addPipeFromTemplate(option)" > {{ template.name }} diff --git a/packages/components/filter-bar/pipes/base-pipe.ts b/packages/components/filter-bar/pipes/base-pipe.ts index 53464c59e..8719cc861 100644 --- a/packages/components/filter-bar/pipes/base-pipe.ts +++ b/packages/components/filter-bar/pipes/base-pipe.ts @@ -1,4 +1,5 @@ -import { FocusMonitor, FocusOrigin } from '@angular/cdk/a11y'; +import { FocusMonitor, FocusOrigin, InputModalityDetector } from '@angular/cdk/a11y'; +import { DOCUMENT } from '@angular/common'; import { afterNextRender, AfterViewInit, @@ -52,6 +53,8 @@ export abstract class KbqBasePipe implements AfterViewInit { protected readonly focusMonitor = inject(FocusMonitor); /** @docs-private */ protected readonly elementRef = inject>(ElementRef); + private readonly inputModalityDetector = inject(InputModalityDetector); + private readonly document = inject(DOCUMENT); /** Last known focus origin within the pipe. Used to preserve the keyboard focus ring on restore. */ private focusOrigin: FocusOrigin = null; @@ -174,12 +177,30 @@ export abstract class KbqBasePipe implements AfterViewInit { * @docs-private */ protected restoreTriggerFocus(): void { + const active = this.document.activeElement; + + // The panel may have been closed by moving focus to another interactive element + // (e.g. an outside click) — stealing focus back would break the user's intent. + // Focus inside the pipe or inside a not-yet-detached overlay is fine to take over. + if ( + active && + active !== this.document.body && + !this.elementRef.nativeElement.contains(active) && + !active.closest('.cdk-overlay-container') + ) { + return; + } + const trigger = this.elementRef.nativeElement.querySelector( 'button:not(.kbq-pipe__remove-button)' ); + // A pipe opened right after being added from pipe-add never received focus itself, + // so `focusOrigin` is unknown there — fall back to the detected input modality. + const origin = this.focusOrigin ?? this.inputModalityDetector.mostRecentModality ?? 'program'; + if (trigger) { - this.focusMonitor.focusVia(trigger, this.focusOrigin ?? 'keyboard'); + this.focusMonitor.focusVia(trigger, origin); } } diff --git a/packages/components/filter-bar/pipes/pipe-date.ts b/packages/components/filter-bar/pipes/pipe-date.ts index bd8a46463..dce50cc9e 100644 --- a/packages/components/filter-bar/pipes/pipe-date.ts +++ b/packages/components/filter-bar/pipes/pipe-date.ts @@ -132,7 +132,9 @@ export class KbqPipeDateComponent extends KbqBasePipe imple /** @docs-private */ readonly popover = viewChild.required('popover'); /** @docs-private */ - listSelection = viewChild.required('listSelection', { read: KbqListSelection }); + // Optional: the list only exists while the popover is open in list mode, and it is read + // in deferred callbacks that may fire after the popover has already closed. + listSelection = viewChild('listSelection', { read: KbqListSelection }); /** @docs-private */ returnButton = viewChild.required('returnButton', { read: KbqButton }); @@ -146,7 +148,7 @@ export class KbqPipeDateComponent extends KbqBasePipe imple // Move keyboard focus onto the period list so Enter selects a preset // instead of the focus trap landing on the "custom period" row. if (this.isListMode) { - setTimeout(() => this.listSelection().focus()); + setTimeout(() => this.listSelection()?.focus()); } } else { this.filterBar?.onClosePipe.emit(this.data); @@ -207,7 +209,7 @@ export class KbqPipeDateComponent extends KbqBasePipe imple showList() { this.isListMode = true; - setTimeout(() => this.listSelection().focus()); + setTimeout(() => this.listSelection()?.focus()); this.popover().updatePosition(true); } diff --git a/packages/components/filter-bar/pipes/pipe-datetime.ts b/packages/components/filter-bar/pipes/pipe-datetime.ts index 8adebf518..f44caa47f 100644 --- a/packages/components/filter-bar/pipes/pipe-datetime.ts +++ b/packages/components/filter-bar/pipes/pipe-datetime.ts @@ -132,7 +132,9 @@ export class KbqPipeDatetimeComponent extends KbqBasePipe i /** @docs-private */ readonly popover = viewChild.required('popover'); /** @docs-private */ - listSelection = viewChild.required('listSelection', { read: KbqListSelection }); + // Optional: the list only exists while the popover is open in list mode, and it is read + // in deferred callbacks that may fire after the popover has already closed. + listSelection = viewChild('listSelection', { read: KbqListSelection }); /** @docs-private */ returnButton = viewChild.required('returnButton', { read: KbqButton }); @@ -146,7 +148,7 @@ export class KbqPipeDatetimeComponent extends KbqBasePipe i // Move keyboard focus onto the period list so Enter selects a preset // instead of the focus trap landing on the "custom period" row. if (this.isListMode) { - setTimeout(() => this.listSelection().focus()); + setTimeout(() => this.listSelection()?.focus()); } } else { this.filterBar?.onClosePipe.emit(this.data); @@ -208,7 +210,7 @@ export class KbqPipeDatetimeComponent extends KbqBasePipe i showList() { this.isListMode = true; - setTimeout(() => this.listSelection().focus()); + setTimeout(() => this.listSelection()?.focus()); this.popover().updatePosition(true); } diff --git a/packages/components/filter-bar/pipes/pipe-select.spec.ts b/packages/components/filter-bar/pipes/pipe-select.spec.ts index fb69249f7..e7b1669c9 100644 --- a/packages/components/filter-bar/pipes/pipe-select.spec.ts +++ b/packages/components/filter-bar/pipes/pipe-select.spec.ts @@ -338,7 +338,8 @@ describe('KbqPipeSelectComponent', () => { expect(changeSpy).toHaveBeenCalled(); expect(component.data.value).toEqual(SELECT_VALUES[0]); expect(component.select().panelOpen).toBe(false); - expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), expect.anything()); + // The keyboard origin is what actually keeps the focus ring visible. + expect(focusViaSpy).toHaveBeenCalledWith(expect.any(HTMLButtonElement), 'keyboard'); }; it('should select the active option with Enter and restore focus (no search)', fakeAsync(() => { diff --git a/tools/public_api_guard/components/filter-bar.api.md b/tools/public_api_guard/components/filter-bar.api.md index e504d8129..198589b11 100644 --- a/tools/public_api_guard/components/filter-bar.api.md +++ b/tools/public_api_guard/components/filter-bar.api.md @@ -108,7 +108,9 @@ export abstract class KbqBasePipe implements AfterViewInit { protected readonly changeDetectorRef: ChangeDetectorRef; readonly data: KbqPipeData; protected readonly destroyRef: DestroyRef; + protected readonly elementRef: ElementRef; protected readonly filterBar: KbqFilterBar | null; + protected readonly focusMonitor: FocusMonitor; get isEmpty(): boolean; isMac: boolean; isTemplateRef(value: unknown): boolean; @@ -118,6 +120,7 @@ export abstract class KbqBasePipe implements AfterViewInit { onClear(): void; onRemove(): void; abstract open(): void; + protected restoreTriggerFocus(): void; get showRemoveButton(): boolean; readonly stateChanges: Subject; updateTemplates: (templates: KbqPipeTemplate[] | null) => void; @@ -401,7 +404,7 @@ export class KbqPipeDateComponent extends KbqBasePipe imple hideCalendars(): void; get isEmpty(): boolean; protected isListMode: boolean; - listSelection: i0.Signal; + listSelection: i0.Signal; // (undocumented) ngAfterViewInit(): void; // (undocumented) @@ -448,7 +451,7 @@ export class KbqPipeDatetimeComponent extends KbqBasePipe i hideCalendars(): void; get isEmpty(): boolean; protected isListMode: boolean; - listSelection: i0.Signal; + listSelection: i0.Signal; // (undocumented) ngAfterViewInit(): void; onApplyPeriod(): void;