From 524d7f668762321b6dc2372b959aa839883980a0 Mon Sep 17 00:00:00 2001 From: Richard Helm Date: Tue, 18 Feb 2025 11:25:26 +0000 Subject: [PATCH 1/7] Prepare inline-time-picker for adoption --- .../inline-time-picker.spec.ts | 52 ++++++++++++++----- .../inline-time-picker.template.ts | 34 +++++++++--- .../inline-time-picker/inline-time-picker.ts | 8 +++ .../src/shared/patterns/trapped-focus.spec.ts | 14 +++++ .../src/shared/patterns/trapped-focus.ts | 8 ++- 5 files changed, 96 insertions(+), 20 deletions(-) diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts index 569cde4438..973230fd9b 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts @@ -1,5 +1,7 @@ import { elementUpdated, fixture } from '@vivid-nx/shared'; -import { InlineTimePicker } from './inline-time-picker.ts'; +import { beforeEach } from 'vitest'; +import { TrappedFocus } from '../../../shared/patterns'; +import { InlineTimePicker } from './inline-time-picker'; import '.'; const COMPONENT_TAG = 'vwc-inline-time-picker'; @@ -24,17 +26,15 @@ describe('vwc-inline-time-picker', () => { const getLabels = (type: 'hours' | 'minutes' | 'seconds' | 'meridies') => getAllPickerItems(type).map((item) => item.innerHTML.trim()); - const pressKey = ( - key: string, - options: KeyboardEventInit = {}, - triggerElement = false - ) => { - const triggeredElement = triggerElement - ? element - : element.shadowRoot!.activeElement; - triggeredElement!.dispatchEvent( - new KeyboardEvent('keydown', { key, bubbles: true, ...options }) - ); + const pressKey = (key: string, options: KeyboardEventInit = {}) => { + const event = new KeyboardEvent('keydown', { + key, + bubbles: true, + composed: true, + ...options, + }); + element.shadowRoot!.activeElement!.dispatchEvent(event); + return event; }; const isScrolledToTop = (element: HTMLElement) => @@ -596,5 +596,33 @@ describe('vwc-inline-time-picker', () => { expect(isScrolledIntoView(getPickerItem('hours', '00'))).toBe(true); }); }); + + describe('focus trap support', () => { + const originalIgnoreEvent = TrappedFocus.ignoreEvent; + beforeEach(() => { + TrappedFocus.ignoreEvent = vi.fn(); + }); + afterEach(() => { + TrappedFocus.ignoreEvent = originalIgnoreEvent; + }); + + it('should submit Tab keydown events that move focus internally to be ignored by focus traps', () => { + (element.shadowRoot!.querySelector('#hours') as HTMLElement).focus(); + + const event = pressKey('Tab'); + + expect(TrappedFocus.ignoreEvent).toHaveBeenCalledTimes(1); + expect(TrappedFocus.ignoreEvent).toHaveBeenCalledWith(event); + }); + + it('should not submit Tab keydown events that move focus out', () => { + (element.shadowRoot!.querySelector('#hours') as HTMLElement).focus(); + pressKey('Tab', { shiftKey: true }); + (element.shadowRoot!.querySelector('#minutes') as HTMLElement).focus(); + pressKey('Tab'); + + expect(TrappedFocus.ignoreEvent).not.toHaveBeenCalled(); + }); + }); }); }); diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts index 399a4fe705..30e6b5b381 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts @@ -2,6 +2,7 @@ import { html, repeat, when } from '@microsoft/fast-element'; import { classNames } from '@microsoft/fast-web-utilities'; import { type PickerOption } from '../time/picker'; import { scrollIntoView } from '../../../shared/utils/scrollIntoView'; +import { TrappedFocus } from '../../../shared/patterns'; import type { InlineTimePicker } from './inline-time-picker'; import { type Column, @@ -20,9 +21,7 @@ const onPickerOptionClick = ( column: Column, optionValue: string ) => { - x.$emit('change', column.updatedValue(x, optionValue), { - bubbles: false, - }); + emitChange(x, column.updatedValue(x, optionValue)); scrollToOption(x, column.id, optionValue, 'start'); @@ -57,9 +56,7 @@ const onPickerKeyDown = ( const newRawIndex = index === -1 ? 0 : index + offset; const newIndex = (newRawIndex + options.length) % options.length; const newValue = options[newIndex].value; - x.$emit('change', column.updatedValue(x, newValue), { - bubbles: false, - }); + emitChange(x, column.updatedValue(x, newValue)); scrollToOption(x, column.id, newValue, 'nearest'); } @@ -82,6 +79,26 @@ export const scrollToOption = ( scrollIntoView(element, element.parentElement!, position); }; +const onBaseKeyDown = (x: InlineTimePicker, event: KeyboardEvent) => { + if (event.key === 'Tab') { + const focusableEls = x.shadowRoot!.querySelectorAll('.picker'); + const terminalElement = event.shiftKey + ? focusableEls[0] + : focusableEls[focusableEls.length - 1]; + + if (x.shadowRoot!.activeElement !== terminalElement) { + // TrappedFocus needs to ignore events that will not move focus out of + // the inline time picker + TrappedFocus.ignoreEvent(event); + } + } + return true; +}; + +const emitChange = (x: InlineTimePicker, time: string) => { + x.$emit('change', time, { bubbles: false, composed: false }); +}; + /** * Renders a picker for hours/minutes/etc. using a listbox pattern. */ @@ -119,7 +136,10 @@ const renderPicker = (column: Column) => { }; export const InlineTimePickerTemplate = () => { - return html`
+ return html`
${renderPicker(HoursColumn)} ${renderPicker(MinutesColumn)} ${when(shouldDisplaySecondsPicker, renderPicker(SecondsColumn))} ${when(shouldDisplay12hClock, renderPicker(MeridiesColumn))} diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.ts index 97f0fc2f57..e5c0f04be5 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.ts @@ -70,6 +70,14 @@ export class InlineTimePicker extends VividElement { ); } } + + override focus(options?: FocusOptions) { + // Override focus instead of relying on default behavior to prevent visible focus + const firstFocusableElement = this.shadowRoot!.querySelector( + '.picker' + ) as HTMLElement; + firstFocusableElement.focus(options); + } } export interface InlineTimePicker extends Localized {} diff --git a/libs/components/src/shared/patterns/trapped-focus.spec.ts b/libs/components/src/shared/patterns/trapped-focus.spec.ts index 62cdc370a2..74c7a945aa 100644 --- a/libs/components/src/shared/patterns/trapped-focus.spec.ts +++ b/libs/components/src/shared/patterns/trapped-focus.spec.ts @@ -80,4 +80,18 @@ describe('TrappedFocus', () => { expect(event.preventDefault).not.toHaveBeenCalled(); expect(element.shadowRoot!.activeElement).toBe(secondButton); }); + + describe('ignoreEvent', () => { + it('should cause the event to be ignored', () => { + lastButton.focus(); + const event = new KeyboardEvent('keydown', { key: 'Tab' }); + event.preventDefault = vi.fn(); + + TrappedFocus.ignoreEvent(event); + element.dispatchEvent(event); + + expect(event.preventDefault).not.toHaveBeenCalled(); + expect(element.shadowRoot!.activeElement).toBe(lastButton); + }); + }); }); diff --git a/libs/components/src/shared/patterns/trapped-focus.ts b/libs/components/src/shared/patterns/trapped-focus.ts index c5be7a96b3..3e3bc24fb3 100644 --- a/libs/components/src/shared/patterns/trapped-focus.ts +++ b/libs/components/src/shared/patterns/trapped-focus.ts @@ -1,4 +1,10 @@ export class TrappedFocus { + private static ignoredEvents = new WeakSet(); + + static ignoreEvent(event: Event) { + this.ignoredEvents.add(event); + } + /** * @returns Whether focus was trapped. * @internal @@ -7,7 +13,7 @@ export class TrappedFocus { event: KeyboardEvent, getFocusableEls: () => NodeListOf ) { - if (event.key === 'Tab') { + if (!TrappedFocus.ignoredEvents.has(event) && event.key === 'Tab') { const focusableEls = getFocusableEls(); const firstFocusableEl = focusableEls[0]; const lastFocusableEl = focusableEls[focusableEls.length - 1]; From f8325beda17379b54078d8e2af8275d3ff490b91 Mon Sep 17 00:00:00 2001 From: Richard Helm Date: Tue, 18 Feb 2025 11:36:05 +0000 Subject: [PATCH 2/7] Rename variable --- .../inline-time-picker/inline-time-picker.template.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts index 30e6b5b381..2f1b5a906f 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts @@ -81,10 +81,10 @@ export const scrollToOption = ( const onBaseKeyDown = (x: InlineTimePicker, event: KeyboardEvent) => { if (event.key === 'Tab') { - const focusableEls = x.shadowRoot!.querySelectorAll('.picker'); + const focusableElements = x.shadowRoot!.querySelectorAll('.picker'); const terminalElement = event.shiftKey - ? focusableEls[0] - : focusableEls[focusableEls.length - 1]; + ? focusableElements[0] + : focusableElements[focusableElements.length - 1]; if (x.shadowRoot!.activeElement !== terminalElement) { // TrappedFocus needs to ignore events that will not move focus out of From 0f60790a8f7e43976637617e91d5a117d052e7a0 Mon Sep 17 00:00:00 2001 From: Richard Helm Date: Tue, 18 Feb 2025 11:41:42 +0000 Subject: [PATCH 3/7] Add test for focus method --- .../inline-time-picker/inline-time-picker.spec.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts index 973230fd9b..74d3835009 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts @@ -625,4 +625,19 @@ describe('vwc-inline-time-picker', () => { }); }); }); + + describe('focus method', () => { + it('should focus the first picker programmatically to avoid visual focus', async () => { + const firstPicker = element.shadowRoot!.querySelector( + '#hours' + ) as HTMLElement; + const focusSpy = vi.spyOn(firstPicker, 'focus'); + const options = {}; + + element.focus(options); + + expect(element.shadowRoot!.activeElement).toBe(firstPicker); + expect(focusSpy).toHaveBeenCalledWith(options); + }); + }); }); From 293bcd044ecef4a424d63ba223e853589689cbf3 Mon Sep 17 00:00:00 2001 From: Richard Helm Date: Mon, 24 Feb 2025 15:47:05 +0000 Subject: [PATCH 4/7] Improve focus trap tests --- .../inline-time-picker.spec.ts | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts index 74d3835009..bd3d191919 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts @@ -595,33 +595,35 @@ describe('vwc-inline-time-picker', () => { expect(isScrolledIntoView(getPickerItem('hours', '00'))).toBe(true); }); - }); - - describe('focus trap support', () => { - const originalIgnoreEvent = TrappedFocus.ignoreEvent; - beforeEach(() => { - TrappedFocus.ignoreEvent = vi.fn(); - }); - afterEach(() => { - TrappedFocus.ignoreEvent = originalIgnoreEvent; - }); - - it('should submit Tab keydown events that move focus internally to be ignored by focus traps', () => { - (element.shadowRoot!.querySelector('#hours') as HTMLElement).focus(); - - const event = pressKey('Tab'); - - expect(TrappedFocus.ignoreEvent).toHaveBeenCalledTimes(1); - expect(TrappedFocus.ignoreEvent).toHaveBeenCalledWith(event); - }); - - it('should not submit Tab keydown events that move focus out', () => { - (element.shadowRoot!.querySelector('#hours') as HTMLElement).focus(); - pressKey('Tab', { shiftKey: true }); - (element.shadowRoot!.querySelector('#minutes') as HTMLElement).focus(); - pressKey('Tab'); - expect(TrappedFocus.ignoreEvent).not.toHaveBeenCalled(); + describe('focus trap support', () => { + const originalIgnoreEvent = TrappedFocus.ignoreEvent; + beforeEach(() => { + TrappedFocus.ignoreEvent = vi.fn(); + }); + afterEach(() => { + TrappedFocus.ignoreEvent = originalIgnoreEvent; + }); + + it('should call focus trap ignoreEvent on tab keydown when focused on non-terminal picker element', () => { + (element.shadowRoot!.querySelector('#hours') as HTMLElement).focus(); + + const event = pressKey('Tab'); + + expect(TrappedFocus.ignoreEvent).toHaveBeenCalledTimes(1); + expect(TrappedFocus.ignoreEvent).toHaveBeenCalledWith(event); + }); + + it('should not call focus trap ignoreEvent on tab keydown when focused on terminal picker element', () => { + (element.shadowRoot!.querySelector('#hours') as HTMLElement).focus(); + pressKey('Tab', { shiftKey: true }); + ( + element.shadowRoot!.querySelector('#minutes') as HTMLElement + ).focus(); + pressKey('Tab'); + + expect(TrappedFocus.ignoreEvent).not.toHaveBeenCalled(); + }); }); }); }); From ae1b3cacdfbde04c2b186cf9d65f90043c7e102b Mon Sep 17 00:00:00 2001 From: Richard Helm Date: Mon, 24 Feb 2025 15:50:16 +0000 Subject: [PATCH 5/7] Add test for composed event --- .../inline-time-picker/inline-time-picker.spec.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts index bd3d191919..741bab631c 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts @@ -329,6 +329,17 @@ describe('vwc-inline-time-picker', () => { expect.objectContaining({ bubbles: false }) ); }); + + it('should not be composed', () => { + const spy = vi.fn(); + element.addEventListener('change', spy); + + getPickerItem('hours', '03').click(); + + expect(spy).toHaveBeenCalledWith( + expect.objectContaining({ composed: false }) + ); + }); }); describe('last-column-selected event', () => { From 1a84d757e8b6ebe38f0fb7de244b46ea18e31aec Mon Sep 17 00:00:00 2001 From: Richard Helm Date: Tue, 25 Feb 2025 10:19:45 +0000 Subject: [PATCH 6/7] Split test --- .../inline-time-picker/inline-time-picker.spec.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts index 741bab631c..56f650885d 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts @@ -625,9 +625,14 @@ describe('vwc-inline-time-picker', () => { expect(TrappedFocus.ignoreEvent).toHaveBeenCalledWith(event); }); - it('should not call focus trap ignoreEvent on tab keydown when focused on terminal picker element', () => { + it('should prevent call to focus trap ignoreEvent on shift + tab keydown when focused on first picker element', () => { (element.shadowRoot!.querySelector('#hours') as HTMLElement).focus(); pressKey('Tab', { shiftKey: true }); + + expect(TrappedFocus.ignoreEvent).not.toHaveBeenCalled(); + }); + + it('should prevent call to focus trap ignoreEvent on tab keydown when focused on last picker element', () => { ( element.shadowRoot!.querySelector('#minutes') as HTMLElement ).focus(); From 74cd8c7e09ac25a71094b954409c79d70051687c Mon Sep 17 00:00:00 2001 From: Richard Helm <86777660+RichardHelm@users.noreply.github.com> Date: Thu, 27 Feb 2025 20:00:15 +0000 Subject: [PATCH 7/7] Update libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts --- .../time-picker/inline-time-picker/inline-time-picker.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts index 56f650885d..f0f8e6d07e 100644 --- a/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts +++ b/libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.spec.ts @@ -330,7 +330,7 @@ describe('vwc-inline-time-picker', () => { ); }); - it('should not be composed', () => { + it('should be emitted with composed set to false', () => { const spy = vi.fn(); element.addEventListener('change', spy);