Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(inline-time-picker): support focus and focus traps (VIV-2296) #2146

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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) =>
Expand Down Expand Up @@ -596,5 +596,48 @@ describe('vwc-inline-time-picker', () => {
expect(isScrolledIntoView(getPickerItem('hours', '00'))).toBe(true);
});
});

describe('focus trap support', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO describes should describe API's.
I know there's more like this in our codebase. It should be refactored.
Anyway, it describes nothing but a single use case so should be removed and its it remain under keyboard interactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think we should use describe to group related behaviour. This is similar to how we have describe("form association") in the codebase.

I don't want to move this under "time picker > keyboard navigation" as it has nothing to do with keyboard navigation for picking times

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What scenario does it test? What user action?

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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I see in this test is:
Focus on hours, press tab, expect ignoreEvent method to be called once with the Tab event.
Can you please write the description in the form of: should {doSomething} when {someThing}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how will you understand the purpose without this description? I think it will not become clearer by phrasing it like that

(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 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);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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');

Expand Down Expand Up @@ -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');
}

Expand All @@ -82,6 +79,26 @@ export const scrollToOption = (
scrollIntoView(element, element.parentElement!, position);
};

const onBaseKeyDown = (x: InlineTimePicker, event: KeyboardEvent) => {
if (event.key === 'Tab') {
const focusableElements = x.shadowRoot!.querySelectorAll('.picker');
const terminalElement = event.shiftKey
? focusableElements[0]
: focusableElements[focusableElements.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.
*/
Expand Down Expand Up @@ -119,7 +136,10 @@ const renderPicker = (column: Column) => {
};

export const InlineTimePickerTemplate = () => {
return html<InlineTimePicker>`<div class="time-pickers">
return html<InlineTimePicker>`<div
class="time-pickers"
@keydown="${(x, { event }) => onBaseKeyDown(x, event as KeyboardEvent)}"
>
${renderPicker(HoursColumn)} ${renderPicker(MinutesColumn)}
${when(shouldDisplaySecondsPicker, renderPicker(SecondsColumn))}
${when(shouldDisplay12hClock, renderPicker(MeridiesColumn))}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand Down
14 changes: 14 additions & 0 deletions libs/components/src/shared/patterns/trapped-focus.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
8 changes: 7 additions & 1 deletion libs/components/src/shared/patterns/trapped-focus.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
export class TrappedFocus {
private static ignoredEvents = new WeakSet<Event>();

static ignoreEvent(event: Event) {
this.ignoredEvents.add(event);
}

/**
* @returns Whether focus was trapped.
* @internal
Expand All @@ -7,7 +13,7 @@ export class TrappedFocus {
event: KeyboardEvent,
getFocusableEls: () => NodeListOf<HTMLElement>
) {
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];
Expand Down
Loading