diff --git a/src/card-controller/keyboard-state-manager.ts b/src/card-controller/keyboard-state-manager.ts index b9863924..ff539a94 100644 --- a/src/card-controller/keyboard-state-manager.ts +++ b/src/card-controller/keyboard-state-manager.ts @@ -142,8 +142,12 @@ export class KeyboardStateManager { return; } - // Taking focus must not scroll the dashboard to bring the card into view. - element.focus({ preventScroll: true }); + // Taking focus must not scroll the dashboard to bring the card into view, + // nor summon the browser's focus ring: script-initiated focus counts as + // keyboard-like and would draw the ring around the entire card on a plain + // pointer press. Tabbing to the card does not pass through here, so + // keyboard users keep their ring. + element.focus({ preventScroll: true, focusVisible: false }); }; private _handleBlur = (ev: FocusEvent): void => { diff --git a/src/components-lib/notification/notification-popup-controller.ts b/src/components-lib/notification/notification-popup-controller.ts index b7dc010c..4bf03b47 100644 --- a/src/components-lib/notification/notification-popup-controller.ts +++ b/src/components-lib/notification/notification-popup-controller.ts @@ -55,7 +55,7 @@ export class NotificationPopupController implements ReactiveController { this._elementFocusedBeforePopup instanceof HTMLElement && document.activeElement === document.body ) { - this._elementFocusedBeforePopup.focus(); + this._elementFocusedBeforePopup.focus({ focusVisible: false }); } this._elementFocusedBeforePopup = null; } diff --git a/src/declarations.d.ts b/src/declarations.d.ts index 5ed73d12..f488ece9 100644 --- a/src/declarations.d.ts +++ b/src/declarations.d.ts @@ -21,3 +21,9 @@ declare module 'action' { // eslint-disable-next-line @typescript-eslint/no-empty-object-type interface ActionContext {} } + +// The `focusVisible` option is part of the focus specification, but is absent +// from the bundled TypeScript DOM types. +interface FocusOptions { + focusVisible?: boolean; +} diff --git a/tests/card-controller/keyboard-state-manager.browser.test.ts b/tests/card-controller/keyboard-state-manager.browser.test.ts index 754bc0be..8844d26f 100644 --- a/tests/card-controller/keyboard-state-manager.browser.test.ts +++ b/tests/card-controller/keyboard-state-manager.browser.test.ts @@ -14,6 +14,7 @@ import { getFocusedElement, holdKey, pressKey, + pressTab, releaseKey, } from '../browser/dom'; import { @@ -232,6 +233,31 @@ describe('KeyboardStateManager', () => { await card.console.waitForMessage(KEY_MESSAGE); }); + it('should not draw a focus indicator when it takes focus', async () => { + const card = await mountCard(); + + await clickMedia(card); + + expect(getFocusedElement()).toBe(card.card); + + // Focus taken by script counts as keyboard-driven, and the browser rings + // the whole card for it: a bright border around a card the user only + // pressed. + expect(card.card.matches(':focus-visible')).toBe(false); + }); + + it('should draw a focus indicator when it is reached with the keyboard', async () => { + const card = await mountCard(); + + await pressTab(); + + expect(getFocusedElement()).toBe(card.card); + + // The card is in the tab order, and a user who arrives on it that way needs + // to be able to see where they are. + expect(card.card.matches(':focus-visible')).toBe(true); + }); + it('should not scroll the page when it takes focus', async () => { // Well below the window, so the card is out of sight until the page is // scrolled to it. diff --git a/tests/card-controller/keyboard-state-manager.test.ts b/tests/card-controller/keyboard-state-manager.test.ts index 4213416f..0168d123 100644 --- a/tests/card-controller/keyboard-state-manager.test.ts +++ b/tests/card-controller/keyboard-state-manager.test.ts @@ -243,13 +243,13 @@ describe('KeyboardStateManager', () => { }); }); - it('should take focus on pointerdown', () => { + it('should take focus on pointerdown without a visible focus ring', () => { const { element } = createManager(); const focus = vi.spyOn(element, 'focus'); element.dispatchEvent(new Event('pointerdown')); - expect(focus).toHaveBeenCalledWith({ preventScroll: true }); + expect(focus).toHaveBeenCalledWith({ preventScroll: true, focusVisible: false }); }); it('should not take focus on pointerdown when focus is already within the card', () => { diff --git a/tests/components-lib/notification/notification-popup-controller.browser.test.ts b/tests/components-lib/notification/notification-popup-controller.browser.test.ts index 46e96c75..e1ff341f 100644 --- a/tests/components-lib/notification/notification-popup-controller.browser.test.ts +++ b/tests/components-lib/notification/notification-popup-controller.browser.test.ts @@ -57,6 +57,14 @@ const showNotification = async (card: MountedCard): Promise => { return await card.waitForSelector('.notification'); }; +const dismissNotification = async (card: MountedCard): Promise => { + await pressKey('Escape'); + await card.waitForRender( + () => (deepQuery(card.card, '.notification') ? null : true), + 'the notification to be removed', + ); +}; + describe('NotificationPopupController', () => { it('should keep the notification open when its own text is pressed', async () => { const card = await mount(); @@ -132,15 +140,29 @@ describe('NotificationPopupController', () => { await showNotification(card); expect(getFocusedElement()).not.toBe(elsewhere); - await pressKey('Escape'); - await card.waitForRender( - () => (deepQuery(card.card, '.notification') ? null : true), - 'the notification to be removed', - ); + await dismissNotification(card); expect(getFocusedElement()).toBe(elsewhere); }); + it('should return focus without a visible focus ring', async () => { + const card = await mount(); + await card.console.waitForMessage(CARD_INITIALIZED_MESSAGE); + + const elsewhere = document.createElement('button'); + document.body.appendChild(elsewhere); + + // Focused as a pointer press leaves it: with no ring, which is the state + // the return of focus must not change. + elsewhere.focus({ focusVisible: false }); + + await showNotification(card); + await dismissNotification(card); + + expect(getFocusedElement()).toBe(elsewhere); + expect(elsewhere.matches(':focus-visible')).toBe(false); + }); + it('should activate a notification control from the keyboard', async () => { const card = await mount({ ...NOTIFICATION, diff --git a/tests/components-lib/notification/notification-popup-controller.test.ts b/tests/components-lib/notification/notification-popup-controller.test.ts index de3d04e3..607fb9b0 100644 --- a/tests/components-lib/notification/notification-popup-controller.test.ts +++ b/tests/components-lib/notification/notification-popup-controller.test.ts @@ -133,6 +133,22 @@ describe('NotificationPopupController', () => { expect(document.activeElement).toBe(before); }); + it('should return focus without a visible focus ring', () => { + const before = document.createElement('button'); + document.body.appendChild(before); + before.focus(); + const focus = vi.spyOn(before, 'focus'); + + const popup = createFocusablePopup(); + const { controller } = create(() => popup); + controller.hostUpdated(); + popup.remove(); + + controller.hostDisconnected(); + + expect(focus).toHaveBeenCalledWith({ focusVisible: false }); + }); + it('should leave focus alone when something else has taken it', () => { const before = document.createElement('button'); document.body.appendChild(before);